Skip to content

fix(observability): resource attributes follow OTel semantic conventions - #68

Open
yordis wants to merge 1 commit into
mainfrom
yordis/fix-otel-resource-semconv
Open

yordis wants to merge 1 commit into
mainfrom
yordis/fix-otel-resource-semconv

Conversation

@yordis

@yordis yordis commented Sep 28, 2026 •

Copy link
Copy Markdown
Member
  • Traces from the desktop window arrive as t3code-web, and the only thing telling them apart from a browser tab was service.mode, which nobody could find without reading the code.
  • service.mode and service.runtime squatted in the service.* namespace OpenTelemetry reserves, and service.mode meant a different thing in each service.
  • Attributes named by the semantic conventions are the ones collectors, dashboards, and vendors already know how to group and filter on.
  • An operator-set deployment.environment.name has to keep winning over the default the desktop app reports.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • New Features
    • Telemetry now distinguishes web and desktop clients and identifies whether a server is managed by the desktop app or runs standalone.
    • Telemetry includes runtime and browser details, including user agent and language information, where available.
    • The deployment environment can be specified through telemetry resource attributes.
  • Documentation
    • Updated observability guidance to explain the new telemetry attributes and service names.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Sep 28, 2026
@cursor

cursor Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Cross-cutting telemetry label changes can break saved queries and dashboards on old resource keys, but they do not alter auth, data handling, or core app behavior.

Overview
OTLP resource identity is aligned with OpenTelemetry semantic conventions instead of overloading reserved service.* keys like service.mode and service.runtime.

Desktop main process now sets service.version, standard process.runtime.* (via shared nodeProcessRuntimeAttributes()), and deployment.environment.name (default dev/prod, overridden when present in OTEL_RESOURCE_ATTRIBUTES). Server exports the same Node runtime attributes plus t3.server.managed_by (desktop vs standalone) and version from package.json. Web UI traces use t3.client.surface (web vs desktop window) and browser-style user_agent.original / browser.* attributes. Relay client tracing maps runtime to process.runtime.name and component to t3.component.

Tests and observability docs were updated for the new keys; fork ledger 0026 records the divergence and notes that existing dashboards filtering on the old attribute names must be updated.

Reviewed by Cursor Bugbot for commit 88878cc. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.5 KiB −6 B (−0.0%) 15.1 KiB ✅
Codex Thread snapshot wire 7.1 KiB 7.1 KiB 0 B (0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.5 KiB 6.4 KiB −6 B (−0.1%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 56.2 KiB 56.2 KiB 0 B (0.0%) 66.4 KiB ✅
Codex Live turn messages 9 9 0 (0.0%) 21 ✅
Claude Total thread wire 13.5 KiB 13.5 KiB −28 B (−0.2%) 15.1 KiB ✅
Claude Thread snapshot wire 7.1 KiB 7.1 KiB −2 B (−0.0%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.4 KiB 6.4 KiB −26 B (−0.4%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.0 KiB 57.0 KiB −44 B (−0.1%) 66.4 KiB ✅
Claude Live turn messages 9 8 −1 (−11.1%) 21 ✅

Baseline: f5cb3b4 · PR result: 88878cc · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 114.0 KiB
  • Claude decoded thread snapshot: 114.7 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: TrogonStack/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5e556d5e-36bb-4f62-a3e2-9f4ab5bdcd20

📥 Commits

Reviewing files that changed from the base of the PR and between f5cb3b4 and 88878cc.

📒 Files selected for processing (12)
  • apps/desktop/src/app/DesktopObservability.test.ts
  • apps/desktop/src/app/DesktopObservability.ts
  • apps/server/src/cloud/relayTracing.ts
  • apps/server/src/config.ts
  • apps/server/src/server.test.ts
  • apps/server/src/serverLogger.test.ts
  • apps/web/src/observability/clientTracing.ts
  • docs/fork/0026-telemetry-says-which-app-sent-it.md
  • docs/fork/README.md
  • docs/operations/observability.md
  • packages/shared/src/observability.ts
  • packages/shared/src/relayTracing.ts

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


📝 Walkthrough

Walkthrough

Telemetry resources now use surface and management attributes, OpenTelemetry runtime attributes, and browser metadata. Tests and observability documentation reflect the updated attributes.

Changes

Telemetry resource attributes

Layer / File(s) Summary
Shared runtime and relay attributes
packages/shared/src/observability.ts, packages/shared/src/relayTracing.ts, apps/server/src/cloud/relayTracing.ts
A shared helper emits Node or Electron runtime attributes. Relay tracing uses updated runtime and component attribute keys, and server relay layers specify nodejs.
Desktop and web resource attributes
apps/desktop/src/app/DesktopObservability.ts, apps/web/src/observability/clientTracing.ts, apps/desktop/src/app/DesktopObservability.test.ts, apps/server/src/server.test.ts
Desktop resources use the configured deployment environment and runtime attributes. Web resources add the client surface, version, and available browser attributes. Tests check the updated resource fields.
Server resources and telemetry documentation
apps/server/src/config.ts, apps/server/src/serverLogger.test.ts, docs/operations/observability.md, docs/fork/0026-telemetry-says-which-app-sent-it.md, docs/fork/README.md
Server resources add the package version, management surface, and runtime attributes. Tests and documentation reflect the updated fields; the fork proposal and ledger record the changes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 88878

The documentation flags migration from the old telemetry keys, and no checked-in dashboard or query is shown to break. The PR appears ready for merge after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 88878

The existing telemetry destination appears unchanged, but browser traces will carry additional device and browser details. The privacy treatment of those details after export is not established, and consumers of the renamed attributes may need migration.

Retained concerns

  • Low · security · inferred: Browser tracing now automatically includes potentially identifying browser and device metadata in resource attributes sent through the existing server-to-collector path. Its downstream retention and privacy treatment are unverified.
Security review details

Security Blast Radius

  • inferred — The newly included browser fields can reach the configured collector whenever browser traces are exported. The reviewed change does not establish a new exporter destination or increased privilege.

Security Findings and Attack Paths

  • inferred — A client can influence its navigator-derived telemetry labels, but the evidence does not establish a new authorization bypass or privileged sink. The material change is automatic export of additional browser metadata.

Trust Boundaries and Controls

  • observed — The server forwarding test posts browser traces with a session cookie and confirms upstream forwarding. That test does not establish production authorization or collector privacy controls.

Hardening Proposals

  • proposed — Confirm that collector retention and access policies cover the added browser fields, and omit fields that are unnecessary for the intended telemetry queries.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the motivation and key problem, but it does not use the required What Changed, Why, and Checklist sections. It also omits the checklist items, although UI sections are not app… Add the required headings and describe the implementation under What Changed and the rationale under Why. Include the repository checklist, marking each applicable item. Remove the UI Changes section only if no UI changes apply.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 9 files. (3 skipped: 3 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: updating observability resource attributes to follow OpenTelemetry semantic conventions.
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: Description check

Explanation

The description explains the motivation and key problem, but it does not use the required What Changed, Why, and Checklist sections. It also omits the checklist items, although UI sections are not applicable.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 9 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

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

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:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant