feat: usage telemetry - #956
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds anonymous telemetry for CLI invocations, setup steps, and terminal platform jobs, wires the ChangesCLI Telemetry
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 8
🧹 Nitpick comments (4)
packages/nemo_platform_ext/tests/cli/telemetry/conftest.py (1)
8-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the production list instead of copying it.
If
events._CI_ENV_VARSgains a var, this copy drifts and CI flakiness returns.♻️ Fix
-# Environment variables that is_ci_environment() treats as a CI signal. -_CI_ENV_VARS = ( - "CI", - "GITLAB_CI", - "GITHUB_ACTIONS", - "BUILDKITE", - "CIRCLECI", - "JENKINS_URL", - "TEAMCITY_VERSION", - "TF_BUILD", - "TRAVIS", -) +from nemo_platform_ext.cli.telemetry.events import _CI_ENV_VARS🤖 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 8 - 31, Update the _clear_ci_env fixture to reuse the production _CI_ENV_VARS definition from events instead of maintaining a local duplicate. Remove the copied tuple and reference the existing production symbol while preserving the current monkeypatch.delenv behavior for every configured CI variable.packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py (1)
47-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
expected_keysto a module constant.Three identical 30-key literals will drift. Also consider parameterizing the three near-identical tests over (event, expected params, name).
♻️ Sketch
+_EXPECTED_ENVELOPE_KEYS = { + "browserType", "clientId", "clientType", "clientVariant", "clientVer", + "cpuArchitecture", "deviceGdprBehOptIn", "deviceGdprFuncOptIn", + "deviceGdprTechOptIn", "deviceId", "deviceMake", "deviceModel", "deviceOS", + "deviceOSVersion", "deviceType", "eventProtocol", "eventSchemaVer", + "eventSysVer", "externalUserId", "gdprBehOptIn", "gdprFuncOptIn", + "gdprTechOptIn", "idpId", "integrationId", "productName", "productVersion", + "sentTs", "sessionId", "userId", "events", +}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, Hoist the repeated 30-key expected_keys literal into a module-level constant in test_wire_contract_smoke.py, then reuse it in all three affected tests. Keep the existing event-specific assertions unchanged; parameterize the near-identical tests over event, expected parameters, and name only if this can be done without altering their coverage.packages/nemo_platform_ext/tests/cli/telemetry/test_onboarding_events.py (1)
161-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant local
import typer.Already imported at line 16.
🤖 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_onboarding_events.py` around lines 161 - 171, Remove the redundant local import of typer from test_failure_emits_error_and_reraises, relying on the existing module-level import while leaving the test assertions and behavior unchanged.packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/runtime.py (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
ctx.
ctxis the only untyped parameter here;click.Context(import normally) satisfies the concrete-type-hint rule.As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints".
♻️ Proposed change
-def on_callback(ctx, *, no_telemetry: bool) -> None: +def on_callback(ctx: click.Context, *, no_telemetry: bool) -> None:Add at top of module:
import click🤖 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/runtime.py` at line 42, Update on_callback by importing click and annotating ctx as click.Context, while preserving the existing no_telemetry annotation and function behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli/configuration.mdx`:
- Around line 145-156: Reconcile the telemetry model-data disclosures with the
actual emitted payload: update docs/cli/configuration.mdx lines 145-156 and
docs/telemetry-and-privacy.mdx lines 27-33 consistently, stating either exact
model names or bucketed model data as implemented. Remove or qualify conflicting
claims so both pages describe the same payload accurately.
In `@docs/fern/snippets/_snippets/cli-summary.mdx`:
- Line 15: Remove the direct edit from
docs/fern/snippets/_snippets/cli-summary.mdx lines 15-15, update the CLI
documentation generator source responsible for docs/cli/reference.mdx lines
33-33, then run make generate-cli-reference-docs to regenerate both artifacts.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py`:
- Around line 541-550: Update the OnboardingStepEvent calls in the provider
creation flow around client.inference.providers.create so provider_type always
uses the fixed provider label expected by telemetry instead of the
user-controlled or host-derived name variable. Apply the same fixed value to
both the ERROR and COMPLETED events.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/help_formatter.py`:
- Around line 118-126: Update the exception handling around
super().main(standalone_mode=False) so KeyboardInterrupt is treated as a user
cancellation rather than an error: record the appropriate non-error task status
before re-raising it, while preserving existing SystemExit and other
BaseException handling.
- Around line 84-117: Update the exception handling in the wrapper around
super().main to catch plain click.ClickException after the more specific
UsageError and Exit cases, set status to TaskStatusEnum.ERROR, and call
handle_exception(e, e.ctx) when standalone_mode is enabled before re-raising;
preserve existing handling for UsageError, Exit, and Abort.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/waiters.py`:
- Line 103: Update the duration calculation in the waiter result to use the
job’s own start timestamp from job_status, so duration_sec represents actual job
runtime rather than polling time; if that timestamp is unavailable, rename and
document the field as wait duration instead.
- Around line 89-93: Update the model handling in the waiter event construction
to avoid emitting arbitrary API-provided model names: coerce non-null values to
str, then bucket or allow-list them to a safe known identifier, using
"undefined" for missing or unrecognized values. Preserve numeric token values
and ensure the resulting model field always satisfies JobRunEvent validation.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/handler.py`:
- Around line 299-303: The _run_sync path that creates a ThreadPoolExecutor must
shut it down after future.result completes. Wrap the executor lifecycle in a
context manager or equivalent guaranteed cleanup, while preserving the existing
30-second timeout and asyncio.run behavior.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/runtime.py`:
- Line 42: Update on_callback by importing click and annotating ctx as
click.Context, while preserving the existing no_telemetry annotation and
function behavior.
In `@packages/nemo_platform_ext/tests/cli/telemetry/conftest.py`:
- Around line 8-31: Update the _clear_ci_env fixture to reuse the production
_CI_ENV_VARS definition from events instead of maintaining a local duplicate.
Remove the copied tuple and reference the existing production symbol while
preserving the current monkeypatch.delenv behavior for every configured CI
variable.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_onboarding_events.py`:
- Around line 161-171: Remove the redundant local import of typer from
test_failure_emits_error_and_reraises, relying on the existing module-level
import while leaving the test assertions and behavior unchanged.
In `@packages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py`:
- Around line 47-78: Hoist the repeated 30-key expected_keys literal into a
module-level constant in test_wire_contract_smoke.py, then reuse it in all three
affected tests. Keep the existing event-specific assertions unchanged;
parameterize the near-identical tests over event, expected parameters, and name
only if this can be done without altering their coverage.
🪄 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: 5ec3284b-642a-4eba-8abb-c187135b41fd
⛔ Files ignored due to path filters (21)
sdk/python/nemo-platform/src/nemo_platform/cli/app.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/core/help_formatter.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/core/waiters.pyis excluded by!sdk/**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/runtime.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/commands/test_setup.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/core/test_waiters.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_command_hook.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_job_events.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_onboarding_events.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 (27)
README.mddocs/cli/configuration.mdxdocs/cli/reference.mdxdocs/fern/snippets/_snippets/cli-summary.mdxdocs/fern/versions/latest.ymldocs/telemetry-and-privacy.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/app.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/core/help_formatter.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/core/waiters.pypackages/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/runtime.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/commands/test_setup.pypackages/nemo_platform_ext/tests/cli/core/test_waiters.pypackages/nemo_platform_ext/tests/cli/telemetry/conftest.pypackages/nemo_platform_ext/tests/cli/telemetry/test_command_hook.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_job_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_onboarding_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py
Contributor
|
mckornfield
force-pushed
the
usage-telemetry/mck
branch
2 times, most recently
from
July 29, 2026 17:16
53317f7 to
fdb4a66
Compare
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 199: Update the documentation sentence near the nemo command to state
that exporting NEMO_TELEMETRY_ENABLED=false applies to the current shell session
and its child processes only. Remove the claim that telemetry remains disabled
for all future sessions unless the variable is explicitly added to shell startup
configuration.
🪄 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: 0e3a3f18-a58d-4bef-a60b-275321830184
⛔ Files ignored due to path filters (10)
sdk/python/nemo-platform/src/nemo_platform/cli/app.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/core/help_formatter.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/core/waiters.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/core/test_waiters.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_command_hook.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_job_events.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/telemetry/test_onboarding_events.pyis excluded by!sdk/**
📒 Files selected for processing (16)
README.mddocs/cli/configuration.mdxdocs/cli/reference.mdxdocs/fern/snippets/_snippets/cli-summary.mdxdocs/fern/versions/latest.ymldocs/telemetry-and-privacy.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/app.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/core/help_formatter.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/core/waiters.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/runtime.pypackages/nemo_platform_ext/tests/cli/commands/test_setup.pypackages/nemo_platform_ext/tests/cli/core/test_waiters.pypackages/nemo_platform_ext/tests/cli/telemetry/test_command_hook.pypackages/nemo_platform_ext/tests/cli/telemetry/test_job_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_onboarding_events.py
🚧 Files skipped from review as they are similar to previous changes (15)
- README.md
- docs/fern/versions/latest.yml
- docs/fern/snippets/_snippets/cli-summary.mdx
- docs/cli/reference.mdx
- packages/nemo_platform_ext/tests/cli/commands/test_setup.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/app.py
- packages/nemo_platform_ext/tests/cli/core/test_waiters.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/runtime.py
- packages/nemo_platform_ext/tests/cli/telemetry/test_command_hook.py
- docs/telemetry-and-privacy.mdx
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/waiters.py
- packages/nemo_platform_ext/tests/cli/telemetry/test_job_events.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/help_formatter.py
- packages/nemo_platform_ext/tests/cli/telemetry/test_onboarding_events.py
anastasia-nesterenko
approved these changes
Jul 29, 2026
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 @.github/workflows/fern-docs-preview-comment.yaml:
- Around line 43-49: Update the PR comment targeting logic around PR_NUMBER so
it does not trust the artifact value alone. Prefer the PR number from
github.event.workflow_run.pull_requests, or validate that the artifact metadata
head SHA matches github.event.workflow_run.head_sha before using its PR_NUMBER;
preserve the existing integer validation and only post the comment after the
trusted association is established.
🪄 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: 99bb2de1-824c-4fb6-867f-a71e441a3467
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/cli/telemetry/events.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_wire_contract_smoke.pyis excluded by!sdk/**
📒 Files selected for processing (5)
.github/workflows/fern-docs-preview-comment.yamldocs/set-up/index.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/telemetry/events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_events.pypackages/nemo_platform_ext/tests/cli/telemetry/test_wire_contract_smoke.py
mckornfield
force-pushed
the
usage-telemetry/mck
branch
2 times, most recently
from
July 29, 2026 21:23
048f5a6 to
f5c7332
Compare
tylersbray
approved these changes
Jul 30, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Nice, thanks. The maybe step name leak is the main thing that should be double checked, the extra setup.py cleanup would be nice, added some nitpicks just to make myself read it, no pressure.
mckornfield
force-pushed
the
usage-telemetry/mck
branch
from
July 30, 2026 19:02
f5c7332 to
fcee8fa
Compare
Contributor
mckornfield
force-pushed
the
usage-telemetry/mck
branch
3 times, most recently
from
July 31, 2026 17:25
0cbee5a to
30c2d52
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 3, 2026
mckornfield
force-pushed
the
usage-telemetry/mck
branch
from
August 3, 2026 22:46
30c2d52 to
3618b90
Compare
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
mckornfield
force-pushed
the
usage-telemetry/mck
branch
from
August 4, 2026 16:52
3618b90 to
fb3d379
Compare
mckornfield
enabled auto-merge
August 4, 2026 16:58
15 tasks
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
4 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
--no-telemetryto disable telemetry for the current invocation.--no-telemetry.