feat: add alternate-prefix mirroring and external cancellation to the metrics endpoint#3474
feat: add alternate-prefix mirroring and external cancellation to the metrics endpoint#3474chet wants to merge 1 commit into
Conversation
… metrics endpoint The shared `metrics-endpoint` crate serves a caller-supplied `prometheus::Registry`, but two capabilities still lived only in per-binary copies of the server: mirroring a metric family under a second name prefix, and shutting the listener down on an external cancellation token. Folding them into the shared crate lets every binary serve `/metrics` through one implementation. - `MetricsEndpointConfig` gains an `additional_prefix: Option<(String, String)>`. When set, the exposition re-emits each family whose name starts with the first prefix under the second, so a scrape sees both the original and the aliased series. `None` -- every current caller -- is byte-for-byte the previous output. - `run_metrics_endpoint_with_cancellation` binds the address and serves until a caller-owned `CancellationToken` fires; `run_metrics_endpoint` now delegates to it with a fresh, never-cancelled token, so its behavior is unchanged. - The five existing callers (bmc-proxy, dhcp-server, dhcp, dns, dsx-exchange-consumer) pass `additional_prefix: None`, keeping their `/metrics` identical. Tests cover all three paths: `None` matches a raw text-encode of the registry, `Some` puts each matching family under both prefixes and leaves non-matching families alone, and the cancellation entry point returns once its token fires. This supports NVIDIA#3180 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe metrics endpoint gains optional alternate-prefix exposition for matching Prometheus metric families. Existing endpoint configurations explicitly disable this behavior with ChangesMetrics endpoint prefix exposition
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MetricsHandler
participant PrometheusRegistry
participant TextEncoder
MetricsHandler->>PrometheusRegistry: Gather metric families
MetricsHandler->>MetricsHandler: Duplicate and rename matching families
MetricsHandler->>TextEncoder: Encode combined families
TextEncoder-->>MetricsHandler: Return metrics response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/metrics-endpoint/src/lib.rs (1)
90-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named struct instead of an anonymous
(String, String)tuple.
additional_prefix: Option<(String, String)>requires every caller to remember which element is "old" and which is "new" purely by positional convention; a transposed tuple compiles fine and silently corrupts exposition. A smallstruct PrefixMigration { old: String, new: String }(or similarly named) removes the ambiguity and is self-documenting at call sites.As per coding guidelines, "Prefer struct literals for plain data; use new() for private or internally initialized fields, Default for all-default common cases, and builders only for genuinely large or difficult constructors." This field is brand-new in this PR (no existing callers set a real value), so the API shape is cheap to adjust now.
♻️ Suggested struct-based alternative
+/// Identifies an old/new prefix pair used to duplicate matching metric +/// families under an alternate name during a rename migration. +pub struct PrefixMigration { + pub old: String, + pub new: String, +} pub struct MetricsEndpointConfig { pub address: SocketAddr, pub registry: prometheus::Registry, pub health_controller: Option<HealthController>, - pub additional_prefix: Option<(String, String)>, + pub additional_prefix: Option<PrefixMigration>, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/metrics-endpoint/src/lib.rs` around lines 90 - 104, Replace the anonymous tuple type used by MetricsEndpointConfig.additional_prefix with a named public PrefixMigration struct containing clearly named old and new String fields, and update the exposition logic and all construction/destructuring sites to use those fields instead of positional tuple elements.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 238-273: Update encode_metrics to detect when a prefixed family’s
replacement name already exists among the gathered metric families or generated
aliases, and skip or reject that alias before encoder.encode. Preserve the
original families and ensure the output never contains duplicate metric-family
names or conflicting HELP/TYPE blocks.
---
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 90-104: Replace the anonymous tuple type used by
MetricsEndpointConfig.additional_prefix with a named public PrefixMigration
struct containing clearly named old and new String fields, and update the
exposition logic and all construction/destructuring sites to use those fields
instead of positional tuple elements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df89772e-e580-4800-8273-c4603f7bef89
📒 Files selected for processing (6)
crates/bmc-proxy/src/metrics.rscrates/dhcp-server/src/main.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/lib.rscrates/metrics-endpoint/src/lib.rs
| /// Encode the registry's metric families in the Prometheus text exposition format. | ||
| /// | ||
| /// When `additional_prefix` is `Some((old, new))`, every gathered family whose name | ||
| /// starts with `old` is additionally emitted under a copy whose name has that prefix | ||
| /// replaced by `new`, so the same series appear under both names. When `None`, the | ||
| /// output is exactly `TextEncoder` over `registry.gather()`. | ||
| fn encode_metrics( | ||
| registry: &prometheus::Registry, | ||
| additional_prefix: Option<&(String, String)>, | ||
| ) -> Vec<u8> { | ||
| let mut buffer = vec![]; | ||
| let encoder = TextEncoder::new(); | ||
| let mut metric_families = registry.gather(); | ||
|
|
||
| if let Some((old_prefix, new_prefix)) = additional_prefix { | ||
| let alt_name_families: Vec<MetricFamily> = metric_families | ||
| .iter() | ||
| .filter_map(|family| { | ||
| if !family.name().starts_with(old_prefix) { | ||
| return None; | ||
| } | ||
|
|
||
| let mut alt_name_family = family.clone(); | ||
| alt_name_family.set_name(family.name().replacen(old_prefix, new_prefix, 1)); | ||
| Some(alt_name_family) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if !alt_name_families.is_empty() { | ||
| metric_families.extend(alt_name_families); | ||
| } | ||
| } | ||
|
|
||
| encoder.encode(&metric_families, &mut buffer).unwrap(); | ||
| buffer | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced lines
sed -n '200,320p' crates/metrics-endpoint/src/lib.rs | cat -n
# Find the configuration and all call sites for additional_prefix / encode_metrics
rg -n "additional_prefix|encode_metrics|MetricsEndpointConfig" crates/metrics-endpoint -S
# Show the surrounding definitions if they exist in the same file
rg -n "struct MetricsEndpointConfig|enum .*MetricsEndpoint|additional_prefix" crates/metrics-endpoint/src/lib.rs -n -A 8 -B 8Repository: NVIDIA/infra-controller
Length of output: 13800
Guard renamed metric-family collisions. A (old_prefix, new_prefix) pair can rename a family onto an existing name, producing duplicate HELP/TYPE blocks and invalid Prometheus text. Reject or skip collisions before encoding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/metrics-endpoint/src/lib.rs` around lines 238 - 273, Update
encode_metrics to detect when a prefixed family’s replacement name already
exists among the gathered metric families or generated aliases, and skip or
reject that alias before encoder.encode. Preserve the original families and
ensure the output never contains duplicate metric-family names or conflicting
HELP/TYPE blocks.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/metrics-endpoint/src/lib.rs (1)
244-273: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing guard against renamed-family collisions (unresolved from prior review).
encode_metricsrenames and appends matching families without checking whether the resulting name already exists among the original families or other renamed families. Ifnew_prefixmaps a family onto an existing name (e.g.old_prefix = "carbide_",new_prefix = ""colliding with an existing unprefixed metric, orold_prefix == new_prefix),metric_familieswill contain two entries with the same name, producing duplicateHELP/TYPEblocks in the Prometheus text output — invalid exposition that scrapers may reject.This exact concern was raised on a previous revision of this function and remains unaddressed.
🐛 Suggested fix
if let Some((old_prefix, new_prefix)) = additional_prefix { + let existing_names: std::collections::HashSet<&str> = + metric_families.iter().map(|f| f.name()).collect(); let alt_name_families: Vec<MetricFamily> = metric_families .iter() .filter_map(|family| { if !family.name().starts_with(old_prefix) { return None; } - let mut alt_name_family = family.clone(); - alt_name_family.set_name(family.name().replacen(old_prefix, new_prefix, 1)); - Some(alt_name_family) + let alt_name = family.name().replacen(old_prefix, new_prefix, 1); + if existing_names.contains(alt_name.as_str()) { + tracing::warn!(alt_name, "skipping alternate-prefix metric due to name collision"); + return None; + } + let mut alt_name_family = family.clone(); + alt_name_family.set_name(alt_name); + Some(alt_name_family) }) .collect();Add a regression test exercising a collision case (e.g.
old_prefix/new_prefixthat produces a name already present in the registry) alongside the existing duplication test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/metrics-endpoint/src/lib.rs` around lines 244 - 273, Update encode_metrics to track metric-family names and only append a renamed family when its resulting name does not already exist among the original or previously generated families, including when old_prefix equals new_prefix. Add a regression test alongside the existing duplication test that registers a colliding metric name and verifies the encoded output contains no duplicate family blocks.
🧹 Nitpick comments (2)
crates/metrics-endpoint/src/lib.rs (2)
418-535: 📐 Maintainability & Code Quality | 🔵 TrivialLGTM!
Coverage nicely locks in the byte-identical
Nonepath and the duplication semantics. As per coding guidelines, table-driven tests via carbide-test-support'sscenarios!/value_scenarios!are preferred for input→output mapping functions likeencode_metrics; given the small, distinctly-shaped assertions here, this is optional rather than a blocker.
[optional_low_priority]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/metrics-endpoint/src/lib.rs` around lines 418 - 535, The tests for encode_metrics use separate cases instead of the preferred table-driven carbide-test-support scenarios. Optionally consolidate the None and additional_prefix input/output coverage around encode_metrics using scenarios! or value_scenarios!, preserving the existing byte-identical and duplication assertions.Source: Coding guidelines
94-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named struct instead of
(String, String)for the prefix pair.A raw tuple makes it easy to accidentally swap
old/newat call sites, and the doc comment has to spell out "first tuple element" / "second element" to compensate. As per coding guidelines, Rust designs should be hard to misuse and let the compiler catch mistakes.♻️ Suggested refactor
+pub struct MetricPrefixRename { + pub old: String, + pub new: String, +} + pub struct MetricsEndpointConfig { pub address: SocketAddr, pub registry: prometheus::Registry, pub health_controller: Option<HealthController>, - pub additional_prefix: Option<(String, String)>, + pub additional_prefix: Option<MetricPrefixRename>, }Based on learnings, this follows the "**/*.rs" guideline: "Prefer designs that are hard to misuse and allow the compiler to catch bugs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/metrics-endpoint/src/lib.rs` around lines 94 - 104, Replace MetricsEndpointConfig.additional_prefix’s raw (String, String) tuple with a named struct whose fields explicitly distinguish the old and new prefixes, and update the associated documentation to reference those field names rather than tuple positions. Propagate the named type through all construction and consumption sites while preserving the existing optional behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 244-273: Update encode_metrics to track metric-family names and
only append a renamed family when its resulting name does not already exist
among the original or previously generated families, including when old_prefix
equals new_prefix. Add a regression test alongside the existing duplication test
that registers a colliding metric name and verifies the encoded output contains
no duplicate family blocks.
---
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 418-535: The tests for encode_metrics use separate cases instead
of the preferred table-driven carbide-test-support scenarios. Optionally
consolidate the None and additional_prefix input/output coverage around
encode_metrics using scenarios! or value_scenarios!, preserving the existing
byte-identical and duplication assertions.
- Around line 94-104: Replace MetricsEndpointConfig.additional_prefix’s raw
(String, String) tuple with a named struct whose fields explicitly distinguish
the old and new prefixes, and update the associated documentation to reference
those field names rather than tuple positions. Propagate the named type through
all construction and consumption sites while preserving the existing optional
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bdbe602e-ce2c-4d0b-87ac-4176a41555cb
📒 Files selected for processing (6)
crates/bmc-proxy/src/metrics.rscrates/dhcp-server/src/main.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/lib.rscrates/metrics-endpoint/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
The shared
metrics-endpointcrate serves a caller-supplied registry, but two capabilities still lived only in per-binary copies of the server: mirroring a metric family under a second name prefix, and shutting the listener down on an external cancellation token. Folding them into the shared crate lets every binary serve/metricsthrough one implementation.MetricsEndpointConfiggainsadditional_prefix: Option<(String, String)>-- when set, the exposition re-emits each family whose name starts with the first prefix under the second, so both series appear.None(every current caller) is byte-for-byte the previous output.run_metrics_endpoint_with_cancellationserves until a caller-ownedCancellationTokenfires;run_metrics_endpointdelegates to it with a fresh, never-cancelled token, so it's unchanged.additional_prefix: None, keeping their/metricsidentical.Tests cover all three:
Nonematches a raw text-encode of the registry,Someputs each matching family under both prefixes and leaves non-matching ones alone, and the cancellation entry point returns once its token fires.This supports #3180