Document the single scrape target requirement for v2 metrics - #3729
Merged
Conversation
Contributor
Contributor
Contributor
🧠 Redis MemoryFound 5 related items from repository history:
Memory updated at fee6a0f |
Contributor
🧠 Redis MemoryFound 5 related items from repository history:
Memory updated at 0a846ac |
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
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.
Documents that the v2 metrics endpoint is cluster-wide and must be scraped through a single Prometheus target.
Jira: DOC-6906 · Related: KBP-1033, observability#94, Zendesk #165134
Why
Every node runs an aggregator that reads the node list from CCS, scrapes every other node, and returns the same complete cluster-wide result. Envoy routes
:8070/v2to the local aggregator on whichever node is asked, so every node returns a complete and identical copy of the cluster's metrics.That breaks the usual Prometheus convention of one exporter per host. Anyone applying the
node_exporterpattern — or substituting node IPs because they have no cluster FQDN configured — lists every node as a target. The result:sum()-based Grafana panel inflated by the number of targetsA customer spent several weeks unable to trust their CPU and connection dashboards because of this. The docs showed the correct single-target config but never said why it was required or what breaks otherwise.
What changed
scrape_configsexample_index/v2/nodealternative for conventional per-node topology, plus the caveat that aggregation adds thecluster/nodelabels so/v2/nodeomits them_indexcount(up{...})returns1;node_metrics_upreports0for unreachable nodesget-started.md<IP>:8070→<cluster_name>:8070— the old placeholder nudged readers toward the exact per-node config this PR warns against9 files, +50/−4, reaching 8 rendered pages.
For reviewers — two things to know
1. Scope is much wider than the three pages on the ticket, because this content is duplicated. Two separate copy-paste clusters, neither using the
embeds/mechanism:/integrate/prometheus-with-redis-enterprise/,/operate/rs/monitoring/metrics_stream_engine/, and/operate/rs/references/metrics/. All 43 lines, same title and content, and already drifted: pairwise they differ by 2, 3, and 10 lines. Nobody diverged them on purpose; someone edited one copy and not the others.rs-prometheus-grafana-quickstart.mdembed and a hand-maintained copy inmonitoring/get-started.md.Fixing only the pages named on the ticket would have left the same wrong guidance live on three others. The second commit here adds the third v1-to-v2 copy, which my first pass missed.
2. This intentionally reaches the frozen 7.8 and 7.22 pages. Two of the edited files are shared embeds (
rs-prometheus-grafana-quickstart.md,rs-observability.md) that those versioned pages consume. Normally we don't propagate current content into frozen snapshots, but this is a factual correction and I verified it holds for both versions (see below). Say the word if you'd rather I fork the embed instead.@markotrapani — the claims most worth your eye, since you reproduced this on a lab cluster: that the
/v2/nodelabel caveat is stated correctly, and thatnode_metrics_upis the right metric to point people at.Verification
Checked against the product source at tags
v8.2.1-2,v7.22.2-174, andv7.8.6-296— all three agree:master_aggregatorisautostart = trueinsupervisord_metrics_exporter.confwith no master-only guard, so it runs on every nodelistener-specs.goroutes/v2→ localmaster_aggregatorunconditionally; theIsCurrNodeMaster()branch above it only changes the listener name and addsConnection: closeHandleMetricsRedirectsfeeds only the legacy/route, confirming redirects are not involved on v2node_metrics_upexists as far back as 7.8, so the verification tip is safe in shared contentTwo small corrections to the ticket, applied here: the sub-endpoint list was missing
/v2/db_controller, andnode_metrics_upis a stronger check thancount(up{...})alone.Hugo builds clean. Vale warnings are at the pre-existing baseline; the only remaining new errors are
Grafanaspellings, which this vocabulary already flags 30+ times in these same files.Follow-up worth filing
Both duplication clusters above should collapse into shared embeds — one canonical v1-to-v2 page, one canonical setup-steps page. That drift is the root cause this ticket surfaced, it already caused a missed page inside this PR, and it will recur.
🤖 Generated with Claude Code