Remove timestamp field from telemetry events

[BatshevaBlack]

What?

Removes the per-event microsecond timestamp (timestampUs_) from nixlTelemetryEvent, updates all constructors and call sites (core telemetry, backend engine, buffer readers), and bumps TELEMETRY_VERSION to 2 so shared-memory ring buffers reject mismatched layouts. Example readers (C++ and Python) no longer print or decode a per-event timestamp field.

Why?

Recording a timestamp on every event added cost (syscall / clock read and 8 bytes per event) without any exporter or downstream consumer using it. Dropping the field reduces hot-path overhead and shrinks each event.

Summary by CodeRabbit

• Breaking Changes

  • Telemetry schema bumped to version 2.
  • Per-event timestamps removed; records now include only category, event name, and value. CSV/export/output reflect the new format.

• Documentation

  • Docs updated to remove per-event timestamp field and show revised example output and behavior.

• Examples

  • Example readers updated to stop displaying timestamps and match the new telemetry format.

• Tests

  • Unit tests adjusted to no longer assert per-event timestamps.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

👋 Hi BatshevaBlack! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

[ovidiusm]

/build

[ovidiusm]

/ok to test f72f03a

[coderabbitai]

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

Walkthrough

Per-event microsecond timestamps were removed from the telemetry schema and code; TELEMETRY_VERSION bumped to 2. Constructors, event storage, exporters, examples, tests, and docs were updated to omit timestamp fields, computations, and outputs, keeping category, event name, and value only.

Changes

Cohort / File(s)	Summary
Core Telemetry Schema src/core/telemetry/telemetry_event.h	Bumped TELEMETRY_VERSION 1→2, removed uint64_t timestampUs_ from nixlTelemetryEvent, and changed constructors to omit the timestamp_us parameter.
Telemetry Implementation src/core/telemetry/telemetry.cpp, src/api/cpp/backend/backend_engine.h	Removed timestamp computations and updated emplace_back(...) callsites to enqueue events with (category, event_name, value) only.
Examples examples/cpp/telemetry_reader.cpp, examples/python/telemetry_reader.py	Deleted timestamp formatting helper and removed Timestamp: output; Python example updated TELEMETRY_VERSION to 2 and removed timestamp_us from NixlTelemetryEvent ctypes structure.
Plugins / Exporter src/plugins/telemetry/README.md	CSV header and export rows no longer include timestamp_us; rows now emit category, event_name, and value only.
Tests test/gtest/telemetry_test.cpp	Adjusted TelemetryEventStructure test to construct events without timestamp and removed assertions referencing timestampUs_.
Docs docs/telemetry.md	Removed Timestamp field and example timestamp lines; updated wording to reflect events are recorded/ordered without per-event timestamps.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through fields of microsecond dust,

I tucked the clocks away and kept my trust.
Version two skips ticks and hums,
Events march on — no timestamp drums.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: removing the timestamp field from telemetry events.
Description check	✅ Passed	The description fully addresses the required template sections (What, Why) with clear, specific information about the changes and their justification.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

examples/python/telemetry_reader.py (1)

25-26: ⚠️ Potential issue | 🟡 Minor

Fix isort pre-commit hook failure.

The pipeline indicates that isort requires a blank line before the # Set up logging comment to separate imports from module-level code.

🔧 Proposed fix

 import time
+
 # Set up logging
 logging.basicConfig(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/python/telemetry_reader.py` around lines 25 - 26, isort is failing
because there is no blank line separating the import block from module-level
code; add a single blank line after the import statement (the line containing
"import time") so there is an empty line before the "# Set up logging" comment
to satisfy isort's import separation rule.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@examples/python/telemetry_reader.py`:
- Around line 25-26: isort is failing because there is no blank line separating
the import block from module-level code; add a single blank line after the
import statement (the line containing "import time") so there is an empty line
before the "# Set up logging" comment to satisfy isort's import separation rule.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 59ca529a-eec7-4ee8-964e-b21732f6f5af

📥 Commits

Reviewing files that changed from the base of the PR and between e6dee7f and b275aab.

📒 Files selected for processing (7)

• examples/cpp/telemetry_reader.cpp
• examples/python/telemetry_reader.py
• src/api/cpp/backend/backend_engine.h
• src/core/telemetry/telemetry.cpp
• src/core/telemetry/telemetry_event.h
• src/plugins/telemetry/README.md
• test/gtest/telemetry_test.cpp

💤 Files with no reviewable changes (1)

• examples/cpp/telemetry_reader.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

examples/python/telemetry_reader.py (2)

91-91: ⚠️ Potential issue | 🟡 Minor

Update the constructor default to TELEMETRY_VERSION.

SharedRingBuffer() still defaults to version=1, so any direct use of this helper now rejects the v2 buffers this PR writes. The CLI path is fine because Line 261 passes TELEMETRY_VERSION explicitly, but the class API is now stale.

Suggested fix

-    def __init__(self, file_path, version=1):
+    def __init__(self, file_path, version=TELEMETRY_VERSION):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/python/telemetry_reader.py` at line 91, The constructor default for
SharedRingBuffer (__init__) still uses version=1 which rejects v2 buffers;
change the default to use TELEMETRY_VERSION instead (i.e., def __init__(self,
file_path, version=TELEMETRY_VERSION):), and ensure TELEMETRY_VERSION is
available in the module (import or reference the constant where defined) so
callers that rely on the default will accept v2 buffers.

---

123-128: ⚠️ Potential issue | 🟡 Minor

Don't delete temp_header before formatting the mismatch error.

On a version mismatch, this branch deletes temp_header and then immediately reads temp_header.version in the f-string. That raises UnboundLocalError instead of the intended schema-mismatch error, which is now a likely user path after bumping the buffer version to 2.

Suggested fix

         if temp_header.version != self.version:
+            actual_version = temp_header.version
             del temp_header
             header_mmap.close()
             raise RuntimeError(
-                f"Version mismatch: expected {self.version}, got {temp_header.version}"
+                f"Version mismatch: expected {self.version}, got {actual_version}"
             )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/python/telemetry_reader.py` around lines 123 - 128, The branch that
handles a version mismatch deletes temp_header before constructing the
RuntimeError message, causing an UnboundLocalError when the f-string reads
temp_header.version; update the mismatch branch in telemetry_reader.py to not
delete temp_header (or move the del temp_header after raising) and then close
header_mmap and raise RuntimeError(f"Version mismatch: expected {self.version},
got {temp_header.version}") so the error message can access temp_header.version
(symbols: temp_header, header_mmap, self.version).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@examples/python/telemetry_reader.py`:
- Line 91: The constructor default for SharedRingBuffer (__init__) still uses
version=1 which rejects v2 buffers; change the default to use TELEMETRY_VERSION
instead (i.e., def __init__(self, file_path, version=TELEMETRY_VERSION):), and
ensure TELEMETRY_VERSION is available in the module (import or reference the
constant where defined) so callers that rely on the default will accept v2
buffers.
- Around line 123-128: The branch that handles a version mismatch deletes
temp_header before constructing the RuntimeError message, causing an
UnboundLocalError when the f-string reads temp_header.version; update the
mismatch branch in telemetry_reader.py to not delete temp_header (or move the
del temp_header after raising) and then close header_mmap and raise
RuntimeError(f"Version mismatch: expected {self.version}, got
{temp_header.version}") so the error message can access temp_header.version
(symbols: temp_header, header_mmap, self.version).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 67768f85-51b8-42ca-a2e0-f03fba018314

📥 Commits

Reviewing files that changed from the base of the PR and between b275aab and f72f03a.

📒 Files selected for processing (1)

• examples/python/telemetry_reader.py

[alexanderbilk]

Fix https://github.com/ai-dynamo/nixl/blob/main/docs/telemetry.md accordingly
The rest seems fine

[ovidiusm]

/build

[ovidiusm]

/ok to test 84c9043

[coderabbitai]

Actionable comments posted: 4

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/backend/backend_engine.h (1)

65-70: ⚠️ Potential issue | 🔴 Critical

Lock before touching telemetryEvents_.

Line 67 reads telemetryEvents_ before telemetryEventsMutex_ is acquired. That races with Line 69's emplace_back(...) and with getTelemetryEvents() at Lines 89-91, which is undefined behavior and still allows the queue to overshoot MAX_TELEMETRY_QUEUE_SIZE under contention.

🔒 Suggested fix

         void
         addTelemetryEvent(const std::string &event_name, uint64_t value) {
-            if (!enableTelemetry_) return;
-            if (telemetryEvents_.size() >= MAX_TELEMETRY_QUEUE_SIZE) return;
-            std::lock_guard<std::mutex> lock(telemetryEventsMutex_);
+            if (!enableTelemetry_) {
+                return;
+            }
+            const std::lock_guard<std::mutex> lock(telemetryEventsMutex_);
+            if (telemetryEvents_.size() >= MAX_TELEMETRY_QUEUE_SIZE) {
+                return;
+            }
             telemetryEvents_.emplace_back(
                 nixl_telemetry_category_t::NIXL_TELEMETRY_BACKEND, event_name, value);
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/cpp/backend/backend_engine.h` around lines 65 - 70, In
addTelemetryEvent, acquire telemetryEventsMutex_ before inspecting or modifying
telemetryEvents_ to avoid races: keep the enableTelemetry_ check early if
desired, then lock telemetryEventsMutex_ and perform the size check against
MAX_TELEMETRY_QUEUE_SIZE and the emplace_back under that same lock; similarly
ensure getTelemetryEvents() uses the same mutex for consistent access. This
prevents the queue from being read/modified concurrently and avoids overshooting
MAX_TELEMETRY_QUEUE_SIZE.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/telemetry.md`:
- Line 63: The sentence "Events are recorded after the corresponding operation
completes and appear in order." implies a global completion-order guarantee;
change it to clarify that only enqueue order is preserved across producers and
consumers observe buffer insertion order, not global operation completion
order—for example, replace that sentence with wording that states events are
recorded when enqueued, concurrent producers serialize on enqueue, and consumers
see insertion order but there is no cross-thread global completion ordering
guarantee.

In `@examples/python/telemetry_reader.py`:
- Line 36: The version mismatch handler crashes because TELEMETRY_VERSION was
bumped to 2 while SharedRingBuffer.__init__ still defaults to version=1 and the
handler deletes temp_header before reading temp_header.version; update the
initialization and/or handler so they use the module-level TELEMETRY_VERSION
consistently and avoid accessing a deleted variable: change
SharedRingBuffer.__init__ to default version=TELEMETRY_VERSION (or read
temp_header.version into a local var before deleting temp_header) and adjust the
version-mismatch error message to reference that local var instead of
temp_header.version.

In `@src/core/telemetry/telemetry_event.h`:
- Line 30: The API version is stale: add V2 = 2 to the
nixl_telemetry_plugin_api_version enum in
src/api/cpp/telemetry/telemetry_plugin.h so it matches the new struct layout
(TELEMETRY_VERSION = 2), and update all exporter plugin registrations
(prometheus, doca, csv, etc.) to register with the new V2 value; ensure
exporters that implement exportEvent(const nixlTelemetryEvent &event) are
registered with V2 to avoid ABI/struct-layout mismatches with
nixlTelemetryEvent.
- Around line 64-76: Change the enum declaration to give
nixl_telemetry_category_t an explicit underlying type that matches the Python
ctypes usage (e.g., "enum class nixl_telemetry_category_t : std::int32_t") and
include <cstdint>, then add a compile-time assertion that the C++ struct layout
used by nixlTelemetryEvent is stable for the mmap/ctypes consumer (for example a
static_assert that sizeof(nixlTelemetryEvent) == sizeof(std::int32_t) +
MAX_EVENT_NAME_LEN + sizeof(uint64_t) and/or that offsetof(nixlTelemetryEvent,
category_) == 0), so the category field and overall struct size match the Python
side; this touches the enum class nixl_telemetry_category_t and the struct/type
nixlTelemetryEvent (and include headers if needed).

---

Outside diff comments:
In `@src/api/cpp/backend/backend_engine.h`:
- Around line 65-70: In addTelemetryEvent, acquire telemetryEventsMutex_ before
inspecting or modifying telemetryEvents_ to avoid races: keep the
enableTelemetry_ check early if desired, then lock telemetryEventsMutex_ and
perform the size check against MAX_TELEMETRY_QUEUE_SIZE and the emplace_back
under that same lock; similarly ensure getTelemetryEvents() uses the same mutex
for consistent access. This prevents the queue from being read/modified
concurrently and avoids overshooting MAX_TELEMETRY_QUEUE_SIZE.

🪄 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: Pro

Run ID: e7208217-0d95-4b32-a9bc-b8204ed13b20

📥 Commits

Reviewing files that changed from the base of the PR and between 38b1794 and 5c7862b.

📒 Files selected for processing (8)

• docs/telemetry.md
• examples/cpp/telemetry_reader.cpp
• examples/python/telemetry_reader.py
• src/api/cpp/backend/backend_engine.h
• src/core/telemetry/telemetry.cpp
• src/core/telemetry/telemetry_event.h
• src/plugins/telemetry/README.md
• test/gtest/telemetry_test.cpp

💤 Files with no reviewable changes (1)

• examples/cpp/telemetry_reader.cpp

[ColinNV]

Btw, getting the current time on Linux is often a userspace operation without system call overhead. Of course if the timestamp is never needed we should still get rid of it.

[alexanderbilk]

/ok to test ae39e41

[brminich]

/build

[ColinNV]

/build

[mkhazraee]

I'm not involved in the details of telemetry implementation, but rather surprised by title of this PR. The whole point of telemetry was to measure post and completion time, are we doing it in another way? Or in some update this capability was dropped? This sounds off to me.

[alexanderbilk]

@mkhazraee post and completion time are getting exported as event values. What we are getting rid of is redundant timestamps of 'When event was captured". We have no real use of it, while turned out it has quite an impact on performance in some cases

[brminich]

/ok to test 0f171d0

[brminich]

/build