Repository navigation
telemetry: unify native Prometheus & DOCA exporter metric output (parity) - #1894
Merged
Merged
Conversation
Both exporters emitted different series for the same events because the counter/gauge policy (type, _total suffix, gauge series name) was written twice and had drifted: native initializeMetrics()/registerCounter()/registerGauge() vs DOCA isCounterEvent()/gaugeMetricName(). Replace the duplication with one shared constexpr descriptor (telemetryMetricDescriptor + telemetry_metric_event_types in telemetry_event.h) that both exporters consume, so parity is structural. DOCA counters gain the OpenMetrics _total suffix and the previously-missing memory and xfer/post-time counters; the native exporter gains the xfer/post-time last-op gauges. The switch has no default, so -Wswitch makes a future missing metric a compile error. The DOCA exporter is unreleased (no scrape consumers), so the _total rename is not a compatibility break. Exporter-side only: no event-type or buffer-format change and no TELEMETRY_VERSION bump. Tests: a descriptor contract test, and a cross-exporter compliance test on each side that scrapes the live endpoint and asserts its agent_* series set equals the shared descriptor oracle -- both using the same OpenMetrics parser. Update docs/telemetry.md and both plugin READMEs to identical metric tables. NIX-1598 Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
👋 Hi e-eygin! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
e-eygin
force-pushed
the
nix-1598-exporter-metric-parity
branch
from
July 6, 2026 17:16
6ec742f to
7a98f01
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a shared telemetry metric descriptor contract, switches both telemetry exporters to derive metric names from it, updates documentation to match the new series naming, and adds contract and scrape-level tests for the exported metrics. ChangesTelemetry metric descriptor unification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/telemetry/doca/doca_exporter.cpp (1)
280-297: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not short-circuit the counter on gauge failure. A transient gauge write error currently returns before the
_totalincrement for events that define both metrics, which can undercount the primary series. Attempt both writes and return failure if either one fails.🤖 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/plugins/telemetry/doca/doca_exporter.cpp` around lines 280 - 297, The telemetry export logic in doca_exporter.cpp short-circuits on a gauge append failure inside the event handling path, which prevents the paired counter from being updated for metrics that define both series. Update the code around telemetryMetricDescriptor, appendGaugeSample, and appendCounterSample so both writes are attempted independently, record whether either call fails, and only return an error after both attempts have run.
🤖 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 `@docs/telemetry.md`:
- Around line 62-69: The telemetry documentation paragraph is missing the
transfer-time gauges from the shared descriptor contract. Update the text around
the Prometheus/DOCA exporter description to also mention `agent_xfer_time` and
`agent_xfer_post_time` as gauges emitted by both exporters, alongside the
existing byte and memory metrics. Keep the wording consistent with the current
explanation that all series come from
`nixlEnumStrings::telemetryMetricDescriptor` in `telemetry_event.h` and that
this is an exporter-side derivation only.
In `@src/core/telemetry/telemetry_event.h`:
- Around line 215-224: The HELP text for the AGENT_XFER_TIME and
AGENT_XFER_POST_TIME cases in the telemetry_event descriptor is inconsistent
with the other metric descriptions and reads like a label instead of a
cumulative metric description. Update the counterHelp strings returned from the
nixl_telemetry_event_type_t::AGENT_XFER_TIME and
nixl_telemetry_event_type_t::AGENT_XFER_POST_TIME branches in telemetry_event.h
to use complete, user-facing sentences that describe the aggregated timing
metric in the same style as the other descriptor help text.
In `@src/plugins/telemetry/doca/README.md`:
- Around line 78-80: The telemetry README prose is missing the newly documented
xfer-time gauges, so update the explanatory text around the Counter/Gauge
descriptions to mention `agent_xfer_time` and `agent_xfer_post_time` as gauge
series. Keep the wording short and consistent with the existing
`agent_<subject>_last_<unit>` convention by adding one sentence near the byte
and memory gauge description so the text matches the table.
---
Outside diff comments:
In `@src/plugins/telemetry/doca/doca_exporter.cpp`:
- Around line 280-297: The telemetry export logic in doca_exporter.cpp
short-circuits on a gauge append failure inside the event handling path, which
prevents the paired counter from being updated for metrics that define both
series. Update the code around telemetryMetricDescriptor, appendGaugeSample, and
appendCounterSample so both writes are attempted independently, record whether
either call fails, and only return an error after both attempts have run.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 99794e93-84df-4489-b925-737cda42108b
📒 Files selected for processing (12)
docs/telemetry.mdsrc/core/telemetry/telemetry_event.hsrc/plugins/telemetry/doca/README.mdsrc/plugins/telemetry/doca/doca_exporter.cppsrc/plugins/telemetry/doca/doca_exporter.hsrc/plugins/telemetry/prometheus/README.mdsrc/plugins/telemetry/prometheus/prometheus_exporter.cppsrc/plugins/telemetry/prometheus/prometheus_exporter.htest/doca-telemetry/telemetry_doca_nixl_test.cpptest/gtest/meson.buildtest/gtest/telemetry_prometheus_test.cpptest/gtest/telemetry_test.cpp
Address CodeRabbit review on ai-dynamo#1894: - telemetry_event.h: make agent_xfer_time_total / agent_xfer_post_time_total HELP text full cumulative-value sentences, consistent with the other counters. - docs/telemetry.md + both plugin READMEs: mention the agent_xfer_time / agent_xfer_post_time gauges alongside the byte and memory pairs so the prose matches the shared-descriptor contract. Docs/HELP text only; no series names, types, or behavior change. NIX-1598 Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/telemetry/telemetry_event.h (1)
182-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public helper.
telemetryMetricDescriptoris exposed from a public header but has no Doxygen block. As per path instructions,src/**/*.hshould use Doxygen-style block comments for public APIs/types/functions.🤖 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/core/telemetry/telemetry_event.h` around lines 182 - 240, Add a Doxygen-style block comment for the public helper telemetryMetricDescriptor in telemetry_event.h, since it is exposed from a public header and must be documented under the src/**/*.h convention. Describe what the function returns for a nixl_telemetry_event_type_t input, including the metric name/help strings and the null result for error/unsupported event types, and place the comment directly above telemetryMetricDescriptor so the API is documented alongside the declaration.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/core/telemetry/telemetry_event.h`:
- Around line 182-240: Add a Doxygen-style block comment for the public helper
telemetryMetricDescriptor in telemetry_event.h, since it is exposed from a
public header and must be documented under the src/**/*.h convention. Describe
what the function returns for a nixl_telemetry_event_type_t input, including the
metric name/help strings and the null result for error/unsupported event types,
and place the comment directly above telemetryMetricDescriptor so the API is
documented alongside the declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d4459705-f2b3-42de-ab2a-d8e882ab9856
📒 Files selected for processing (4)
docs/telemetry.mdsrc/core/telemetry/telemetry_event.hsrc/plugins/telemetry/doca/README.mdsrc/plugins/telemetry/prometheus/README.md
…tor (review) Address CodeRabbit outside-diff-range comments on ai-dynamo#1894: - doca_exporter.cpp: exportEvent attempted the gauge then returned on its failure before the counter increment, which could undercount the cumulative _total series for events that emit both. Attempt gauge and counter independently and return failure only after both have run. - telemetry_event.h: add a Doxygen block for the public telemetryMetricDescriptor helper, per the src/**/*.h documentation convention. Behavior on the success path is unchanged; DOCA runtime tests + native contract/compliance tests pass. NIX-1598 Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
Contributor
Author
Contributor
Author
|
/build |
Contributor
Author
|
/ok to test 094403d |
ColinNV
approved these changes
Jul 7, 2026
e-eygin
added a commit
to e-eygin/nixl
that referenced
this pull request
Jul 7, 2026
…-454) Make silent producer-side telemetry loss observable. When updateData or the all-or-none 4-event addXferStats batch cannot be enqueued because the staging queue is full, the drop is counted in an atomic droppedEvents_; each flush takes-and-resets it (exchange) and, when positive, emits a synthetic AGENT_TELEMETRY_EVENTS_DROPPED event. It bypasses the staging queue (so it can never itself be staging-dropped) and flows through the shared metric descriptor to both exporters as agent_telemetry_events_dropped_total, plus the raw BUFFER stream. Counts only producer-side staging-queue loss (events that never reach an exporter); it does not count BUFFER cyclic-ring loss from a slow downstream reader, which is a separate, uncounted condition. Adding a new event type is a stream schema change, so TELEMETRY_VERSION is bumped 3 -> 4 and examples/python/telemetry_reader.py updated to match. Rebased onto the unified exporter descriptor contract (ai-dynamo#1894): the counter is registered purely via telemetry_metric_event_types + telemetryMetricDescriptor, so no exporter-specific code is needed. Docs and tests updated; the DOCA descriptor-parity test now also validates the new series. Prerequisite for NIX-1205 (extended/stress telemetry tests), whose overflow conservation asserts produced == exported + dropped.
e-eygin
added a commit
to e-eygin/nixl
that referenced
this pull request
Jul 7, 2026
The telemetryMetricContract test (added in ai-dynamo#1894) asserts its expected-series table covers every entry in telemetry_metric_event_types. Add the AGENT_TELEMETRY_EVENTS_DROPPED row (agent_telemetry_events_dropped_total, counter-only) so the sizes match after this PR extends the array.
e-eygin
added a commit
that referenced
this pull request
Jul 7, 2026
…-454) (#1887) ## Summary Makes silent producer-side telemetry loss observable via a new cumulative `agent_telemetry_events_dropped` counter on both exporters and in the raw BUFFER stream. Prerequisite for NIX-1205. - **Core**: new `AGENT_TELEMETRY_EVENTS_DROPPED` event type (`= 20`). `updateData` and the all-or-none `addXferStats` batch now count drops when the staging queue is full (`droppedEvents_`). On each flush, `exportDroppedEvents()` emits the delta since the last flush as a synthetic event — it bypasses the staging queue (so it can never itself be staging-dropped) and is emitted only when the delta is positive, so no-overflow paths keep their exact event counts. - **Exporters**: registered through the shared metric descriptor (from #1894), so both Prometheus and DOCA serve `agent_telemetry_events_dropped_total` — our own counter through the standard counter path, no DOCA-native "dropped" feature involved. - **Scope**: counts only staging-queue loss — events that never reach an exporter. It does **not** count BUFFER cyclic-ring loss from a slow downstream reader (a separate, uncounted condition). - **Compatibility**: new event type is a stream schema change, so `TELEMETRY_VERSION` is bumped **3 → 4** and `examples/python/telemetry_reader.py` updated to match. - **Docs**: `docs/telemetry.md` (new metric + version note), Prometheus/DOCA/plugin READMEs. Design/plan: `docs/proposals/metrics-loss-detection-plan.md`. ## Tests Deterministic and hardware-independent (no concurrent-reader ring races): - **Core** (`telemetry_test.cpp`): `DroppedEventsAppearInBufferStream` stages `capacity-2` accepted events + forces `addXferStats` to reject whole batches, so the single flush pushes exactly `capacity-1` into the ring — provably lossless, single-threaded. Verifies the synthetic reaches the BUFFER stream with the exact drop count and `produced == exported + dropped`. - **Prometheus** (`telemetry_prometheus_test.cpp`): drives `nixlTelemetry` end-to-end through the (lossless, ring-free) Prometheus exporter for both `updateData` and `addXferStats` overflow. Conservation `accepted + dropped == produced` holds regardless of flush timing; plus a direct cumulative-counter check. Both tests also assert `dropped > 0`, so a zero-drop or drop-but-don't-count implementation cannot pass. - **DOCA** (`telemetry_doca_nixl_test.cpp`, `TELEMETRY_DOCA`-gated): scrapes `agent_telemetry_events_dropped_total` == sum of emitted deltas. Run via CTest / the gtest + doca_nixl_test binaries: telemetry 26/26, DOCA 7/7, stable across repeated runs. ## Notes - Rebased onto #1894 (unified exporter descriptor contract). - NIX-1205 (extended/stress telemetry tests) is now unblocked. - Separately noticed (not addressed here, out of scope): `nixlTelemetry::buffer_` is never assigned, so the destructor's `if (buffer_) flushPendingEvents()` block is dead code — the only flush path is the periodic task. Worth a follow-up ticket. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new telemetry metric for dropped events, exposed in Prometheus and DOCA exports. * Telemetry now reports producer-side queue drops as a synthetic event during flushes. * **Bug Fixes** * Improved telemetry accounting so dropped events are captured instead of silently lost. * Updated telemetry version handling to require the newer format for readers and exporters. * **Documentation** * Documented the new dropped-events metric and updated telemetry version notes in the telemetry guides. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jul 8, 2026
e-eygin
added a commit
that referenced
this pull request
Jul 14, 2026
## What? Add an IPC->DTS delivery mode to the DOCA/CollectX telemetry exporter. Delivery backends are now selected by `NIXL_TELEMETRY_DOCA_BACKENDS` — a comma-separated **set** (default `scrape`; e.g. `scrape`, `ipc`, `scrape,ipc`), parsed with `absl::StrSplit` + `absl::StripAsciiWhitespace` (the same idiom `nixl::trace` uses for `NIXL_TRACE_BACKENDS`), plus `NIXL_TELEMETRY_DOCA_IPC_SOCKETS_DIR` for the shared socket folder: - `scrape` — opens the local Prometheus HTTP endpoint (existing behavior). - `ipc` — pushes metrics over IPC to the DOCA Telemetry Service (DTS); no HTTP endpoint of its own. If DTS is unreachable the exporter logs a WARN and continues (no crash). - The two compose; unknown tokens warn+ignore; an empty/unknown set falls back to `scrape`. Default is unchanged (`scrape`), so this is backward-compatible. Exporter-side only — no `TELEMETRY_VERSION` bump. Remote-write / OTLP remain DTS-side onward backends (reached via `ipc`), not exporter flags. ## Why? Under multi-process runs (TP/DP/PP/EP) every NIXL process opens its own scrape endpoint and they collide on the shared port, so only one rank exports (NIX-1557). Pushing over IPC to a single per-node DTS aggregates all processes behind one Prometheus endpoint with no per-process listener — the strategic DOCA-path fix. NIX-1590 (per-rank port offset) stays the near-term mitigation; DTS deployment (K8s DaemonSet/sidecar, multi-process aggregation, per-rank identity) is the follow-up NIX-1600. Tracking: NIX-1599. Builds on the unified exporter descriptor from #1894 (NIX-1598). ## How? All wiring is in the `DocaSharedContext` ctor. In `ipc` mode it skips `setenv(PROMETHEUS_ENDPOINT)`, calls `doca_telemetry_exporter_schema_set_ipc_enabled` (+ `set_ipc_sockets_dir` when configured) between `schema_init` and `schema_start`, and after `source_start` calls `check_ipc_status`, logging WARN-and-continue when not `CONNECTED`. Links `absl_strings` into the plugin + test. Adds DOCA-gated unit tests (backend gating, multi-backend, graceful degradation without DTS) and an opt-in `DISABLED_` end-to-end test that pushes over IPC and scrapes a live DTS. Validated end-to-end (client DOCA 3.3 → DTS 3.5): `agent_tx_bytes_total=7000` + `agent_tx_last_bytes=4000` observed at the DTS endpoint. Docs: DOCA plugin README + `docs/telemetry.md`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added experimental DOCA/CollectX telemetry delivery backends with environment-driven selection for `scrape` and/or `ipc`, plus optional IPC socket directory control. * **Bug Fixes** * If IPC can’t reach the telemetry service, the process continues running and metrics exporting via other enabled backends still works. * If no known backend is selected, the exporter defaults to `scrape`; unknown backend tokens now fail fast. * **Documentation** * Updated telemetry documentation and the DOCA exporter README with backend configuration and runtime behavior details. * **Tests** * Added end-to-end coverage for backend selection, IPC-only endpoint behavior, scrape+IPC mixed operation, and a disabled live IPC integration test. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
hxieustc
pushed a commit
to hxieustc/nixl
that referenced
this pull request
Sep 29, 2026
…ity) (ai-dynamo#1894) ## What? Unifies the metric output of the two Prometheus-family telemetry exporters — the native (prometheus-cpp) exporter and the DOCA/CollectX exporter — so both emit **identical** `agent_*` series (same names, types, and coverage). A single shared `constexpr` descriptor (`telemetryMetricDescriptor` + `telemetry_metric_event_types` in `src/core/telemetry/telemetry_event.h`) now drives both exporters, replacing the two hand-written counter/gauge policies that had drifted (`initializeMetrics()`/`registerCounter()`/`registerGauge()` on the native side vs `isCounterEvent()`/`gaugeMetricName()` on the DOCA side). Net changes: - DOCA counters gain the OpenMetrics `_total` suffix and the previously-missing memory and xfer/post-time counters. - The native exporter gains the `agent_xfer_time` / `agent_xfer_post_time` last-op gauges. - `docs/telemetry.md` and both plugin READMEs updated to identical metric tables. Exporter-side only: no event-type or buffer-format change, and **no `TELEMETRY_VERSION` bump**. ## Why? The two exporters diverged for the same events — e.g. native exposed `agent_xfer_time_total` (counter) while DOCA exposed `agent_xfer_time` (gauge); DOCA counters lacked the `_total` suffix; DOCA emitted no memory counters. A user switching telemetry backends got different metric names/types and different dashboards. Consolidating onto one descriptor makes parity structural (a `switch` with no `default`, so `-Wswitch` turns any future missing metric into a compile error). Tracking issue: NIX-1598. Also a prerequisite for the DOCA IPC→DTS work (NIX-1599), which runs the DOCA exporter on a different CollectX backend and therefore inherits its series shape. Not a breaking change: the DOCA exporter has never shipped in a released NIXL version, so there are no DOCA scrape consumers — the `_total` rename just gets the OpenMetrics shape right before first release. ## How? - Add the shared descriptor + metric-event list to `telemetry_event.h`; both exporters and the tests read from it. - Native `initializeMetrics()` iterates the descriptor and stops appending `_total` locally; DOCA `exportEvent()` drives off it, and `isCounterEvent()`/`gaugeMetricName()` are deleted (this also removes DOCA's per-event `std::string` allocation, superseding ai-dynamo#1886). Error counters keep the `status`-labelled path; `agent_errors_total` comes from the shared definition. - Tests (GoogleTest, run via CTest): - `telemetryMetricContract` pins the descriptor's exact name/type contract. - `prometheusTelemetryTest.ScrapeEmitsExactlyTheDescriptorSeries` and `docaNixlExporterTest.ScrapeEmitsExactlyTheDescriptorSeries` each scrape the live endpoint and assert the emitted `agent_*` series set equals the shared descriptor oracle — both using the **same** OpenMetrics parser. (The two exporters can't be co-linked — DOCA gRPC vs nixl-core gRPC — so each is validated in its own binary against the common oracle.) - Existing byte counter/last-gauge and error-status tests updated to `_total`. - `ninja -C build` green; telemetry gtests (43) + DOCA runtime tests (`doca_test`, `doca_nixl_test`, real CollectX) pass; clang-format clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Telemetry metrics are now driven by a unified descriptor contract, aligning Prometheus and DOCA series names and semantics. - Transfer-time and byte metrics now expose both cumulative `_total` counters and corresponding last-value gauges (when applicable). - **Bug Fixes** - Fixed inconsistent counter/gauge support and tx/rx byte-series naming between exporters. - **Documentation** - Updated telemetry documentation and exporter README tables to reflect the unified metric series and naming. - **Tests** - Added contract validation and scrape-coverage tests to confirm exporters emit exactly the expected series. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
hxieustc
pushed a commit
to hxieustc/nixl
that referenced
this pull request
Sep 29, 2026
…-454) (ai-dynamo#1887) ## Summary Makes silent producer-side telemetry loss observable via a new cumulative `agent_telemetry_events_dropped` counter on both exporters and in the raw BUFFER stream. Prerequisite for NIX-1205. - **Core**: new `AGENT_TELEMETRY_EVENTS_DROPPED` event type (`= 20`). `updateData` and the all-or-none `addXferStats` batch now count drops when the staging queue is full (`droppedEvents_`). On each flush, `exportDroppedEvents()` emits the delta since the last flush as a synthetic event — it bypasses the staging queue (so it can never itself be staging-dropped) and is emitted only when the delta is positive, so no-overflow paths keep their exact event counts. - **Exporters**: registered through the shared metric descriptor (from ai-dynamo#1894), so both Prometheus and DOCA serve `agent_telemetry_events_dropped_total` — our own counter through the standard counter path, no DOCA-native "dropped" feature involved. - **Scope**: counts only staging-queue loss — events that never reach an exporter. It does **not** count BUFFER cyclic-ring loss from a slow downstream reader (a separate, uncounted condition). - **Compatibility**: new event type is a stream schema change, so `TELEMETRY_VERSION` is bumped **3 → 4** and `examples/python/telemetry_reader.py` updated to match. - **Docs**: `docs/telemetry.md` (new metric + version note), Prometheus/DOCA/plugin READMEs. Design/plan: `docs/proposals/metrics-loss-detection-plan.md`. ## Tests Deterministic and hardware-independent (no concurrent-reader ring races): - **Core** (`telemetry_test.cpp`): `DroppedEventsAppearInBufferStream` stages `capacity-2` accepted events + forces `addXferStats` to reject whole batches, so the single flush pushes exactly `capacity-1` into the ring — provably lossless, single-threaded. Verifies the synthetic reaches the BUFFER stream with the exact drop count and `produced == exported + dropped`. - **Prometheus** (`telemetry_prometheus_test.cpp`): drives `nixlTelemetry` end-to-end through the (lossless, ring-free) Prometheus exporter for both `updateData` and `addXferStats` overflow. Conservation `accepted + dropped == produced` holds regardless of flush timing; plus a direct cumulative-counter check. Both tests also assert `dropped > 0`, so a zero-drop or drop-but-don't-count implementation cannot pass. - **DOCA** (`telemetry_doca_nixl_test.cpp`, `TELEMETRY_DOCA`-gated): scrapes `agent_telemetry_events_dropped_total` == sum of emitted deltas. Run via CTest / the gtest + doca_nixl_test binaries: telemetry 26/26, DOCA 7/7, stable across repeated runs. ## Notes - Rebased onto ai-dynamo#1894 (unified exporter descriptor contract). - NIX-1205 (extended/stress telemetry tests) is now unblocked. - Separately noticed (not addressed here, out of scope): `nixlTelemetry::buffer_` is never assigned, so the destructor's `if (buffer_) flushPendingEvents()` block is dead code — the only flush path is the periodic task. Worth a follow-up ticket. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new telemetry metric for dropped events, exposed in Prometheus and DOCA exports. * Telemetry now reports producer-side queue drops as a synthetic event during flushes. * **Bug Fixes** * Improved telemetry accounting so dropped events are captured instead of silently lost. * Updated telemetry version handling to require the newer format for readers and exporters. * **Documentation** * Documented the new dropped-events metric and updated telemetry version notes in the telemetry guides. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
hxieustc
pushed a commit
to hxieustc/nixl
that referenced
this pull request
Sep 29, 2026
) ## What? Add an IPC->DTS delivery mode to the DOCA/CollectX telemetry exporter. Delivery backends are now selected by `NIXL_TELEMETRY_DOCA_BACKENDS` — a comma-separated **set** (default `scrape`; e.g. `scrape`, `ipc`, `scrape,ipc`), parsed with `absl::StrSplit` + `absl::StripAsciiWhitespace` (the same idiom `nixl::trace` uses for `NIXL_TRACE_BACKENDS`), plus `NIXL_TELEMETRY_DOCA_IPC_SOCKETS_DIR` for the shared socket folder: - `scrape` — opens the local Prometheus HTTP endpoint (existing behavior). - `ipc` — pushes metrics over IPC to the DOCA Telemetry Service (DTS); no HTTP endpoint of its own. If DTS is unreachable the exporter logs a WARN and continues (no crash). - The two compose; unknown tokens warn+ignore; an empty/unknown set falls back to `scrape`. Default is unchanged (`scrape`), so this is backward-compatible. Exporter-side only — no `TELEMETRY_VERSION` bump. Remote-write / OTLP remain DTS-side onward backends (reached via `ipc`), not exporter flags. ## Why? Under multi-process runs (TP/DP/PP/EP) every NIXL process opens its own scrape endpoint and they collide on the shared port, so only one rank exports (NIX-1557). Pushing over IPC to a single per-node DTS aggregates all processes behind one Prometheus endpoint with no per-process listener — the strategic DOCA-path fix. NIX-1590 (per-rank port offset) stays the near-term mitigation; DTS deployment (K8s DaemonSet/sidecar, multi-process aggregation, per-rank identity) is the follow-up NIX-1600. Tracking: NIX-1599. Builds on the unified exporter descriptor from ai-dynamo#1894 (NIX-1598). ## How? All wiring is in the `DocaSharedContext` ctor. In `ipc` mode it skips `setenv(PROMETHEUS_ENDPOINT)`, calls `doca_telemetry_exporter_schema_set_ipc_enabled` (+ `set_ipc_sockets_dir` when configured) between `schema_init` and `schema_start`, and after `source_start` calls `check_ipc_status`, logging WARN-and-continue when not `CONNECTED`. Links `absl_strings` into the plugin + test. Adds DOCA-gated unit tests (backend gating, multi-backend, graceful degradation without DTS) and an opt-in `DISABLED_` end-to-end test that pushes over IPC and scrapes a live DTS. Validated end-to-end (client DOCA 3.3 → DTS 3.5): `agent_tx_bytes_total=7000` + `agent_tx_last_bytes=4000` observed at the DTS endpoint. Docs: DOCA plugin README + `docs/telemetry.md`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added experimental DOCA/CollectX telemetry delivery backends with environment-driven selection for `scrape` and/or `ipc`, plus optional IPC socket directory control. * **Bug Fixes** * If IPC can’t reach the telemetry service, the process continues running and metrics exporting via other enabled backends still works. * If no known backend is selected, the exporter defaults to `scrape`; unknown backend tokens now fail fast. * **Documentation** * Updated telemetry documentation and the DOCA exporter README with backend configuration and runtime behavior details. * **Tests** * Added end-to-end coverage for backend selection, IPC-only endpoint behavior, scrape+IPC mixed operation, and a disabled live IPC integration test. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
This branch was previously deployed
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.
What?
Unifies the metric output of the two Prometheus-family telemetry exporters — the
native (prometheus-cpp) exporter and the DOCA/CollectX exporter — so both emit
identical
agent_*series (same names, types, and coverage).A single shared
constexprdescriptor (telemetryMetricDescriptor+telemetry_metric_event_typesinsrc/core/telemetry/telemetry_event.h) nowdrives both exporters, replacing the two hand-written counter/gauge policies that
had drifted (
initializeMetrics()/registerCounter()/registerGauge()on thenative side vs
isCounterEvent()/gaugeMetricName()on the DOCA side).Net changes:
_totalsuffix and the previously-missingmemory and xfer/post-time counters.
agent_xfer_time/agent_xfer_post_timelast-op gauges.
docs/telemetry.mdand both plugin READMEs updated to identical metric tables.Exporter-side only: no event-type or buffer-format change, and no
TELEMETRY_VERSIONbump.Why?
The two exporters diverged for the same events — e.g. native exposed
agent_xfer_time_total(counter) while DOCA exposedagent_xfer_time(gauge);DOCA counters lacked the
_totalsuffix; DOCA emitted no memory counters. A userswitching telemetry backends got different metric names/types and different
dashboards. Consolidating onto one descriptor makes parity structural (a
switchwith no
default, so-Wswitchturns any future missing metric into a compileerror).
Tracking issue: NIX-1598. Also a prerequisite for the DOCA IPC→DTS work
(NIX-1599), which runs the DOCA exporter on a different CollectX backend and
therefore inherits its series shape.
Not a breaking change: the DOCA exporter has never shipped in a released NIXL
version, so there are no DOCA scrape consumers — the
_totalrename just gets theOpenMetrics shape right before first release.
How?
telemetry_event.h; bothexporters and the tests read from it.
initializeMetrics()iterates the descriptor and stops appending_totallocally; DOCAexportEvent()drives off it, andisCounterEvent()/gaugeMetricName()are deleted (this also removes DOCA'sper-event
std::stringallocation, superseding telemetry: avoid DOCA counter event name allocation #1886). Error counters keep thestatus-labelled path;agent_errors_totalcomes from the shared definition.telemetryMetricContractpins the descriptor's exact name/type contract.prometheusTelemetryTest.ScrapeEmitsExactlyTheDescriptorSeriesanddocaNixlExporterTest.ScrapeEmitsExactlyTheDescriptorSerieseach scrape thelive endpoint and assert the emitted
agent_*series set equals the shareddescriptor oracle — both using the same OpenMetrics parser. (The two
exporters can't be co-linked — DOCA gRPC vs nixl-core gRPC — so each is
validated in its own binary against the common oracle.)
_total.ninja -C buildgreen; telemetry gtests (43) + DOCA runtime tests(
doca_test,doca_nixl_test, real CollectX) pass; clang-format clean.Summary by CodeRabbit
Summary by CodeRabbit
New Features
_totalcounters and corresponding last-value gauges (when applicable).Bug Fixes
Documentation
Tests