Skip to content

refactor(observability): name each service after its application - #13699

Merged
juliusmarminge merged 6 commits into
pingdotgg:mainfrom
yordis:yordis/refactor-static-otlp-service-name
Sep 26, 2026
Merged

juliusmarminge merged 6 commits into
pingdotgg:mainfrom
yordis:yordis/refactor-static-otlp-service-name

Conversation

@yordis

@yordis yordis commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor
  • A service name that differs per install or per code path splits one application across several services in a collector, so every dashboard and query has to know each name someone picked.
  • Environment, version, release channel, and which part of the app emitted a span are dimensions, and OTEL_RESOURCE_ATTRIBUTES already carries them, for example through the stable deployment.environment.name convention. A configurable name adds nothing they cannot express.
  • A shared t3code- prefix and the t3code service.namespace keep T3 Code's services grouped and distinguishable in a collector that also receives other applications, whether a backend groups by name or by namespace.
  • Anyone setting T3CODE_OTLP_SERVICE_NAME or querying the old names has to switch, which is cheaper now than after more dashboards depend on them.

Summary by CodeRabbit

  • Improvements
    • Standardized telemetry service names across desktop, mobile, web, server, and relay components, with services grouped under the t3code namespace.
    • Deployment environment attributes remain available to help distinguish environments when viewing telemetry.
  • Configuration Changes
    • Service names are now fixed rather than configurable through T3CODE_OTLP_SERVICE_NAME. OTEL_SERVICE_NAME and service name or namespace values in OTEL_RESOURCE_ATTRIBUTES no longer override them.
  • Documentation
    • Updated observability setup and Tempo guidance to reflect the fixed service names and environment filtering.

…plication

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:M 30-99 changed lines (additions + deletions). labels Sep 25, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes the default telemetry identity across server, desktop, web, mobile, and relay paths and removes the server’s configurable service-name setting. Existing telemetry queries and installations may need updates, so the default-change and cross-cutting runtime impact warrant human review.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

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

Comment thread apps/server/src/config.ts Outdated
Comment thread apps/desktop/src/app/DesktopObservability.ts Outdated
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis changed the title refactor(observability): make the OTLP service name a constant per application refactor(observability): name each service after its application Sep 25, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Comment thread apps/server/src/cloud/relayTracing.ts
Comment thread apps/mobile/src/features/observability/tracing.ts
Comment thread apps/web/src/lib/runtime.ts
Comment thread infra/relay/src/observability.ts
Comment thread apps/server/src/cloud/relayTracing.ts
Comment thread infra/relay/src/observability.ts
Comment thread packages/shared/src/relayTracing.ts
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 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: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c93148ce-545d-46e2-81e4-1957ddaf4677

📥 Commits

Reviewing files that changed from the base of the PR and between 2598ce7 and 91dc7f4.

📒 Files selected for processing (20)
  • apps/desktop/src/app/DesktopObservability.test.ts
  • apps/desktop/src/app/DesktopObservability.ts
  • apps/mobile/src/features/observability/tracing.ts
  • apps/server/src/bin.test.ts
  • apps/server/src/cli/config.test.ts
  • apps/server/src/cli/config.ts
  • apps/server/src/cli/pair.ts
  • apps/server/src/cloud/relayTracing.ts
  • apps/server/src/config.ts
  • apps/server/src/environment/ServerEnvironment.test.ts
  • apps/server/src/persistence/Layers/Sqlite.ts
  • apps/server/src/server.test.ts
  • apps/server/src/serverLogger.test.ts
  • apps/web/src/lib/runtime.ts
  • apps/web/src/observability/clientTracing.ts
  • docs/operations/observability.md
  • infra/relay/src/observability.test.ts
  • infra/relay/src/observability.ts
  • packages/shared/src/relayTracing.test.ts
  • packages/shared/src/relayTracing.ts
💤 Files with no reviewable changes (5)
  • apps/server/src/cli/config.test.ts
  • apps/server/src/cli/pair.ts
  • apps/server/src/environment/ServerEnvironment.test.ts
  • apps/server/src/bin.test.ts
  • apps/server/src/cli/config.ts

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


📝 Walkthrough

Walkthrough

Telemetry resources now use fixed service names across server, desktop, mobile, web, and relay components. The server configuration no longer accepts a configurable OTLP service name. Tests and observability documentation reflect the updated names and resource attributes.

Changes

OTLP service names

Layer / File(s) Summary
Fix server service identity
apps/server/src/cli/*, apps/server/src/config.ts, apps/server/src/cloud/relayTracing.ts, apps/server/src/persistence/Layers/Sqlite.ts, apps/server/src/*.test.ts, apps/server/src/environment/ServerEnvironment.test.ts
Server configuration removes otlpServiceName. Server tracing uses t3code-server; its resource retains the t3code namespace. Tests cover exported server logs and updated browser OTLP expectations.
Set app service names and document them
apps/desktop/src/app/DesktopObservability*, apps/mobile/src/features/observability/tracing.ts, apps/web/src/observability/clientTracing.ts, apps/web/src/lib/runtime.ts, apps/server/src/server.test.ts, docs/operations/observability.md
Desktop, mobile, and web tracing use updated service names. Desktop and web resources add the t3code namespace. The guide documents fixed names and deployment environment attributes; browser OTLP tests expect t3code-web.
Set relay resource identities
infra/relay/src/observability*, packages/shared/src/relayTracing*
Relay resources use updated service names and namespace attributes. Export tests assert the resource attributes.

Priority: ⬇️ Low

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

Change: Refactor

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 91dc7

The fixed service identities appear consistent across the reviewed components. No identified issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 91dc7

Existing telemetry queries and alerts may need to switch to the new service names during rollout. The reviewed changes do not show a new telemetry destination or a weakened export control, but collector-side rules were not available to check.

Retained concerns

  • Low · reliability · inferred: Queries or alerts still selecting the old service names could miss telemetry during a staggered rollout. Whether any security-critical monitoring uses those selectors is unverified.
Security review details

Security Blast Radius

  • inferred — The changed identities can affect collector views spanning server, client, and relay telemetry, but the reviewed resource changes do not establish expanded application privileges or a new export destination.

Trust Boundaries and Controls

  • observed — The reviewed desktop and web exporter paths keep destination selection separate from their changed resource names. The relay exporter uses its supplied traces endpoint and ingest token; collector-side treatment of the new identity remains unknown.

Resilience and Maintainability Implications

  • inferred — If security monitoring filters on service identity, migrating those filters alongside the emitters matters to monitoring continuity. No such deployed filter was available to verify.

Hardening Proposals

  • proposed — For installations with service-name-based alerts or access policies, check their selectors against both identities during rollout and verify the resulting telemetry before retiring old selectors.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
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 2 functions across 14 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the rationale and migration impact, but it does not follow the required template. It omits the "What Changed" heading, the checklist, and explicit confirmation that UI changes… Rewrite the description using the required sections: "## What Changed", "## Why", and "## Checklist". Summarize the implementation changes under "What Changed", retain the rationale under "Why", state that UI changes are not applicable or r…
✅ Passed checks (3 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the primary change: replacing varied observability service names with stable application-based names.
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 2 functions across 14 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description explains the rationale and migration impact, but it does not follow the required template. It omits the "What Changed" heading, the checklist, and explicit confirmation that UI changes are not applicable.

Resolution

Rewrite the description using the required sections: "## What Changed", "## Why", and "## Checklist". Summarize the implementation changes under "What Changed", retain the rationale under "Why", state that UI changes are not applicable or remove that section, and complete the checklist.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@juliusmarminge
juliusmarminge merged commit 2a9832b into pingdotgg:main Sep 26, 2026
23 checks passed
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 26, 2026
## What's Changed
* feat(desktop): honor the standard OTLP variables in the main process and WSL backends by @yordis in pingdotgg/t3code#13641
* fix(web): restore compact provider instance badges by @t3-code[bot] in pingdotgg/t3code#13700
* perf(mobile): lighter Home rows away from the viewport by @AKolenda in pingdotgg/t3code#13702
* fix(usage): price Cursor cache savings by base model by @Yash-Singh1 in pingdotgg/t3code#13731
* perf(mobile): render Home rows further ahead while scrolling by @AKolenda in pingdotgg/t3code#13705
* refactor(observability): name each service after its application by @yordis in pingdotgg/t3code#13699
* perf(server): stop remapping every thread on each thread event by @t3dotgg in pingdotgg/t3code#13720
* Remove unused items tracking from Claude adapter state by @t3dotgg in pingdotgg/t3code#13718
* feat(observability): name the command on subprocess spans by @t3dotgg in pingdotgg/t3code#13701
* fix(cli): t3 triage points agents at log files that exist by @t3dotgg in pingdotgg/t3code#13685
* fix(server): the SQLite WAL file shrinks back after large writes by @t3dotgg in pingdotgg/t3code#13684
* feat(cli): summarize the server trace file from the command line by @t3dotgg in pingdotgg/t3code#13698
* perf(server): pull request sync reads only threads with linked pull requests by @t3dotgg in pingdotgg/t3code#13704

## New Contributors
* @AKolenda made their first contribution in pingdotgg/t3code#13702

**Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260925.2269...v0.0.43-nightly.20260926.2282

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260926.2282
@yordis
yordis deleted the yordis/refactor-static-otlp-service-name branch September 26, 2026 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). 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.

2 participants