Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark disable_compat_1.9 to deprecate in 1.13, change default to true #12675

Merged
merged 6 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/12675.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:breaking-change
telemetry: the disable_compat_1.9 option now defaults to true. 1.9 style `consul.http...` metrics cam still be enabled by setting `disable_compat_1.9 = false`. However, we will remove these metrics in 1.13.
acpana marked this conversation as resolved.
Show resolved Hide resolved
```
4 changes: 2 additions & 2 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ func (a *Agent) Start(ctx context.Context) error {
// DEPRECATED: Warn users if they're emitting deprecated metrics. Remove this warning and the flagged metrics in a
// future release of Consul.
if !a.config.Telemetry.DisableCompatOneNine {
a.logger.Warn("DEPRECATED Backwards compatibility with pre-1.9 metrics enabled. These metrics will be removed in a future version of Consul. Set `telemetry { disable_compat_1.9 = true }` to disable them.")
a.logger.Warn("DEPRECATED Backwards compatibility with pre-1.9 metrics enabled. These metrics will be removed in Consul 1.13. Consider not using this flag and rework instrumentation for 1.10 style http metrics.")
}

if a.tlsConfigurator.Cert() != nil {
Expand Down Expand Up @@ -3711,7 +3711,7 @@ func (a *Agent) ReloadConfig() error {
// DEPRECATED: Warn users on reload if they're emitting deprecated metrics. Remove this warning and the flagged
// metrics in a future release of Consul.
if !a.config.Telemetry.DisableCompatOneNine {
a.logger.Warn("DEPRECATED Backwards compatibility with pre-1.9 metrics enabled. These metrics will be removed in a future version of Consul. Set `telemetry { disable_compat_1.9 = true }` to disable them.")
a.logger.Warn("DEPRECATED Backwards compatibility with pre-1.9 metrics enabled. These metrics will be removed in Consul 1.13. Consider not using this flag and rework instrumentation for 1.10 style http metrics.")
}

return a.reloadConfigInternal(newCfg)
Expand Down
2 changes: 1 addition & 1 deletion agent/config/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@ func (b *builder) build() (rt RuntimeConfig, err error) {
CirconusCheckTags: stringVal(c.Telemetry.CirconusCheckTags),
CirconusSubmissionInterval: stringVal(c.Telemetry.CirconusSubmissionInterval),
CirconusSubmissionURL: stringVal(c.Telemetry.CirconusSubmissionURL),
DisableCompatOneNine: boolVal(c.Telemetry.DisableCompatOneNine),
DisableCompatOneNine: boolValWithDefault(c.Telemetry.DisableCompatOneNine, true),
DisableHostname: boolVal(c.Telemetry.DisableHostname),
DogstatsdAddr: stringVal(c.Telemetry.DogstatsdAddr),
DogstatsdTags: c.Telemetry.DogstatsdTags,
Expand Down
3 changes: 1 addition & 2 deletions agent/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,7 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler {
labels := []metrics.Label{{Name: "method", Value: req.Method}, {Name: "path", Value: path_label}}
metrics.MeasureSinceWithLabels([]string{"api", "http"}, start, labels)

// DEPRECATED Emit pre-1.9 metric as `consul.http...` to maintain backwards compatibility. Enabled by
// default. Users may set `telemetry { disable_compat_1.9 = true }`
// DEPRECATED Emit pre-1.9 metric as `consul.http...`. This will be removed in 1.13.
if !s.agent.config.Telemetry.DisableCompatOneNine {
key := append([]string{"http", req.Method}, parts...)
metrics.MeasureSince(key, start)
Expand Down
61 changes: 61 additions & 0 deletions agent/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,67 @@ func TestHTTPHandlers_AgentMetrics_ConsulAutopilot_Prometheus(t *testing.T) {
})
}

// TestHTTPHandlers_AgentMetrics_Disable1Dot9MetricsChange adds testing around the 1.9 style metrics
// https://www.consul.io/docs/agent/options#telemetry-disable_compat_1.9
func TestHTTPHandlers_AgentMetrics_Disable1Dot9MetricsChange(t *testing.T) {
skipIfShortTesting(t)
// This test cannot use t.Parallel() since we modify global state, ie the global metrics instance

// 1.9 style http metrics looked like this:
// agent_http_2_http_GET_v1_agent_members{quantile="0.5"} 0.1329520046710968
t.Run("check that no consul.http metrics are emitted by default", func(t *testing.T) {
hcl := `
telemetry = {
prometheus_retention_time = "5s"
disable_hostname = true
metrics_prefix = "agent_http"
}
`

a := StartTestAgent(t, TestAgent{HCL: hcl})
defer a.Shutdown()

// we have to use the `a.srv.handler()` to actually trigger the wrapped function
uri := fmt.Sprintf("http://%s%s", a.HTTPAddr(), "/v1/agent/members")
req, err := http.NewRequest("GET", uri, nil)
require.NoError(t, err)
resp := httptest.NewRecorder()
handler := a.srv.handler(true)
handler.ServeHTTP(resp, req)

respRec := httptest.NewRecorder()
recordPromMetrics(t, a, respRec)

assertMetricNotExists(t, respRec, "agent_http_http_GET_v1_agent_members")
})

t.Run("check that we cam still turn on consul.http metrics", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cam => can

acpana marked this conversation as resolved.
Show resolved Hide resolved
hcl := `
telemetry = {
prometheus_retention_time = "5s",
disable_compat_1.9 = false
metrics_prefix = "agent_http_2"
}
`

a := StartTestAgent(t, TestAgent{HCL: hcl})
defer a.Shutdown()

uri := fmt.Sprintf("http://%s%s", a.HTTPAddr(), "/v1/agent/members")
req, err := http.NewRequest("GET", uri, nil)
require.NoError(t, err)
resp := httptest.NewRecorder()

handler := a.srv.handler(true)
handler.ServeHTTP(resp, req)

respRec := httptest.NewRecorder()
recordPromMetrics(t, a, respRec)

assertMetricExists(t, respRec, "agent_http_2_http_GET_v1_agent_members")
})
}

func TestHTTPHandlers_AgentMetrics_TLSCertExpiry_Prometheus(t *testing.T) {
skipIfShortTesting(t)
// This test cannot use t.Parallel() since we modify global state, ie the global metrics instance
Expand Down
2 changes: 1 addition & 1 deletion website/content/docs/agent/options.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -2175,7 +2175,7 @@ There are also a number of common configuration options supported by all provide
geo location or datacenter, dc:sfo). By default, this is left blank and not used.

- `disable_compat_1.9` ((#telemetry-disable_compat_1.9))
This allows users to disable metrics deprecated in 1.9 so they are no longer emitted, saving on performance and storage in large deployments. Defaults to false.
This allows users to disable metrics deprecated in 1.9 so they are no longer emitted, saving on performance and storage in large deployments. As of 1.12 this defaults to `true` and will be removed, along with 1.9 style http metrics in 1.13.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'saving on performance' is odd, maybe improving performance and reducing storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good! That sentence was there before me but I can try to reword it.

acpana marked this conversation as resolved.
Show resolved Hide resolved

- `disable_hostname` ((#telemetry-disable_hostname))
This controls whether or not to prepend runtime telemetry with the machine's
Expand Down