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

only add prom autopilot gauges to servers #11241

Merged
merged 1 commit into from
Oct 13, 2021
Merged

Conversation

acpana
Copy link
Contributor

@acpana acpana commented Oct 6, 2021

Overview

This PR makes it such that consul autopilot gauges are not configured for PrometheusOpts for non server agents (clients).

For clients, there is no documented behavior on https://www.consul.io/docs/agent/telemetry#autopilot. In 1.9 consul emitted NaN for these metrics. In 1.10, by virtue of changes to the underlying go-metrics lib, consul emits 0.

This change proposes not emitting these metrics at all for clients.

Issue Related

Base automatically changed from ffmmm/b-10730 to main October 8, 2021 17:31
@vercel vercel bot temporarily deployed to Preview – consul October 8, 2021 19:36 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging October 8, 2021 19:36 Inactive
@vercel vercel bot temporarily deployed to Preview – consul October 8, 2021 19:49 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging October 8, 2021 19:49 Inactive
@acpana acpana marked this pull request as ready for review October 8, 2021 19:51
@acpana acpana added backport/1.10 pr/do-not-merge PR cannot be merged in its current form. labels Oct 8, 2021
@acpana acpana removed the pr/do-not-merge PR cannot be merged in its current form. label Oct 12, 2021
Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. Backporting to 1.10 seems like a good thing, but probably not 1.9.

@@ -216,6 +216,11 @@ func getPrometheusDefs(cfg lib.TelemetryConfig) ([]prometheus.GaugeDefinition, [
raftGauges,
}

// TODO(ffmmm): conditionally add only leader specific metrics to gauges, counters, summaries, etc
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this comment

Seems like it would be worth it to move all the server-only metrics in this PR, since they are all here in this same area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed that we have the opportunity to separate our metrics. And also communicate that on the docs.

I'd rather us wait for hashicorp/go-metrics#129 to go in.

That way, we can programmatically test over all the metrics for the desired behavior. ie appear on servers, not appear on clients.

I can write out a quick issue around this.

For now, I'd love for us to merge this in so we can fully close #10730

@acpana acpana merged commit 62980ff into main Oct 13, 2021
@acpana acpana deleted the ffmmm/b-10730-client-fix branch October 13, 2021 16:25
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/471498.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit 62980ff onto release/1.10.x succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants