Skip to content

fix(client-runtime): omit machine labels from relay spans - #30940

Open
Mr-Neutr0n wants to merge 1 commit into
pingdotgg:mainfrom
Mr-Neutr0n:fix/relay-trace-labels
Open

Mr-Neutr0n wants to merge 1 commit into
pingdotgg:mainfrom
Mr-Neutr0n:fix/relay-trace-labels

Conversation

@Mr-Neutr0n

@Mr-Neutr0n Mr-Neutr0n commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Built-in relay connection spans include environment.label, which can be the user's computer name. The existing environment ID already identifies the connection for diagnostics, so exporting its human-readable label adds unnecessary identifying data.

Remove that attribute from the relay connection-attempt span. Retain environment ID, target kind, attempt/outcome metadata, UI labels, and local diagnostics. This is a focused data-minimization change to existing diagnostics, with no new setting or connection behavior.

Validation on macOS with Node 26.12.1:

  • Extended two existing supervisor cases to inspect spans from the actual relay tracer. Both failed before the fix because environment.label was exported.
  • All 59 supervisor tests pass after the fix, including successful reconnects and failed attempts. Environment ID/kind and UI labels remain available.
  • vp run check:changed passes the changed-file checks.

Scope is the explicit span attribute. Arbitrary error-message redaction is separate work; this does not promise that user-derived text can never appear in diagnostic errors. No production telemetry endpoint was contacted.

Model: GPT-6.1 Sol. Harness: Codex in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Nov 4, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at b69af3c

Macroscope's review found this PR approvable — Removes machine labels from relay connection-attempt spans while retaining IDs, target kinds and connection diagnostics. The change reduces telemetry exposure without altering connection behavior.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The relay connection-attempt span now records the environment ID and target kind directly. Tests verify the span attributes and confirm that the supervisor target retains its label.

Changes: Relay span attributes

Layer / File(s) Summary
Update relay span attributes and assertions
packages/client-runtime/src/connection/supervisor.ts, packages/client-runtime/src/connection/supervisor.test.ts
The relay attempt span sets environment.id and environment.target.kind directly and no longer adds environment.label. Tests check failed and credential-change attempts, and verify that the supervisor target retains its label.

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

Change: Bug fix

Possibly related PRs

  • pingdotgg/t3code#26556: Adds development logging for span attributes, which can display the relay attempt span attributes changed here.

Suggested labels: bugfix, size:S

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to b69af

No actionable merge-blocking risk was identified; the scoped telemetry change is ready to merge.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the client-runtime fix and the specific behavior change: omitting machine labels from relay spans.
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.
Description check Passed The description covers the problem, change, scope, approval rationale, and verification. It provides focused test results, environment details, limitations, and agent metadata. It does not include an …
Full details: Description check

Explanation

The description covers the problem, change, scope, approval rationale, and verification. It provides focused test results, environment details, limitations, and agent metadata. It does not include an issue link or maintainer approval comment, but it explains why this focused bug fix qualifies without prior discussion.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 9e85538 and b69af3c.

Files selected for processing: 2
  • packages/client-runtime/src/connection/supervisor.test.ts
  • packages/client-runtime/src/connection/supervisor.ts

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

@Mr-Neutr0n

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Nice to keep the connection diagnostics useful while sending a little less identifying data.

@Mr-Neutr0n

Copy link
Copy Markdown
Contributor Author

Added the attribution to the PR description to match the repository convention. Model: GPT-6.1 Sol. Harness: Codex in T3 Code.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant