fix: stop consensus_is_ready from carrying a stale node_mode label - #2544
Open
Thiago316316 wants to merge 7 commits into
Open
fix: stop consensus_is_ready from carrying a stale node_mode label#2544Thiago316316 wants to merge 7 commits into
Thiago316316 wants to merge 7 commits into
Conversation
node_mode was injected as an implicit label on every metric (including consensus_is_ready) by the generic gauge/counter/histogram codegen, read fresh from global state at call time. Since a node is only ever in one mode, labeling a singleton reading by a mutable dimension meant every leader/follower swap created a brand-new Prometheus series while leaving the old one registered forever (metrics-exporter-prometheus never expires a series without an explicit idle_timeout, which isn't configured here) - so consensus_is_ready ended up reporting both leader and follower labels simultaneously after any swap. Add a gauge_no_auto_label metric kind that skips the automatic node_mode label, and use it for consensus_is_ready so it's structurally a single series. Node mode itself is now exposed as its own dedicated stratus_node_mode gauge, refreshed for all known modes together on every health check tick so no per-mode series can go stale independently. Fixes cloudwalk#2529
![cloudwalk-review-agent[bot]](https://avatars.githubusercontent.com/u/6581007?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice fix — this addresses the stale-series root cause in a concrete way.
What I validated
consensus_is_readyno longer carries the autonode_modelabel (gauge_no_auto_labelpath), preventing mode-flip label fanout for this health metric.- A dedicated
node_mode{mode}gauge is introduced and explicitly maintained as one-hot inupdate_health(). - Metric type registration was updated so
gauge_no_auto_labelis described correctly (metrics_types.rs). - Health/role metric refresh is triggered immediately on mode transitions (leader/follower paths), reducing observability lag.
Risk check
No issues found in the diff related to auth, permissions, money movement, migration/deploy safety, or API contract regressions. Changes are scoped to metrics emission behavior and appear internally consistent.
Author
|
Hi @carneiro-cw my PR passed the agent, if you have some time to review it, i will be grateful. Thanks!! |
Contributor
|
I'm a bit pressed for time this week. But I'll get to it! |
…el' into fix/consensus-is-ready-stale-label
There was a problem hiding this comment.
Summary
LGTM. The change cleanly addresses the stale node_mode label issue by decoupling health (consensus_is_ready) from role labeling and introducing explicit one-hot node_mode{mode} updates.
What I validated in the diff:
gauge_no_auto_labelwas added to metric generation and wired into metric description registration.consensus_is_readynow usesgauge_no_auto_label, preventing label fanout across mode flips.node_mode{mode}metric was added and is updated across all known modes inupdate_health().update_health()is now called on leader/follower transitions (including follower-init failure rollback), reducing metric staleness windows after mode changes.
No concrete correctness/security/deploy-safety issues found in the provided changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @carneiro-cw I first tried the same idea as #2123 (129ff91) — reset stale state right at the mode-flip point — but applied to the metric value instead of a cache. That zeroed the value but didn't remove the duplicate series itself, since Prometheus doesn't garbage-collect a label combination. The final one involved more layers of the system but was a proper correction, i think.
the fix works in two steps:
1° i added a new gauge/leg on metrics_impl_fn_inc to remove the automatic node mode labeling, with the new leg that didin't pass this, the metric consensus_is_ready (a health reading), just emit 0 or 1 depending on the readiness/health.
2° the responsability to show wich role the node has now is of node_mode{mode} (declared right after it in the source) and the process happens on the update_health, that iterates over all 3 modes and the one that correspond to the currently state get the 1, the rest are zeroed.
Plus: i put ctx.server.update_health().await; on every mode/role swap, so the info about the leader is more recent as possible.
manual test:
Fixes #2529