-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
c29092c
to
096fde6
Compare
Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
096fde6
to
202495f
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
🍒 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. |
🍒✅ Cherry pick of commit 62980ff onto |
Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
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 emittedNaN
for these metrics. In1.10
, by virtue of changes to the underlyinggo-metrics
lib, consul emits0
.This change proposes not emitting these metrics at all for clients.
Issue Related
consul.autopilot.healthy|consul_autpilot_healthy
metric in Consul 1.10 #10730