telemetry: materialize dropped-event counter children at 0 - #927
telemetry: materialize dropped-event counter children at 0#927philosaether wants to merge 4 commits into
Conversation
A labeled Prometheus counter exports no series until its first labels() call, so telemetry_events_dropped stayed invisible until an event was actually dropped — impossible to alert on or graph, and "no drops" was indistinguishable from "metric missing / scrape broken". Pre-create the (namespace, reason) children at 0 on emitter start, for each reason the emitter can emit, so the metric is always present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPrometheus metrics now pre-materialize bounded label series during API, deriver, and emitter startup. Reconciliation reports pending message embeddings through a gauge, with tests covering valid domains, process isolation, dropped counters, and disabled metrics. ChangesTelemetry metrics initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIStartup
participant DeriverStartup
participant PrometheusMetrics
participant Reconciler
participant Database
APIStartup->>PrometheusMetrics: initialize API bounded metrics
DeriverStartup->>PrometheusMetrics: initialize deriver bounded metrics
PrometheusMetrics-->>APIStartup: materialized zero-valued series
PrometheusMetrics-->>DeriverStartup: materialized zero-valued series
Reconciler->>Database: count pending MessageEmbedding rows
Database-->>Reconciler: pending count
Reconciler->>PrometheusMetrics: set pending embeddings gauge
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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 `@src/telemetry/prometheus/metrics.py`:
- Around line 341-347: Update initialize_telemetry_dropped_metrics to catch only
the expected Prometheus labels(reason=reason) validation exception, using the
appropriate specific Prometheus exception type. Let unexpected exceptions from
telemetry_events_dropped_counter.labels propagate instead of routing them
through _handle_metric_error, preserving the existing handling for valid label
errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f114054c-c059-4b97-816c-ba9cad4842e6
📒 Files selected for processing (2)
src/telemetry/emitter.pysrc/telemetry/prometheus/metrics.py
Rajat-Ahuja1997
left a comment
Rajat-Ahuja1997
left a comment
There was a problem hiding this comment.
looks good, 1 comment
Extends #927 (which zero-inited telemetry_events_dropped) to every counter whose label domain is bounded and known at startup, so metrics are present in Prometheus before their first event — a missing series then signals a broken scrape rather than "nothing happened yet". - add initialize_bounded_metrics(instance_type) on PrometheusMetrics; call it per-process from main.py (api) and deriver/__main__.py (deriver). - extract a shared _touch() helper; refactor initialize_telemetry_dropped_metrics onto it (that one stays per-emitter in start() — it's prefix-dependent). - explicit ALL_EVENT_TYPES / HIGH_VOLUME_EVENT_TYPES registry in telemetry.events, drift-guarded by tests that walk BaseEvent subclasses. - only VALID (task_type, token_type, component) tuples for deriver_tokens (the cartesian product would fabricate impossible always-0 series); only high-volume event types for sampled_out; high-cardinality labels (endpoint, workspace_name) left open. - gauges: zero-init embed_now_tasks_in_flight + telemetry_buffer_size; add a new message_embeddings_pending backlog gauge, set each reconciliation cycle and zero-inited at deriver startup (Rajat's pending/in-flight ask). - backfills the tests #927 shipped without. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/telemetry/prometheus/metrics.py`:
- Around line 403-458: Replace the direct gauge initialization calls in
initialize_bounded_metrics with the class’s fail-soft _touch path, including
telemetry_buffer_size_gauge, embed_now_tasks_in_flight_gauge, and
message_embeddings_pending_gauge. Preserve initializing each gauge to zero while
ensuring any metric setup failure is handled consistently without interrupting
startup.
In `@tests/telemetry/test_metric_zero_init.py`:
- Around line 227-241: The test test_api_init_does_not_touch_deriver_counters
currently covers only API-to-deriver isolation. Add a before/after assertion
around prometheus_metrics.initialize_bounded_metrics(instance_type="deriver")
using an API-only bounded metric and its appropriate labels, asserting the
samples are unchanged so deriver initialization cannot materialize API metrics.
- Around line 95-102: Update the validation around _DERIVER_TOKEN_COMBOS to
compare the complete set of token/component pairs against the exact expected
pair set, rather than checking token and component membership independently.
Also assert the combo collection length equals the expected pair-set length so
duplicate entries are rejected, while preserving the existing coverage checks as
appropriate.
- Around line 31-43: Use unique namespaces in test_metric_zero_init.py for all
global REGISTRY assertions: replace the fixed NS value used by sample() with a
module/test-specific namespace, and introduce a separate unique namespace for
the disabled-metrics check at lines 258-269. Update the relevant metric
configuration and assertions to use these namespaces so materialized children
from other tests cannot affect results.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 605569e1-376d-4abd-a7a8-df494bba2741
📒 Files selected for processing (6)
src/deriver/__main__.pysrc/main.pysrc/reconciler/sync_vectors.pysrc/telemetry/events/__init__.pysrc/telemetry/prometheus/metrics.pytests/telemetry/test_metric_zero_init.py
I1: _DERIVER_TOKEN_COMBOS was factored task-independently, materializing the impossible (ingestion, input, previous_summary) series — previous_summary is summary-only. Make combos task-aware (_DERIVER_TOKEN_COMBOS_BY_TASK) so no always-0 impossible series is fabricated, matching the PR's own goal. Tests tightened to assert the ingestion/previous_summary series is absent. I2: the three gauge .set(0) zero-inits were bare while the counter inits go through the fail-soft _touch. Add _set_gauge_zero() so a gauge init can't propagate an exception into process startup either. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/telemetry/prometheus/metrics.py (1)
379-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a Google-style docstring.
Document the
gaugeparameter in anArgs:section.Proposed fix
def _set_gauge_zero(self, gauge: NamespacedGauge) -> None: - """Materialize a (namespace-only) gauge at 0. Fail-soft, like ``_touch``: - a startup init must never propagate an exception into process boot.""" + """Materialize a namespace-only gauge at zero without failing startup. + + Args: + gauge: The namespace-aware gauge to materialize. + """🤖 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 `@src/telemetry/prometheus/metrics.py` around lines 379 - 381, Update the _set_gauge_zero docstring to Google style by adding an Args: section that documents the gauge parameter and its NamespacedGauge type, while preserving the existing behavior description.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.
Nitpick comments:
In `@src/telemetry/prometheus/metrics.py`:
- Around line 379-381: Update the _set_gauge_zero docstring to Google style by
adding an Args: section that documents the gauge parameter and its
NamespacedGauge type, while preserving the existing behavior description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70ebefb0-e803-4f21-a26c-4130f2563d10
📒 Files selected for processing (2)
src/telemetry/prometheus/metrics.pytests/telemetry/test_metric_zero_init.py
|
Finn-loop review of 313fd41 CI: not configured ( ReviewSummary: PR materializes Prometheus counter children at 0 on startup so metrics like 1. Must fix before merge
2. Should fix soon
3. Safe to mergeNo — merge is mechanically blocked by BLOCKED status, and required CI is not configured. Code quality is high: fail-soft on init (never crashes startup), drift-guard tests prevent stale registries, cartesian-product explosion is avoided by explicit valid-combo tables, and the 294-line test suite is thorough. Label notesThis reviewer's account does not have admin rights to
|
Global-REGISTRY assertions used a fixed "test" namespace, which several other suites also pin, so another test's materialized children could satisfy a presence assertion or break an absence one. Each test now runs under a unique namespace resolved from settings at read time. Adds the inverse per-process isolation test: deriver-only init must not materialize API-only series (dialectic tokens, embed_now). Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
What
Pre-create the
telemetry_events_droppedcounter's(namespace, reason)child series at0when the telemetry emitter starts, for eachreasonthe emitter can emit ({prefix}buffer_full,{prefix}send_failed).Why
A labeled Prometheus counter exports no time series until its first
labels(...)call. Sotelemetry_events_droppedwas invisible in Prometheus/Grafana until an event was actually dropped — which means:Materializing the children at
0on startup keeps the metric always present, so a missing series now signals a real problem rather than a healthy system.Changes
src/telemetry/prometheus/metrics.py: addinitialize_telemetry_dropped_metrics(reasons=...), which pre-touches each(namespace, reason)child (namespace auto-injected byNamespacedCounter, matching existingrecord_telemetry_event_droppedusage).src/telemetry/emitter.py: call it on emitter start with the emitter's own prefixed reasons.Testing
Verified in a sandbox: the counter's zero-valued children appear on the metrics endpoint before any drop occurs, and increment correctly when drops happen.
🤖 Generated with Claude Code
Summary by CodeRabbit
/metricsvisibility in both API and deriver.