Skip to content

[None][fix] Validate telemetry deployment IDs as UUIDs - #20777

Merged
Mgluhovskoi merged 2 commits into
NVIDIA:release/1.3from
Mgluhovskoi:fix/telemetry-generated-deployment-id
Oct 22, 2026
Merged

Mgluhovskoi merged 2 commits into
NVIDIA:release/1.3from
Mgluhovskoi:fix/telemetry-generated-deployment-id

Conversation

@Mgluhovskoi

@Mgluhovskoi Mgluhovskoi commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Validate TRTLLM_DISAGG_DEPLOYMENT_ID as a UUID before including it in telemetry. Valid values are normalized to 32 lowercase hexadecimal characters; missing or invalid values produce an empty deploymentId.

Preserves the existing environment-variable propagation and launcher-generated IDs, including shared UUIDs supplied by custom launchers. UUID validation checks format, not provenance or whether a UUID encodes identifying data.

Targets release/1.3. No launcher, schema version, event field, or cache-format changes. Existing historical records are unchanged; future non-UUID IDs are omitted and UUID formatting is normalized.

Test Coverage

  • Reporter, schema, transport, collectors, and opt-out suites: 238 passed, 6 optional tests skipped.
  • Covers missing/malformed IDs, paths, emails, valid UUIDs and normalization across initial, heartbeat, and exit payloads, including terminal-only failures.
  • Updated tests are already included through unittest/usage in CPU CI.
  • Pre-commit checks passed.

Dev Engineer Review

  • usage_lib.py validates TRTLLM_DISAGG_DEPLOYMENT_ID as a UUID.
  • Valid IDs are normalized to 32 lowercase hexadecimal characters.
  • Missing or invalid IDs produce an empty deploymentId.
  • Initial, heartbeat, and terminal telemetry use the normalized value.
  • No launcher, schema version, event field, or cache-format changes are indicated.
  • Review finding counts are unavailable from the supplied evidence.

QA Engineer Review

  • tests/unittest/usage/test_reporter.py was modified.
  • Tests cover missing, malformed, identifying, incorrectly sized, valid, and non-canonical IDs.
  • Tests cover initial, heartbeat, exit, and terminal-event payloads.
  • Tests verify that rejected IDs are absent from serialized telemetry.
  • unittest/usage is listed in tests/integration/test_lists/test-db/l0_cpu.yml.
  • Reported results include 238 tests passed and 6 optional tests skipped.
  • Coverage verdict: sufficient.

Per-File QA Perspective

  • tensorrt_llm/usage/schemas/README.md: Documents UUID validation, lowercase hexadecimal normalization, and empty output for missing or invalid IDs.
  • tensorrt_llm/usage/usage_lib.py: Verify normalized IDs and empty output across session metadata and telemetry event types. Verify existing environment propagation and snapshot length clamping.
  • tests/unittest/usage/test_reporter.py: Covers validation, normalization, privacy behavior, and terminal-event correlation. The suite is listed in the CPU CI test list.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
@Mgluhovskoi
Mgluhovskoi force-pushed the fix/telemetry-generated-deployment-id branch from e4a9254 to 5ed325b Compare October 21, 2026 22:47
@Mgluhovskoi
Mgluhovskoi changed the base branch from main to release/1.3 October 21, 2026 22:47
… propagation

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
@Mgluhovskoi Mgluhovskoi changed the title [None][fix] Remove user-provided telemetry deployment IDs [None][fix] Validate telemetry deployment IDs as UUIDs Oct 21, 2026
@Mgluhovskoi
Mgluhovskoi marked this pull request as ready for review October 21, 2026 23:12
@Mgluhovskoi
Mgluhovskoi requested a review from a team as a code owner October 21, 2026 23:12
@Mgluhovskoi
Mgluhovskoi requested a review from arysef October 21, 2026 23:12
@Mgluhovskoi

Copy link
Copy Markdown
Collaborator Author

/bot run

1 similar comment
@Mgluhovskoi

Copy link
Copy Markdown
Collaborator Author

/bot run

@coderabbitai

coderabbitai Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: cc1d7b84-4b96-4614-a288-452d51e9e327
📥 Commits

Reviewing files that changed from the base of the PR and between 135d95a and 276ed1e.

📒 Files selected for processing: 3
  • tensorrt_llm/usage/schemas/README.md
  • tensorrt_llm/usage/usage_lib.py
  • tests/unittest/usage/test_reporter.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Disaggregated telemetry now parses deployment IDs as UUIDs and emits valid values as lowercase hexadecimal without hyphens. Missing or invalid values produce an empty string. Documentation and tests cover the updated behavior.

Changes

Deployment ID normalization

Layer / File(s) Summary
UUID validation and normalization
tensorrt_llm/usage/usage_lib.py, tensorrt_llm/usage/schemas/README.md, tests/unittest/usage/test_reporter.py
The telemetry helper parses deployment IDs as UUIDs and returns lowercase hexadecimal. Documentation and tests specify and verify this format, including empty output for invalid or unset values.
Telemetry report integration
tensorrt_llm/usage/usage_lib.py, tests/unittest/usage/test_reporter.py
Session metadata and empty event snapshots use normalized deployment IDs. Payload tests check valid and rejected IDs across report events, including terminal-event correlation.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: venkywonka

Merge Risk: ⚪ Minimal · up to 276ed

No actionable issue is established; the change appears mergeable after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title follows the required [None][fix] format and clearly states that telemetry deployment IDs are validated as UUIDs.
Description check Passed The description explains the problem, solution, scope, compatibility impact, and test coverage. It omits the PR Checklist section, but the core required information is complete.
Docstring Coverage Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 u…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79704 [ run ] triggered by Bot. Commit: 276ed1e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79705 [ run ] triggered by Bot. Commit: 276ed1e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79704 [ run ] completed with state ABORTED. Commit: 276ed1e

Link to invocation

@github-actions

Copy link
Copy Markdown

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79705 [ run ] completed with state FAILURE. Commit: 276ed1e
/LLM/release-1.3/L0_MergeRequest_PR pipeline #478 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Mgluhovskoi

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79813 [ run ] triggered by Bot. Commit: 276ed1e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79813 [ run ] completed with state FAILURE. Commit: 276ed1e
/LLM/release-1.3/L0_MergeRequest_PR pipeline #498 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Mgluhovskoi

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79903 [ run ] triggered by Bot. Commit: 276ed1e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #79903 [ run ] completed with state SUCCESS. Commit: 276ed1e
/LLM/release-1.3/L0_MergeRequest_PR pipeline #503 completed with status: 'SUCCESS'

CI Report

Link to invocation

@Mgluhovskoi
Mgluhovskoi merged commit 6c172f3 into NVIDIA:release/1.3 Oct 22, 2026
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Oct 26, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Nov 2, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Nov 2, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Nov 3, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Nov 3, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
SimengLiu-nv pushed a commit to SimengLiu-nv/TensorRT-LLM that referenced this pull request Nov 3, 2026
…etry deployment IDs as UUIDs

Forward-port of release/1.3 PR NVIDIA#20777
(release/1.3 commit 6c172f3) to main.

Adapted to main: the new payload test patches _HEARTBEAT_STOP, which main
renamed from _REPORTER_STOP.

Signed-off-by: Maxim Gluhovskoi <mgluhovskoi@nvidia.com>
Signed-off-by: Simeng Liu <simengl@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants