telemetry/doca: share one timestamp per export batch (NIX-1625)#1952
telemetry/doca: share one timestamp per export batch (NIX-1625)#1952e-eygin wants to merge 4 commits into
Conversation
Add a scoped withBatch(body) callback to nixlTelemetryExporter, backed by a private RAII guard driving no-op private virtual onBatchBegin/onBatchEnd hooks (NVI). flushPendingEvents() runs the whole drain (queued events plus the synthetic AGENT_TELEMETRY_EVENTS_DROPPED event) inside one withBatch. The DOCA exporter overrides the hooks to read the DOCA clock once per batch (lazy currentTimestamp with inBatch_ + optional batchTimestamp_) instead of 2-3 reads per event, so every sample in a drain shares one timestamp. The append helpers take an explicit timestamp; histograms are unaffected (they are timestamped at histogram_flush). Standalone exportEvent (no batch) still gets a fresh timestamp per event. No metric names, labels, event types, buffer format, or TELEMETRY_VERSION change; scrape output is unchanged. Adds gtests covering the shared-batch timestamp and the standalone path. 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. 🚀 |
📝 WalkthroughWalkthroughTelemetry exports now support RAII-managed batch scopes. Pending events and dropped-event metrics share a batch, while the DOCA exporter uses one timestamp per batch and fresh timestamps for standalone exports. Tests cover batched, standalone, and nested scopes. ChangesTelemetry batching and timestamp consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Telemetry
participant Exporter
participant DOCA
participant Metrics
Telemetry->>Exporter: withBatch(flush exports)
Exporter->>DOCA: onBatchBegin()
Telemetry->>DOCA: exportEvent(event)
DOCA->>Metrics: write samples with cached timestamp
Telemetry->>DOCA: exportDroppedEvents()
Exporter->>DOCA: onBatchEnd()
Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/cpp/telemetry/telemetry_exporter.h`:
- Line 87: Rename the nested type BatchScope to batchScope and update all
references to this type throughout the surrounding telemetry exporter code,
preserving its existing behavior and API usage.
In `@src/plugins/telemetry/doca/doca_exporter.cpp`:
- Around line 371-379: Update nixlTelemetryDocaExporter::onBatchBegin and
onBatchEnd to track batch nesting depth, initializing inBatch_ and
batchTimestamp_ only on the 0→1 transition and clearing them only on the 1→0
transition; preserve the outer batch timestamp while nested withBatch scopes are
active. Add a regression test covering nested batch scopes and verifying all
outer-scope events share the original timestamp.
🪄 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: d04a9654-32ac-476d-887a-3074dab53610
📒 Files selected for processing (5)
src/api/cpp/telemetry/telemetry_exporter.hsrc/core/telemetry/telemetry.cppsrc/plugins/telemetry/doca/doca_exporter.cppsrc/plugins/telemetry/doca/doca_exporter.htest/doca-telemetry/telemetry_doca_nixl_test.cpp
…afe batches Rename the nested guard BatchScope -> batchScope to follow the repo lowerCamelCase type-naming rule (docs/CodeStyle.md). Make withBatch nesting-safe in the base: track a batch depth and fire onBatchBegin/onBatchEnd only on the outermost 0->1 and 1->0 transitions, so an inner scope's exit no longer resets a still-active outer batch's shared timestamp. Add a DOCA regression test asserting events before, inside, and after a nested scope all carry the one outer-batch timestamp. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/cpp/telemetry/telemetry_exporter.h (1)
87-118: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCoordinate a telemetry ABI bump. Adding
onBatchBegin()/onBatchEnd()andbatchDepth_changesnixlTelemetryExporter’s vtable and object layout, so prebuilt or out-of-tree exporters can break when loaded. Keep the ABI stable, or gate this behind a major-version/rebuild plan.🤖 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/api/cpp/telemetry/telemetry_exporter.h` around lines 87 - 118, Update nixlTelemetryExporter and batchScope so the batching support does not change the existing exporter ABI or object layout. Remove or isolate the virtual onBatchBegin()/onBatchEnd() additions and batchDepth_ storage using an explicitly coordinated major-version/rebuild mechanism, preserving compatibility for prebuilt and out-of-tree exporters.Source: Learnings
🤖 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/api/cpp/telemetry/telemetry_exporter.h`:
- Around line 79-84: Add a Doxygen-style block comment immediately before the
public withBatch template describing that it executes the supplied callback,
supports nested batching scopes, and guarantees the associated lifecycle hooks
are invoked appropriately. Keep the implementation unchanged.
In `@test/doca-telemetry/telemetry_doca_nixl_test.cpp`:
- Around line 645-666: Update the metric validation in the test around the
series iteration and counted assertion to require exactly the three expected
non-histogram series from before, inside, and after nesting. Identify each
expected series by name, verify it is present and has a timestamp, and assert
all three timestamps equal the shared outer batch timestamp instead of relying
on EXPECT_GE(counted, 2u).
---
Outside diff comments:
In `@src/api/cpp/telemetry/telemetry_exporter.h`:
- Around line 87-118: Update nixlTelemetryExporter and batchScope so the
batching support does not change the existing exporter ABI or object layout.
Remove or isolate the virtual onBatchBegin()/onBatchEnd() additions and
batchDepth_ storage using an explicitly coordinated major-version/rebuild
mechanism, preserving compatibility for prebuilt and out-of-tree exporters.
🪄 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: 9f7d4619-13a2-41f4-9dc1-737af810968b
📒 Files selected for processing (2)
src/api/cpp/telemetry/telemetry_exporter.htest/doca-telemetry/telemetry_doca_nixl_test.cpp
Add a Doxygen block to the public withBatch() describing callback execution, exception-safe batch lifecycle, and outermost-only nested-scope hook semantics. Tighten NestedBatchScopesShareOuterTimestamp: assert the three series produced before, inside, and after the nested scope (agent_tx_bytes / agent_rx_bytes / agent_memory_registered_last_bytes) are each present and all share the outer batch timestamp, instead of the loose EXPECT_GE(counted, 2) check. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/cpp/telemetry/telemetry_exporter.h (1)
121-127: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the exporter ABI. These additions change the vtable and base-class layout, so out-of-tree telemetry exporters built against the old header will break. Move the batch state/hooks out of the exported base class, or gate this behind an explicit ABI-major bump and rebuild plan.
🤖 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/api/cpp/telemetry/telemetry_exporter.h` around lines 121 - 127, Preserve the ABI of the telemetry exporter base class by removing onBatchBegin(), onBatchEnd(), and batchDepth_ from the exported class layout, then implement batch tracking through a non-exported mechanism or existing extension point. Do not alter the virtual table or base-class size unless this change is explicitly accompanied by an ABI-major bump and rebuild plan.Source: Learnings
🤖 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/api/cpp/telemetry/telemetry_exporter.h`:
- Around line 79-87: Add parameter-level Doxygen documentation to the withBatch
comment: include `@tparam` Body for the template parameter and `@param` body for the
callback argument. Keep the existing `@brief` and batch-lifecycle description
unchanged, and do not add an `@return` tag because withBatch returns void.
---
Outside diff comments:
In `@src/api/cpp/telemetry/telemetry_exporter.h`:
- Around line 121-127: Preserve the ABI of the telemetry exporter base class by
removing onBatchBegin(), onBatchEnd(), and batchDepth_ from the exported class
layout, then implement batch tracking through a non-exported mechanism or
existing extension point. Do not alter the virtual table or base-class size
unless this change is explicitly accompanied by an ABI-major bump and rebuild
plan.
🪄 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: 0f123ef8-ff36-4dd9-889b-3ce36602f71d
📒 Files selected for processing (2)
src/api/cpp/telemetry/telemetry_exporter.htest/doca-telemetry/telemetry_doca_nixl_test.cpp
Complete the public withBatch() documentation with parameter-level Doxygen tags per the repo public-API convention; no @return since it returns void. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
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/api/cpp/telemetry/telemetry_exporter.h (1)
124-130: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCoordinate the telemetry exporter ABI change.
nixlTelemetryExporteris a plugin-facing base class, so addingonBatchBegin()/onBatchEnd()changes its vtable andbatchDepth_changes its object layout. That breaks binary compatibility for out-of-tree exporters built against the previous header; add a version bump/rebuild plan or avoid new virtual slots.🤖 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/api/cpp/telemetry/telemetry_exporter.h` around lines 124 - 130, The plugin-facing nixlTelemetryExporter ABI is broken by the new virtual onBatchBegin/onBatchEnd slots and batchDepth_ member. Avoid adding virtual methods or instance data to the existing base class, or coordinate an explicit ABI version bump and rebuild plan for all out-of-tree exporters, preserving compatibility with exporters built against the previous header.Source: Learnings
🤖 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/api/cpp/telemetry/telemetry_exporter.h`:
- Around line 124-130: The plugin-facing nixlTelemetryExporter ABI is broken by
the new virtual onBatchBegin/onBatchEnd slots and batchDepth_ member. Avoid
adding virtual methods or instance data to the existing base class, or
coordinate an explicit ABI version bump and rebuild plan for all out-of-tree
exporters, preserving compatibility with exporters built against the previous
header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 373cb95c-b22b-4892-a13f-37fe000b53da
📒 Files selected for processing (1)
src/api/cpp/telemetry/telemetry_exporter.h
|
/build |
|
🤖 CI Triage Agent — TL;DR: Both parallel Docker image builds failed at Full analysisSummary: The "Compiling NIXL Docker Image for DL" stages (node 159 = ucx-master, node 162 = ucx-v1.22.x) both failed at Dockerfile STEP 15/17 during Root cause: apt could not download Implicated commit: none — infrastructure/network failure, not a code change (the failing File: Suggested fix: Re-run the build; the failure should clear once mirror connectivity is restored. To harden against recurrence: (1) point apt at a reliable internal/proxied Ubuntu mirror instead of the public Related: none |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Stage "Run DL EP elastic tests" (#340, node 223) failed on plan Root cause: During the fault-injection phase, rank 2 was intentionally killed ("[rank 2] Killing rank during dispatch/combine" → "SIGTERM (15) received … releasing rank and exiting"). Teardown then hit Implicated commit: unknown for the exact regression, but the fault path is handled by File: Suggested fix: Make the killed-rank teardown crash-safe: in Related: #1500 (planned SIGTERM handling), #1693 (rank/expert refactor), #1584 (preserve CUDA graphs across scale up/down); PR under test #1952. No existing issue found for this illegal-memory-access-on-teardown signature. |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage failed with exit code 42 because CUDA failed to initialize on the test node Full analysisSummary: 69/154 CPP gtests failed on node Root cause: Every test process logs Implicated commit: none — not a code regression from PR #1952 (commit 0ef06af); this is a test-node CUDA environment failure on File: Suggested fix: Repair/reset the CUDA driver environment on test node Related: none (no matching issue for the |
|
🤖 CI Triage Agent — TL;DR: The wheel built fine, but the "Build sanity image" stage failed because Full analysisSummary: Stage 470 "Build sanity image" (aarch64 vLLM) failed during Root cause: During the image build, Implicated commit: unknown — not a code regression. The Dockerfile step originates from File: Suggested fix: Re-run the build first — this is almost certainly a transient egress outage to Related: PR #1777 (added this sanity-image build); PR under test #1952. No matching issue found for the |
|
🤖 CI Triage Agent — TL;DR: The "Run DL EP elastic tests" stage failed because the surviving ranks in the Full analysisSummary: Stage 203 ( Root cause: In the fault-injection phase (plan Implicated commit: Most likely File: Suggested fix: In the LL NVLink dispatch/combine kernel, guard peer-buffer/IPC-endpoint accesses so a rank marked failed/departed (the Related: PR #1751 (LL NVLink path), PR #1500 (planned-SIGTERM handling in elastic test), PR #1846 (forcibly close UCP endpoints — possibly relevant to endpoint teardown on fault).
|
|
🤖 CI Triage Agent — TL;DR: The Jenkins build compiled NIXL and pushed the test image successfully; both failing stages died from cluster infrastructure faults — a compute-node failure on Full analysisSummary: "Run CPP tests" and "Allocate Environment" stages failed due to SLURM/Jenkins-agent infrastructure faults, not test or build errors. Root cause: In "Run CPP tests" (node 220), the docker image import via pyxis was interrupted by Implicated commit: none — not code-related (commit [REDACTED:Hex High Entropy String] is not implicated) File: none (infrastructure: SLURM node Suggested fix: Retry the build; it should pass on a healthy node. Have infra investigate the health of node Related: none
|
|
/ok to test 2cca182 |
|
🤖 CI Triage Agent — TL;DR: The "Run Nixlbench tests" stage was killed at the 15-min mark (exit 143) because the UCCL Full analysisSummary: Jenkins stage 337 "Run Nixlbench tests" (nixl-ci-gpu #2805, v1.22.x pipeline) was aborted with exit code 143 after a ~15-minute silent hang. Root cause: A process hang, not a wall-clock shortfall. The last application output is at Implicated commit: Not definitively identifiable from logs; UCCL backend behavior last substantially changed in File: Suggested fix: Do not raise the stage timeout. Instead: (1) wrap each Related: #1271 (UCCL optimize batch transfers), #1560 (sleep to prevent both nixlbench processes becoming clients), #1800 (export container on test failure for crash debugging).
|
2 participants
What?
Make the DOCA telemetry exporter stamp every sample produced in a single
drain/export cycle with one shared timestamp, instead of reading the DOCA clock
2–3 times per event.
withBatch(body)callback tonixlTelemetryExporter(
src/api/cpp/telemetry/telemetry_exporter.h), backed by a private RAII guardthat drives two no-op private virtual hooks,
onBatchBegin()/onBatchEnd()(NVI idiom). Default no-ops leave the Prometheus / BUFFER / NOP exporters
unchanged.
nixlTelemetry::flushPendingEvents()runs the whole drain — the queued eventsplus the synthetic
AGENT_TELEMETRY_EVENTS_DROPPEDevent — inside onewithBatch.(a lazy
currentTimestamp()backed byinBatch_+std::optional<uint64_t> batchTimestamp_) and passes that timestamp explicitly to the counter / errorcounter / gauge append helpers.
timestamp (even when events are spaced past DOCA's millisecond resolution),
and one asserting a standalone
exportEvent(no batch) still stamps each eventfreshly.
No metric names, labels, event types, buffer format, or
TELEMETRY_VERSIONchange; the Prometheus scrape output (values, series, labels) is unchanged.
Why?
Every append helper (
appendCounterSample,appendErrorCounterSample,appendGaugeSample) previously calleddocaTimestamp()independently, so asingle event emitting a gauge + counter read the clock twice and a full drain of
N events triggered ~2N–3N reads — each a slightly different value for samples
that logically belong to the same flush instant. All events in one drain
represent the same snapshot moment, so they should share one timestamp: it is
cheaper (one clock read per batch) and semantically consistent.
Tracking: NIX-1625. This is PR 1 of 2; the companion NIX-1626 (constant
agent_namelabel) rebases onto this.How?
The batch boundary is an RAII-backed callback rather than manually-paired
begin/end calls:
withBatch(body)opens a privateBatchScopeguard, runsbody, and the guard's destructor closes the batch — so the boundary is alwaysbalanced and there is no naked scope for callers to mismanage. The begin/end
hooks are private virtuals (NVI), and
BatchScopeis a private implementationdetail, so
withBatchis the only entry point.The DOCA exporter fills
batchTimestamp_lazily on the first sample of a batch(zero cost for an empty batch) and returns it for every subsequent sample; a
standalone
exportEventoutside any batch still reads a fresh timestamp perevent. Histograms are unaffected — they are timestamped at
histogram_flushviaHISTOGRAM_TIMESTAMP_UPDATE, not at observe time. Thread-safety is unchanged:each
nixlTelemetrydrains on its own single-thread asio pool, soinBatch_/batchTimestamp_are only ever touched by one thread.Testing: DOCA suite (
doca_test,doca_nixl_test,histogram_parity_test)3/3 pass; telemetry gtests 60/60 pass; clang-format-19 clean; commit is
DCO-signed.
Summary by CodeRabbit