Repository: traefik/traefik (language: go)

Issue:
ConfigReloadsFailureCounter metric does not increment on config reload failures
<!-- PLEASE FOLLOW THE ISSUE TEMPLATE TO HELP TRIAGE AND SUPPORT! -->

### Do you want to request a *feature* or report a *bug*?

<!--
DO NOT FILE ISSUES FOR GENERAL SUPPORT QUESTIONS.

The issue tracker is for reporting bugs and feature requests only.
For end-user related support questions, please refer to one of the following:

- the Traefik community forum: https://community.containo.us/

-->

Bug

<!--

The configurations between 1.X and 2.X are NOT compatible.
Please have a look here https://doc.traefik.io/traefik/getting-started/configuration-overview/.

-->

### What did you do?

I introduced deliberately an error in the dynamic config load sequence (404 response to the HTTP provider).

<!--

HOW TO WRITE A GOOD BUG REPORT?

- Respect the issue template as much as possible.
- The title should be short and descriptive.
- Explain the conditions which led you to report this issue: the context.
- The context should lead to something, an idea or a problem that you’re facing.
- Remain clear and concise.
- Format your messages to help the reader focus on what matters and understand the structure of your message, use Markdown syntax https://help.github.com/articles/github-flavored-markdown

-->

### What did you expect to see?

I expected to see an increment in the metric `ConfigReloadsFailureCounter`. In this particular case I'm using DataDog so the metric should be `config.reload.total` with the tag `failure: true`. 

### What did you see instead?

I did not see the metric mentioned above with the tag `failure: true`.

### Output of `traefik version`: (_What version of Traefik are you using?_)

<!--
`latest` is not considered as a valid version.

For the Traefik Docker image:
    docker run [IMAGE] version
    ex: docker run traefik version

-->

```
Version:      2.3.1
Codename:     picodon
Go version:   go1.15.2
Built:        2020-09-29T15:49:06Z
OS/Arch:      linux/amd64
```

### What is your environment & configuration (arguments, toml, provider, platform, ...)?

```yaml
entryPoints:
  Main:
    address: ":8888"
    transport:
      lifeCycle:
        requestAcceptGraceTimeout: 42
        graceTimeOut: 42
      respondingTimeouts:
        readTimeout: 42
        writeTimeout: 42
        idleTimeout: 42
providers:
  http:
    endpoint: "http://127.0.0.1:8000/dynamic-config.yaml"

log:
  level: INFO
tracing:
  datadog: {}
metrics:
  datadog: {}

ping:
  entryPoint: "Main"
```

<!--
Add more configuration information here.
-->


### If applicable, please paste the log output in DEBUG level (`--log.level=DEBUG` switch)

```
N/A
```



Discussion / hints:
Hi @pbarsotti-glovo,

Thanks for your interest in Traefik!

After looking deeper into the code, you're right the `ConfigReloadsFailureCounter` is never used and incremented (by all the providers to be accurate). After a very quick look, we might see two ways to implement this metric by: 

- Modifying each provider to increment the `ConfigReloadsFailureCounter` when something went wrong.
- Modifying the `configuration` channel to forward a configuration reload error to the configuration `watcher`.

Would you be interested to look into this?

Hey @kevinpollet I just submitted a PR. Feedback is much appreciated. 
When can we expect this fix?

In our scenario we are using a periodic reload from an S3 bucket to fetch the latest configuration, having this metric is essential to our reporting.

Fix patch:
diff --git a/pkg/metrics/datadog.go b/pkg/metrics/datadog.go
index ecc7ba33bd..37442e7f2a 100644
--- a/pkg/metrics/datadog.go
+++ b/pkg/metrics/datadog.go
@@ -19,9 +19,7 @@ var (
 // Metric names consistent with https://github.com/DataDog/integrations-extras/pull/64
 const (
 	ddConfigReloadsName             = "config.reload.total"
-	ddConfigReloadsFailureTagName   = "failure"
 	ddLastConfigReloadSuccessName   = "config.reload.lastSuccessTimestamp"
-	ddLastConfigReloadFailureName   = "config.reload.lastFailureTimestamp"
 	ddTLSCertsNotAfterTimestampName = "tls.certs.notAfterTimestamp"
 
 	ddEntryPointReqsName        = "entrypoint.request.total"
@@ -64,9 +62,7 @@ func RegisterDatadog(ctx context.Context, config *types.Datadog) Registry {
 
 	registry := &standardRegistry{
 		configReloadsCounter:           datadogClient.NewCounter(ddConfigReloadsName, 1.0),
-		configReloadsFailureCounter:    datadogClient.NewCounter(ddConfigReloadsName, 1.0).With(ddConfigReloadsFailureTagName, "true"),
 		lastConfigReloadSuccessGauge:   datadogClient.NewGauge(ddLastConfigReloadSuccessName),
-		lastConfigReloadFailureGauge:   datadogClient.NewGauge(ddLastConfigReloadFailureName),
 		tlsCertsNotAfterTimestampGauge: datadogClient.NewGauge(ddTLSCertsNotAfterTimestampName),
 	}
 
diff --git a/pkg/metrics/influxdb2.go b/pkg/metrics/influxdb2.go
index 3c1b2339c3..2ad0555859 100644
--- a/pkg/metrics/influxdb2.go
+++ b/pkg/metrics/influxdb2.go
@@ -25,9 +25,7 @@ var (
 
 const (
 	influxDBConfigReloadsName           = "traefik.config.reload.total"
-	influxDBConfigReloadsFailureName    = influxDBConfigReloadsName + ".failure"
 	influxDBLastConfigReloadSuccessName = "traefik.config.reload.lastSuccessTimestamp"
-	influxDBLastConfigReloadFailureName = "traefik.config.reload.lastFailureTimestamp"
 
 	influxDBTLSCertsNotAfterTimestampName = "traefik.tls.certs.notAfterTimestamp"
 
@@ -84,9 +82,7 @@ func RegisterInfluxDB2(ctx context.Context, config *types.InfluxDB2) Registry {
 
 	registry := &standardRegistry{
 		configReloadsCounter:           influxDB2Store.NewCounter(influxDBConfigReloadsName),
-		configReloadsFailureCounter:    influxDB2Store.NewCounter(influxDBConfigReloadsFailureName),
 		lastConfigReloadSuccessGauge:   influxDB2Store.NewGauge(influxDBLastConfigReloadSuccessName),
-		lastConfigReloadFailureGauge:   influxDB2Store.NewGauge(influxDBLastConfigReloadFailureName),
 		tlsCertsNotAfterTimestampGauge: influxDB2Store.NewGauge(influxDBTLSCertsNotAfterTimestampName),
 	}
 
diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
index ac49d389ac..cf408adb07 100644
--- a/pkg/metrics/metrics.go
+++ b/pkg/metrics/metrics.go
@@ -22,9 +22,7 @@ type Registry interface {
 	// server metrics
 
 	ConfigReloadsCounter() metrics.Counter
-	ConfigReloadsFailureCounter() metrics.Counter
 	LastConfigReloadSuccessGauge() metrics.Gauge
-	LastConfigReloadFailureGauge() metrics.Gauge
 
 	// TLS
 
@@ -71,9 +69,7 @@ func NewVoidRegistry() Registry {
 // This allows for feature disparity between the different metric implementations.
 func NewMultiRegistry(registries []Registry) Registry {
 	var configReloadsCounter []metrics.Counter
-	var configReloadsFailureCounter []metrics.Counter
 	var lastConfigReloadSuccessGauge []metrics.Gauge
-	var lastConfigReloadFailureGauge []metrics.Gauge
 	var tlsCertsNotAfterTimestampGauge []metrics.Gauge
 	var entryPointReqsCounter []metrics.Counter
 	var entryPointReqsTLSCounter []metrics.Counter
@@ -100,15 +96,9 @@ func NewMultiRegistry(registries []Registry) Registry {
 		if r.ConfigReloadsCounter() != nil {
 			configReloadsCounter = append(configReloadsCounter, r.ConfigReloadsCounter())
 		}
-		if r.ConfigReloadsFailureCounter() != nil {
-			configReloadsFailureCounter = append(configReloadsFailureCounter, r.ConfigReloadsFailureCounter())
-		}
 		if r.LastConfigReloadSuccessGauge() != nil {
 			lastConfigReloadSuccessGauge = append(lastConfigReloadSuccessGauge, r.LastConfigReloadSuccessGauge())
 		}
-		if r.LastConfigReloadFailureGauge() != nil {
-			lastConfigReloadFailureGauge = append(lastConfigReloadFailureGauge, r.LastConfigReloadFailureGauge())
-		}
 		if r.TLSCertsNotAfterTimestampGauge() != nil {
 			tlsCertsNotAfterTimestampGauge = append(tlsCertsNotAfterTimestampGauge, r.TLSCertsNotAfterTimestampGauge())
 		}
@@ -179,9 +169,7 @@ func NewMultiRegistry(registries []Registry) Registry {
 		svcEnabled:                     len(serviceReqsCounter) > 0 || len(serviceReqDurationHistogram) > 0 || len(serviceOpenConnsGauge) > 0 || len(serviceRetriesCounter) > 0 || len(serviceServerUpGauge) > 0,
 		routerEnabled:                  len(routerReqsCounter) > 0 || len(routerReqDurationHistogram) > 0 || len(routerOpenConnsGauge) > 0,
 		configReloadsCounter:           multi.NewCounter(configReloadsCounter...),
-		configReloadsFailureCounter:    multi.NewCounter(configReloadsFailureCounter...),
 		lastConfigReloadSuccessGauge:   multi.NewGauge(lastConfigReloadSuccessGauge...),
-		lastConfigReloadFailureGauge:   multi.NewGauge(lastConfigReloadFailureGauge...),
 		tlsCertsNotAfterTimestampGauge: multi.NewGauge(tlsCertsNotAfterTimestampGauge...),
 		entryPointReqsCounter:          multi.NewCounter(entryPointReqsCounter...),
 		entryPointReqsTLSCounter:       multi.NewCounter(entryPointReqsTLSCounter...),
@@ -211,9 +199,7 @@ type standardRegistry struct {
 	routerEnabled                  bool
 	svcEnabled                     bool
 	configReloadsCounter           metrics.Counter
-	configReloadsFailureCounter    metrics.Counter
 	lastConfigReloadSuccessGauge   metrics.Gauge
-	lastConfigReloadFailureGauge   metrics.Gauge
 	tlsCertsNotAfterTimestampGauge metrics.Gauge
 	entryPointReqsCounter          metrics.Counter
 	entryPointReqsTLSCounter       metrics.Counter
@@ -253,18 +239,10 @@ func (r *standardRegistry) ConfigReloadsCounter() metrics.Counter {
 	return r.configReloadsCounter
 }
 
-func (r *standardRegistry) ConfigReloadsFailureCounter() metrics.Counter {
-	return r.configReloadsFailureCounter
-}
-
 func (r *standardRegistry) LastConfigReloadSuccessGauge() metrics.Gauge {
 	return r.lastConfigReloadSuccessGauge
 }
 
-func (r *standardRegistry) LastConfigReloadFailureGauge() metrics.Gauge {
-	return r.lastConfigReloadFailureGauge
-}
-
 func (r *standardRegistry) TLSCertsNotAfterTimestampGauge() metrics.Gauge {
 	return r.tlsCertsNotAfterTimestampGauge
 }
diff --git a/pkg/metrics/opentelemetry.go b/pkg/metrics/opentelemetry.go
index 1a8dd4d6c5..8afeb54bae 100644
--- a/pkg/metrics/opentelemetry.go
+++ b/pkg/metrics/opentelemetry.go
@@ -57,9 +57,7 @@ func RegisterOpenTelemetry(ctx context.Context, config *types.OpenTelemetry) Reg
 		routerEnabled:                  config.AddRoutersLabels,
 		svcEnabled:                     config.AddServicesLabels,
 		configReloadsCounter:           newOTLPCounterFrom(meter, configReloadsTotalName, "Config reloads"),
-		configReloadsFailureCounter:    newOTLPCounterFrom(meter, configReloadsFailuresTotalName, "Config reload failures"),
 		lastConfigReloadSuccessGauge:   newOTLPGaugeFrom(meter, configLastReloadSuccessName, "Last config reload success", unit.Milliseconds),
-		lastConfigReloadFailureGauge:   newOTLPGaugeFrom(meter, configLastReloadFailureName, "Last config reload failure", unit.Milliseconds),
 		tlsCertsNotAfterTimestampGauge: newOTLPGaugeFrom(meter, tlsCertsNotAfterTimestamp, "Certificate expiration timestamp", unit.Milliseconds),
 	}
 
diff --git a/pkg/metrics/prometheus.go b/pkg/metrics/prometheus.go
index 7be5d504bf..ed27cedcb0 100644
--- a/pkg/metrics/prometheus.go
+++ b/pkg/metrics/prometheus.go
@@ -21,11 +21,9 @@ const (
 	MetricNamePrefix = "traefik_"
 
 	// server meta information.
-	metricConfigPrefix             = MetricNamePrefix + "config_"
-	configReloadsTotalName         = metricConfigPrefix + "reloads_total"
-	configReloadsFailuresTotalName = metricConfigPrefix + "reloads_failure_total"
-	configLastReloadSuccessName    = metricConfigPrefix + "last_reload_success"
-	configLastReloadFailureName    = metricConfigPrefix + "last_reload_failure"
+	metricConfigPrefix          = MetricNamePrefix + "config_"
+	configReloadsTotalName      = metricConfigPrefix + "reloads_total"
+	configLastReloadSuccessName = metricConfigPrefix + "last_reload_success"
 
 	// TLS.
 	metricsTLSPrefix          = MetricNamePrefix + "tls_"
@@ -118,18 +116,10 @@ func initStandardRegistry(config *types.Prometheus) Registry {
 		Name: configReloadsTotalName,
 		Help: "Config reloads",
 	}, []string{})
-	configReloadsFailures := newCounterFrom(stdprometheus.CounterOpts{
-		Name: configReloadsFailuresTotalName,
-		Help: "Config failure reloads",
-	}, []string{})
 	lastConfigReloadSuccess := newGaugeFrom(stdprometheus.GaugeOpts{
 		Name: configLastReloadSuccessName,
 		Help: "Last config reload success",
 	}, []string{})
-	lastConfigReloadFailure := newGaugeFrom(stdprometheus.GaugeOpts{
-		Name: configLastReloadFailureName,
-		Help: "Last config reload failure",
-	}, []string{})
 	tlsCertsNotAfterTimestamp := newGaugeFrom(stdprometheus.GaugeOpts{
 		Name: tlsCertsNotAfterTimestamp,
 		Help: "Certificate expiration timestamp",
@@ -137,9 +127,7 @@ func initStandardRegistry(config *types.Prometheus) Registry {
 
 	promState.vectors = []vector{
 		configReloads.cv,
-		configReloadsFailures.cv,
 		lastConfigReloadSuccess.gv,
-		lastConfigReloadFailure.gv,
 		tlsCertsNotAfterTimestamp.gv,
 	}
 
@@ -148,9 +136,7 @@ func initStandardRegistry(config *types.Prometheus) Registry {
 		routerEnabled:                  config.AddRoutersLabels,
 		svcEnabled:                     config.AddServicesLabels,
 		configReloadsCounter:           configReloads,
-		configReloadsFailureCounter:    configReloadsFailures,
 		lastConfigReloadSuccessGauge:   lastConfigReloadSuccess,
-		lastConfigReloadFailureGauge:   lastConfigReloadFailure,
 		tlsCertsNotAfterTimestampGauge: tlsCertsNotAfterTimestamp,
 	}
 
diff --git a/pkg/metrics/statsd.go b/pkg/metrics/statsd.go
index c2e1027aba..cc2664f8f2 100644
--- a/pkg/metrics/statsd.go
+++ b/pkg/metrics/statsd.go
@@ -18,9 +18,7 @@ var (
 
 const (
 	statsdConfigReloadsName           = "config.reload.total"
-	statsdConfigReloadsFailureName    = statsdConfigReloadsName + ".failure"
 	statsdLastConfigReloadSuccessName = "config.reload.lastSuccessTimestamp"
-	statsdLastConfigReloadFailureName = "config.reload.lastFailureTimestamp"
 
 	statsdTLSCertsNotAfterTimestampName = "tls.certs.notAfterTimestamp"
 
@@ -63,9 +61,7 @@ func RegisterStatsd(ctx context.Context, config *types.Statsd) Registry {
 
 	registry := &standardRegistry{
 		configReloadsCounter:           statsdClient.NewCounter(statsdConfigReloadsName, 1.0),
-		configReloadsFailureCounter:    statsdClient.NewCounter(statsdConfigReloadsFailureName, 1.0),
 		lastConfigReloadSuccessGauge:   statsdClient.NewGauge(statsdLastConfigReloadSuccessName),
-		lastConfigReloadFailureGauge:   statsdClient.NewGauge(statsdLastConfigReloadFailureName),
 		tlsCertsNotAfterTimestampGauge: statsdClient.NewGauge(statsdTLSCertsNotAfterTimestampName),
 	}
 


Test patch:
diff --git a/pkg/metrics/datadog_test.go b/pkg/metrics/datadog_test.go
index ea837fb523..5a02b4b94e 100644
--- a/pkg/metrics/datadog_test.go
+++ b/pkg/metrics/datadog_test.go
@@ -45,9 +45,7 @@ func testDatadogRegistry(t *testing.T, metricsPrefix string, datadogRegistry Reg
 
 	expected := []string{
 		metricsPrefix + ".config.reload.total:1.000000|c\n",
-		metricsPrefix + ".config.reload.total:1.000000|c|#failure:true\n",
 		metricsPrefix + ".config.reload.lastSuccessTimestamp:1.000000|g\n",
-		metricsPrefix + ".config.reload.lastFailureTimestamp:1.000000|g\n",
 
 		metricsPrefix + ".tls.certs.notAfterTimestamp:1.000000|g|#key:value\n",
 
@@ -80,9 +78,7 @@ func testDatadogRegistry(t *testing.T, metricsPrefix string, datadogRegistry Reg
 
 	udp.ShouldReceiveAll(t, expected, func() {
 		datadogRegistry.ConfigReloadsCounter().Add(1)
-		datadogRegistry.ConfigReloadsFailureCounter().Add(1)
 		datadogRegistry.LastConfigReloadSuccessGauge().Add(1)
-		datadogRegistry.LastConfigReloadFailureGauge().Add(1)
 
 		datadogRegistry.TLSCertsNotAfterTimestampGauge().With("key", "value").Set(1)
 
diff --git a/pkg/metrics/influxdb2_test.go b/pkg/metrics/influxdb2_test.go
index c93fe9f13d..86f952503e 100644
--- a/pkg/metrics/influxdb2_test.go
+++ b/pkg/metrics/influxdb2_test.go
@@ -47,15 +47,11 @@ func TestInfluxDB2(t *testing.T) {
 
 	expectedServer := []string{
 		`(traefik\.config\.reload\.total count=1) [\d]{19}`,
-		`(traefik\.config\.reload\.total\.failure count=1) [\d]{19}`,
 		`(traefik\.config\.reload\.lastSuccessTimestamp value=1) [\d]{19}`,
-		`(traefik\.config\.reload\.lastFailureTimestamp value=1) [\d]{19}`,
 	}
 
 	influxDB2Registry.ConfigReloadsCounter().Add(1)
-	influxDB2Registry.ConfigReloadsFailureCounter().Add(1)
 	influxDB2Registry.LastConfigReloadSuccessGauge().Set(1)
-	influxDB2Registry.LastConfigReloadFailureGauge().Set(1)
 	msgServer := <-c
 
 	assertMessage(t, *msgServer, expectedServer)
diff --git a/pkg/metrics/opentelemetry_test.go b/pkg/metrics/opentelemetry_test.go
index 8cf4d070bf..15d9a214ed 100644
--- a/pkg/metrics/opentelemetry_test.go
+++ b/pkg/metrics/opentelemetry_test.go
@@ -340,15 +340,11 @@ func TestOpenTelemetry(t *testing.T) {
 	// TODO: the len of startUnixNano is no supposed to be 20, it should be 19
 	expected = append(expected,
 		`({"name":"traefik_config_reloads_total","description":"Config reloads","unit":"1","sum":{"dataPoints":\[{"startTimeUnixNano":"[\d]{19}","timeUnixNano":"[\d]{19}","asDouble":1}\],"aggregationTemporality":2,"isMonotonic":true}})`,
-		`({"name":"traefik_config_reloads_failure_total","description":"Config reload failures","unit":"1","sum":{"dataPoints":\[{"startTimeUnixNano":"[\d]{19}","timeUnixNano":"[\d]{19}","asDouble":1}\],"aggregationTemporality":2,"isMonotonic":true}})`,
 		`({"name":"traefik_config_last_reload_success","description":"Last config reload success","unit":"ms","gauge":{"dataPoints":\[{"startTimeUnixNano":"[\d]{20}","timeUnixNano":"[\d]{19}","asDouble":1}\]}})`,
-		`({"name":"traefik_config_last_reload_failure","description":"Last config reload failure","unit":"ms","gauge":{"dataPoints":\[{"startTimeUnixNano":"[\d]{20}","timeUnixNano":"[\d]{19}","asDouble":1}\]}})`,
 	)
 
 	registry.ConfigReloadsCounter().Add(1)
-	registry.ConfigReloadsFailureCounter().Add(1)
 	registry.LastConfigReloadSuccessGauge().Set(1)
-	registry.LastConfigReloadFailureGauge().Set(1)
 	msgServer := <-c
 
 	assertMessage(t, *msgServer, expected)
diff --git a/pkg/metrics/prometheus_test.go b/pkg/metrics/prometheus_test.go
index 468bbc8983..a631cf1cdc 100644
--- a/pkg/metrics/prometheus_test.go
+++ b/pkg/metrics/prometheus_test.go
@@ -100,9 +100,7 @@ func TestPrometheus(t *testing.T) {
 	}
 
 	prometheusRegistry.ConfigReloadsCounter().Add(1)
-	prometheusRegistry.ConfigReloadsFailureCounter().Add(1)
 	prometheusRegistry.LastConfigReloadSuccessGauge().Set(float64(time.Now().Unix()))
-	prometheusRegistry.LastConfigReloadFailureGauge().Set(float64(time.Now().Unix()))
 
 	prometheusRegistry.
 		TLSCertsNotAfterTimestampGauge().
@@ -201,18 +199,10 @@ func TestPrometheus(t *testing.T) {
 			name:   configReloadsTotalName,
 			assert: buildCounterAssert(t, configReloadsTotalName, 1),
 		},
-		{
-			name:   configReloadsFailuresTotalName,
-			assert: buildCounterAssert(t, configReloadsFailuresTotalName, 1),
-		},
 		{
 			name:   configLastReloadSuccessName,
 			assert: buildTimestampAssert(t, configLastReloadSuccessName),
 		},
-		{
-			name:   configLastReloadFailureName,
-			assert: buildTimestampAssert(t, configLastReloadFailureName),
-		},
 		{
 			name: tlsCertsNotAfterTimestamp,
 			labels: map[string]string{
diff --git a/pkg/metrics/statsd_test.go b/pkg/metrics/statsd_test.go
index 73aeb0af90..611b26df8f 100644
--- a/pkg/metrics/statsd_test.go
+++ b/pkg/metrics/statsd_test.go
@@ -49,9 +49,7 @@ func testRegistry(t *testing.T, metricsPrefix string, registry Registry) {
 
 	expected := []string{
 		metricsPrefix + ".config.reload.total:1.000000|c\n",
-		metricsPrefix + ".config.reload.total.failure:1.000000|c\n",
 		metricsPrefix + ".config.reload.lastSuccessTimestamp:1.000000|g\n",
-		metricsPrefix + ".config.reload.lastFailureTimestamp:1.000000|g\n",
 
 		metricsPrefix + ".tls.certs.notAfterTimestamp:1.000000|g\n",
 
@@ -81,9 +79,7 @@ func testRegistry(t *testing.T, metricsPrefix string, registry Registry) {
 
 	udp.ShouldReceiveAll(t, expected, func() {
 		registry.ConfigReloadsCounter().Add(1)
-		registry.ConfigReloadsFailureCounter().Add(1)
 		registry.LastConfigReloadSuccessGauge().Set(1)
-		registry.LastConfigReloadFailureGauge().Set(1)
 
 		registry.TLSCertsNotAfterTimestampGauge().With("key", "value").Set(1)