feat(cli): add telemetry client library - #930
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a CLI telemetry subsystem with typed event schemas, persisted session IDs, layered opt-out controls, asynchronous and synchronous delivery, retry/DLQ handling, first-run messaging, configuration, documentation, and comprehensive tests. ChangesCLI telemetry
Sequence Diagram(s)sequenceDiagram
participant CLI
participant emit_event
participant TelemetryHandler
participant httpx.AsyncClient
participant TelemetryEndpoint
CLI->>emit_event: Submit PlatformTelemetryEvent
emit_event->>TelemetryHandler: Check opt-in and enqueue event
TelemetryHandler->>httpx.AsyncClient: Build and send batch
httpx.AsyncClient->>TelemetryEndpoint: POST telemetry payload
TelemetryEndpoint-->>httpx.AsyncClient: Return HTTP status
httpx.AsyncClient-->>TelemetryHandler: Complete delivery or route to DLQ
TelemetryHandler-->>CLI: Stop and flush without raising
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 7
🧹 Nitpick comments (5)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/__init__.py (1)
4-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePublic
__all__exporting a private name.
_telemetry_enabledis underscore-private but re-exported as public surface. Either rename it (e.g.telemetry_enabled) or drop it from__all__and keep callers importing fromhandler.🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/__init__.py` around lines 4 - 16, Remove _telemetry_enabled from __all__ and stop re-exporting it from the telemetry package initializer; keep the private symbol available through the handler module for existing internal callers.packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py (1)
47-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the 30-key envelope set to one module-level constant.
Three identical copies; any schema change requires three edits.
Also applies to: 114-145, 182-213
🤖 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 `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py` around lines 47 - 78, Define one module-level constant for the 30-key telemetry envelope set and replace the duplicated local sets in the affected test cases with references to it. Update the three locations consistently so future schema changes require editing only the shared constant.packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py (1)
87-97: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMarker write should precede the notice, or a failing
touchreprints forever.If
mkdir/touchraises, the notice is emitted on every invocation. Create the marker first, then write the text.🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py` around lines 87 - 97, Update maybe_print_first_run_notice so the marker directory creation and marker touch complete before writing _NOTICE_TEXT to stderr. Preserve the existing opt-in and marker-exists early returns, and allow marker creation failures to prevent the notice from being emitted.packages/nemo_platform_ext/tests/cli/telemetry/test_handler.py (1)
174-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest doesn't test what its name claims.
test_enqueue_at_max_queue_size_signals_flush_when_runningnever starts a loop and only asserts queue length; the signalling path is unverified. Either rename to reflect the no-loop no-op case or start the handler and assert the flush fired.🤖 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 `@packages/nemo_platform_ext/tests/cli/telemetry/test_handler.py` around lines 174 - 181, The test test_enqueue_at_max_queue_size_signals_flush_when_running does not exercise the running-loop signalling behavior. Start the TelemetryHandler’s background loop in the test and assert that reaching max_queue_size triggers the flush signal; otherwise rename the test to describe the no-loop behavior it currently verifies.packages/nemo_platform_ext/tests/cli/telemetry/conftest.py (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated CI var list will drift.
Import
_CI_ENV_VARSfromnemo_platform_ext.cli.telemetry.eventsinstead of copying it, so a new CI signal doesn't silently break determinism here.🤖 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 `@packages/nemo_platform_ext/tests/cli/telemetry/conftest.py` around lines 9 - 19, Remove the local _CI_ENV_VARS tuple in the telemetry test configuration and import _CI_ENV_VARS from nemo_platform_ext.cli.telemetry.events instead. Update references to use the imported constant so the tests share the production CI-signal list.
🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py`:
- Around line 33-44: Update _config_opted_out to read telemetry_enabled directly
from cfg.get_config_file() instead of using getattr with a default, while
preserving the existing exception handler so missing or invalid configuration
fails closed by returning True.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.py`:
- Around line 63-64: Remove the unused _deployment_type() helper from handler.py
and update its tests to reuse the existing events.py _deployment_type()
implementation. Preserve the events.py helper’s DeploymentTypeEnum return type
and "cli" default as the single source of truth for NEMO_DEPLOYMENT_TYPE.
- Around line 299-303: Update the _run_sync method’s running-loop branch to
create ThreadPoolExecutor through a context manager, ensuring the executor is
shut down after future.result(timeout=30) completes while preserving the
existing coroutine execution and timeout behavior.
- Around line 418-433: Update the response-status handling in
_send_events_with_client so HTTP 429 is included in the same retry/DLQ branch as
408 and 5xx responses. Preserve the existing success, payload-too-large
splitting, and other status behavior.
- Around line 30-31: Update the telemetry handler’s module-level imports to
import httpx at runtime instead of only under TYPE_CHECKING, remove the
redundant local import in _send_events(), and declare httpx as a dependency in
pyproject.toml.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_emit.py`:
- Around line 46-47: Isolate telemetry tests from the developer’s real
configuration by adding an autouse fixture that points NMP_CONFIG_FILE to a
temporary file under tmp_path. Ensure the fixture applies to test_default_is_on
and the notice-related tests, while preserving their existing assertions.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py`:
- Line 93: Update the telemetry test environment cleanup fixture in conftest.py
to remove NEMO_DEPLOYMENT_TYPE automatically before each test, ensuring
deploymentType remains "cli" regardless of ambient environment variables. Apply
this cleanup to the assertions at the referenced locations without changing
their expected behavior.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/__init__.py`:
- Around line 4-16: Remove _telemetry_enabled from __all__ and stop re-exporting
it from the telemetry package initializer; keep the private symbol available
through the handler module for existing internal callers.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py`:
- Around line 87-97: Update maybe_print_first_run_notice so the marker directory
creation and marker touch complete before writing _NOTICE_TEXT to stderr.
Preserve the existing opt-in and marker-exists early returns, and allow marker
creation failures to prevent the notice from being emitted.
In `@packages/nemo_platform_ext/tests/cli/telemetry/conftest.py`:
- Around line 9-19: Remove the local _CI_ENV_VARS tuple in the telemetry test
configuration and import _CI_ENV_VARS from
nemo_platform_ext.cli.telemetry.events instead. Update references to use the
imported constant so the tests share the production CI-signal list.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_handler.py`:
- Around line 174-181: The test
test_enqueue_at_max_queue_size_signals_flush_when_running does not exercise the
running-loop signalling behavior. Start the TelemetryHandler’s background loop
in the test and assert that reaching max_queue_size triggers the flush signal;
otherwise rename the test to describe the no-loop behavior it currently
verifies.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py`:
- Around line 47-78: Define one module-level constant for the 30-key telemetry
envelope set and replace the duplicated local sets in the affected test cases
with references to it. Update the three locations consistently so future schema
changes require editing only the shared constant.
🪄 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: 7e765e47-358f-430e-90b1-feab7e54c2f8
⛔ Files ignored due to path filters (11)
sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/emit.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/events.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/handler.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/session.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/config/models.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/conftest.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_emit.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_events.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_handler.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_wire_contract_smoke.pyis excluded by!sdk/**
📒 Files selected for processing (11)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/events.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/session.pypackages/nemo_platform_ext/src/nemo_platform_ext/config/models.pypackages/nemo_platform_ext/tests/cli/telemetry/conftest.pypackages/nemo_platform_ext/tests/cli/telemetry/test_emit.pypackages/nemo_platform_ext/tests/cli/telemetry/test_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_handler.pypackages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py
Contributor
|
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
mckornfield
force-pushed
the
add-client-lib-telemetry/mck
branch
from
July 28, 2026 00:03
d28ff44 to
0dc886d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py (1)
94-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isCi is Falsewill fail on any CI runner unless conftest clearsCI.GitHub Actions and most runners export
CI=true, so this assertion inverts in the very environment the file targets. Same ambient-env exposure as the previously flaggeddeploymentType == "cli"assertions. Eithermonkeypatch.delenvboth (CI,NEMO_DEPLOYMENT_TYPE) in the telemetryconftest.py, or assert on the key's presence/type instead of the value.Also applies to Lines 131-134 and 169-172.
#!/bin/bash fd -t f 'conftest.py' packages/nemo_platform_ext/tests | xargs rg -n 'delenv|CI|NEMO_DEPLOYMENT_TYPE' -C2 rg -n '_is_ci|isCi' packages/nemo_platform_ext/src -C3🤖 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 `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py` around lines 94 - 97, Update the telemetry test setup and assertions around the relevant smoke tests so ambient CI and deployment environment variables cannot make results runner-dependent: clear CI and NEMO_DEPLOYMENT_TYPE via the telemetry conftest fixture, or replace the value assertions with checks for key presence and boolean/string type. Apply the same fix to the assertions near all three referenced test sections and keep _is_ci behavior covered.
🤖 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 `@packages/nemo_platform_ext/tests/cli/telemetry/test_emit.py`:
- Around line 16-22: Update the autouse fixture _isolate_local_telemetry_state
to remove NEMO_TELEMETRY_ENABLED via monkeypatch.delenv before yielding,
ensuring test_default_is_on and notice tests are unaffected by the developer’s
environment while retaining existing cleanup behavior.
In `@plugins/nemo-customizer/openapi/openapi.yaml`:
- Around line 1488-1492: Update the PlatformJobLogPage schema so next_page and
prev_page explicitly allow null while remaining required, and update
PlatformJobTaskStatusResponse.error_stack similarly; preserve required fields
while ensuring generated clients accept null for absent pagination links and
successful task responses.
- Around line 395-400: The OpenAPI change removes the existing RL job
compatibility endpoint while nemo-rl callers and documentation still use it.
Preserve POST /apis/customization/v2/workspaces/{workspace}/rl/jobs as an alias
with the same contract as the Unsloth job endpoint, or update all nemo-rl code
and documentation references to the new path in the same change.
---
Duplicate comments:
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py`:
- Around line 94-97: Update the telemetry test setup and assertions around the
relevant smoke tests so ambient CI and deployment environment variables cannot
make results runner-dependent: clear CI and NEMO_DEPLOYMENT_TYPE via the
telemetry conftest fixture, or replace the value assertions with checks for key
presence and boolean/string type. Apply the same fix to the assertions near all
three referenced test sections and keep _is_ci behavior covered.
🪄 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: 6b281b09-a36c-4c1d-9ab1-40c6206010c3
⛔ Files ignored due to path filters (13)
sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/emit.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/events.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/handler.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/session.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/config/models.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/conftest.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_emit.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_events.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_handler.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_wire_contract_smoke.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
packages/nemo_platform_ext/pyproject.tomlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/events.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/session.pypackages/nemo_platform_ext/src/nemo_platform_ext/config/models.pypackages/nemo_platform_ext/tests/cli/telemetry/conftest.pypackages/nemo_platform_ext/tests/cli/telemetry/test_emit.pypackages/nemo_platform_ext/tests/cli/telemetry/test_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_handler.pypackages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.pyplugins/nemo-customizer/openapi/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/init.py
- packages/nemo_platform_ext/tests/cli/telemetry/conftest.py
- packages/nemo_platform_ext/src/nemo_platform_ext/config/models.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/session.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/events.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.py
- packages/nemo_platform_ext/tests/cli/telemetry/test_events.py
tylersbray
approved these changes
Jul 28, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Looks good, here's a nonblocking approval for you. Check that openapi file, the docs and our official "we tried to tell you" posture.
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Contributor
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 `@docs/cli/configuration.mdx`:
- Line 138: Clarify the precedence statement in the telemetry configuration
documentation to explicitly identify telemetry as an exception: persisted
telemetry_enabled: false must disable telemetry even when
NEMO_TELEMETRY_ENABLED=true, while other environment values follow the
documented environment-layer behavior.
🪄 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: e0dd6ec6-fe58-4901-bc2b-7a21a1aaad94
⛔ Files ignored due to path filters (9)
sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/emit.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/events.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/handler.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/session.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/conftest.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_emit.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_events.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_handler.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_wire_contract_smoke.pyis excluded by!sdk/**
📒 Files selected for processing (5)
docs/cli/configuration.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.pypackages/nemo_platform_ext/tests/cli/telemetry/test_emit.pypackages/nemo_platform_ext/tests/cli/telemetry/test_handler.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/emit.py
- packages/nemo_platform_ext/tests/cli/telemetry/test_handler.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.py
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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
2 participants
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.
Summary by CodeRabbit