Skip to content

feat(metrics-endpoint): count /metrics scrape failures by outcome#3686

Merged
hatamzad-nv merged 3 commits into
NVIDIA:mainfrom
hatamzad-nv:fix/3546-metrics-scrape-failure-counter
Jul 22, 2026
Merged

feat(metrics-endpoint): count /metrics scrape failures by outcome#3686
hatamzad-nv merged 3 commits into
NVIDIA:mainfrom
hatamzad-nv:fix/3546-metrics-scrape-failure-counter

Conversation

@hatamzad-nv

@hatamzad-nv hatamzad-nv commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Closes #3546. Adds a carbide_metrics_scrape_failures_total counter labeled by a bounded ScrapeOutcome enum, emitted at each of the five /metrics failure branches alongside the existing logs. Includes a catalogue row in docs/observability/core_metrics.md and a unit test.

@hatamzad-nv
hatamzad-nv requested a review from a team as a code owner July 17, 2026 19:19
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The metrics endpoint now emits carbide_metrics_scrape_failures_total, labeled by scrape failure outcome. Failure branches report outcomes, tests verify counter deltas, and the core metrics documentation lists the new counter.

Changes

Metrics scrape observability

Layer / File(s) Summary
Failure metric contract and instrumentation wiring
crates/metrics-endpoint/Cargo.toml, crates/metrics-endpoint/src/lib.rs
Adds carbide-instrument dependencies and defines the labeled MetricsScrapeFailed event.
Failure-path event emission
crates/metrics-endpoint/src/lib.rs
Records queue, encoder, encoding, panic, and dropped-reply failures before returning HTTP errors.
Metric validation and documentation
crates/metrics-endpoint/src/lib.rs, docs/observability/core_metrics.md
Tests outcome-specific counter deltas and documents carbide_metrics_scrape_failures_total.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ScrapeHandler as Metrics scrape handler
  participant FailureEvent as MetricsScrapeFailed
  participant Counter as Failure counter
  ScrapeHandler->>FailureEvent: emit labeled failure outcome
  FailureEvent->>Counter: increment outcome counter
  ScrapeHandler-->>ScrapeHandler: return HTTP error response
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: counting /metrics scrape failures by outcome.
Description check ✅ Passed The description matches the implemented changes and references the new counter, docs row, and test.
Linked Issues check ✅ Passed The PR implements the requested failure counter for all five /metrics branches with bounded outcomes and tests.
Out of Scope Changes check ✅ Passed The changes are focused on the metrics-endpoint observability feature, docs, and supporting test/dependency updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 591-628: Update the test scrape_failures_count_per_outcome to
invoke handle_metrics_request for each failure scenario instead of emitting
MetricsScrapeFailed directly. Use a table-driven set of cases that exercises all
five handler failure outcomes, then assert carbide_metrics_scrape_failures_total
deltas for every outcome label, including zero where appropriate.
🪄 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: 03dceb13-5362-4cd2-b120-b965825b69a2

📥 Commits

Reviewing files that changed from the base of the PR and between 0dae3ca and b95f711.

📒 Files selected for processing (3)
  • crates/metrics-endpoint/Cargo.toml
  • crates/metrics-endpoint/src/lib.rs
  • docs/observability/core_metrics.md

Comment thread crates/metrics-endpoint/src/lib.rs Outdated
@hatamzad-nv
hatamzad-nv force-pushed the fix/3546-metrics-scrape-failure-counter branch 2 times, most recently from ff978be to 5ae90e8 Compare July 17, 2026 19:40
@hatamzad-nv

Copy link
Copy Markdown
Contributor Author

Good call — reworked the tests to drive the handler rather than emit directly. test_handle_metrics_request_failure_outcomes is now table-driven and asserts both the status and the per-outcome counter delta for encoder_busy, encoder_gone, and reply_dropped through handle_metrics_request; test_encoder_survives_collector_panic now also asserts the encoder_panicked counter. The encode_failed branch (a TextEncoder error) can't be triggered deterministically — the text encoder doesn't error on a valid registry and there's no seam to inject a failure — so it keeps a documented direct-emit check. That covers 4/5 outcomes through the real handler.

@hatamzad-nv
hatamzad-nv force-pushed the fix/3546-metrics-scrape-failure-counter branch from 5ae90e8 to 6348f12 Compare July 17, 2026 19:59
@hatamzad-nv
hatamzad-nv requested a review from chet July 17, 2026 21:45
Comment thread crates/metrics-endpoint/src/lib.rs Outdated
metric_name = "carbide_metrics_scrape_failures_total",
component = "metrics-endpoint",
metric = counter,
log = off,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replace log = off with:

    log         = dynamic,
    message     = dynamic,

#[label]
outcome: ScrapeOutcome,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And then add this here:

impl DynamicLog for MetricsScrapeFailed {
    fn log_at(&self) -> LogAt {
        match self.outcome {
            ScrapeOutcome::EncoderBusy => LogAt::Level(Level::WARN),
            _ => LogAt::Level(Level::ERROR),
        }
    }
}

impl DynamicMessage for MetricsScrapeFailed {
    fn message(&self) -> &'static str {
        match self.outcome {
            ScrapeOutcome::EncoderBusy     => "metrics encoder busy; shedding scrape with 503",
            ScrapeOutcome::EncoderGone     => "metrics encoder thread is gone; cannot encode metrics",
            ScrapeOutcome::EncodeFailed    => "failed to encode metrics",
            ScrapeOutcome::EncoderPanicked => "metrics encoder caught a panic while encoding",
            ScrapeOutcome::ReplyDropped    => "metrics encoder dropped the reply without responding",
        }
    }
}

Comment thread crates/metrics-endpoint/src/lib.rs Outdated
@@ -448,13 +483,19 @@ async fn handle_metrics_request<B>(
// The single encoder is already busy with a backlog; shed this
// scrape rather than pile on more work.
tracing::warn!("metrics encoder busy; shedding scrape with 503");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...and then you can drop all of the tracing::warn and tracing::error lines (emit will now take care of it for you)

@chet

chet commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

This is awesome @hatamzad-nv ! Thanks so much for doing this. One adjustment you can make, and otherwise looks great. Thanks!

The shared metrics-endpoint handler can fail a scrape five distinct
ways (encoder queue full -> 503; encoder gone, encode error, encoder
panic, dropped reply -> 500). Each was only logged.

Add a carbide_metrics_scrape_failures_total counter, labeled by a
bounded ScrapeOutcome enum, emitted at each failure branch alongside
the existing logs. This meta-observability surfaces a busy or erroring
encoder on the next successful scrape, where aggregatable/alertable
counters beat logs alone.

Closes NVIDIA#3546
@hatamzad-nv
hatamzad-nv force-pushed the fix/3546-metrics-scrape-failure-counter branch from 6348f12 to 2f080df Compare July 21, 2026 21:08
@hatamzad-nv
hatamzad-nv force-pushed the fix/3546-metrics-scrape-failure-counter branch from 90cba68 to 22f2a84 Compare July 21, 2026 21:50

@chet chet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @hatamzad-nv!

@hatamzad-nv
hatamzad-nv merged commit b8c30d3 into NVIDIA:main Jul 22, 2026
62 checks passed
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.

metrics-endpoint: count /metrics scrape failures by outcome

2 participants