[SpencerTorres]

Closes HDX-5951

Summary

HyperDX now instruments itself with the new ClickStack SDKs (@clickhouse/clickstack-browser, @clickhouse/clickstack-node), the same path users take when they migrate from @hyperdx/browser and @hyperdx/node-opentelemetry. Existing env vars keep working: HYPERDX_API_KEY, per-signal browser endpoints and NEXT_PUBLIC_OTEL_RESOURCE_ATTRIBUTES.

Changes

• API (packages/api/src/telemetry.ts, bin/hyperdx, utils/logger.ts, tasks/usageStats.ts): a -r preload replaces the old one and still starts nothing without a key. Pino and Express come from the bundled instrumentations. Usage stats get their own exporter, so HyperDX's ingestion key never reaches them.
• Browser (packages/app/instrumentation-client.ts, AppNav): setUser and setAttributes({ teamName }) replace setGlobalAttributes. Custom actions stay custom-action spans for the Sessions page, and logout starts a new session.
• Next.js server (packages/app/instrumentation.ts, Dockerfiles): moved to the package root so production builds load it, which they never did before. It reports as <service>-next and skips static assets. HDX_EXPORTER_ENABLED=false turns it off.
• Shared (packages/common-utils/src/selfTelemetry.ts): one tested helper decides whether the API and the Next server start the SDK.

Screenshots

Example self-instrumented data on ClickStack SDK

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 6, 2026 2:51pm UTC
hyperdx-storybook	Ready	Preview	Nov 6, 2026 2:51pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: 8739f80

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/api	Minor
@hyperdx/app	Minor
@hyperdx/otel-collector	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

E2E Test Results

✅ All tests passed • 463 passed • 1 skipped • 1389s

Status	Count
✅ Passed	463
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

• Background tasks or delivery pipeline substantially modified — 246 lines (bar: 30):
  • docker/hyperdx/Dockerfile
  • docker/otel-collector/config.standalone.yaml
  • packages/api/src/tasks/index.ts
  • packages/api/src/tasks/usageStats.ts
• Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 35
• Production lines changed: 983 (+ 897 in test files, excluded from tier calculation)
• Critical-path lines changed: 246
• Branch: sdk-upgrade
• Author: SpencerTorres

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

🟠 major — Usage-stats exporter still sends the operator's OTLP headers to in-otel.hyperdx.io

OTLPLogExporter merges OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_LOGS_HEADERS into the explicit headers. The explicit authorization replaces only that one key. Any other header the operator set for their own collector (for example x-honeycomb-team=<secret>, x-scope-orgid, or a vendor API-key header) is still sent to HyperDX's usage endpoint. The old SDK did the same, but this PR adds a dedicated exporter precisely to keep this path isolated. Fix it by not using the env-aware exporter: serialize the record with @opentelemetry/otlp-transformer (JsonLogsSerializer) and POST it with only the usage-stats key. The test mocks OTLPLogExporter, so it can't see the env merge. Add a test with OTEL_EXPORTER_OTLP_HEADERS set that checks the outgoing request headers.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — exitAfterShutdown isn't run-once or time-bounded, and the in-app cron keeps ticking while it waits

Before, process.exit ran right away. Now an uncaughtException or unhandledRejection leaves the process running until shutdown() settles, which can be up to the exporter/BSP timeouts against a black-holed collector. Meanwhile the CronJob started at line 80 can fire more ticks, and every further exception or rejection starts another shutdown(). A late rejection can also race exitAfterShutdown(0) from the one-shot path. Fix: guard with a module-level exiting flag, stop the cron job, and Promise.race the shutdown against a short timeout (for example 5 s) before calling process.exit.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

🔵 minor — SDK shutdown runs once per HTTP server, so the first server to finish stops telemetry while the other is still draining

gracefulShutdown is registered separately for appServer and opampServer (line 75), and each onShutdown now awaits shutdownTelemetry(). Whichever finishes first shuts down the SDK while the other server may still be draining requests (up to the 10 s timeout). Those requests' spans and logs are dropped, along with that server's own 'Server gracefully shut down...' line. Fix: shut telemetry down once, after both servers' shutdowns settle, for example with a shared promise built from Promise.allSettled over both, or a run-once wrapper.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/api/src/index.ts:403 — Second exit-after-telemetry helper with different semantics from tasks/index.ts → exitAfterFlush here and exitAfterShutdown in packages/api/src/tasks/index.ts solve the same problem (process.exit() skips the SDK's exit-time flush), but they behave differently. This one calls flush() and has no exiting guard, so repeated uncaught exceptions in dev start several flushes and several process.exit calls. The task version calls shutdown(), runs once, and keeps the first exit code. Move one guarded helper to packages/api/src/utils/ and use it in both entry points.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Deep Review

✅ No critical issues found. This SDK-migration PR (@hyperdx/* → @clickhouse/clickstack-* self-instrumentation) is well-tested and carefully commented. No P0/P1 issues survived re-grading: the reliability concerns all depend on unverified SDK internals (OTel exporters normally bound their own export timeout) and degrade telemetry data rather than app correctness, auth, or data integrity. Secret redaction, telemetry start-gating, and usage-stats key isolation were all verified intact. The items below are recommended hardening before or shortly after merge.

🟡 P2 — recommended

• packages/api/src/index.ts:13 — The start-failure and uncaughtException exit paths changed from a guaranteed process.exit(1) to void flush().finally(() => process.exit(code)), so a flush() that never settles (unreachable collector, no bounded timeout at the call site) weakens the deliberate "exit so the orchestrator restarts" guarantee the #2966 comment protects; the task runner's shutdown() at tasks/index.ts:76 has the same shape.
  • Fix: Race flush()/shutdown() against an unref'd timeout that forces process.exit(code) so a stalled exporter cannot defer exit.
  • _{adversarial, reliability}
• packages/api/src/server.ts:80 — runOnceAfterAll(servers.length, shutdownTelemetry) only runs the telemetry shutdown after both servers' onShutdown fire, so if one server's onShutdown stalls the drained server waits on it and the SDK flush is skipped when the 10s force-timeout fires, dropping buffered telemetry.
  • Fix: Decouple per-server exit or bound the shared barrier with an independent timer so one stalled server cannot block the other's telemetry flush.
  • _{adversarial, reliability}
• packages/app/src/components/AppNav/AppNav.tsx:223 — setUser/setAttributes fire when meData resolves, but browser SDK init() is gated behind the async fetch('/api/config') in instrumentation-client.ts; if identity wins that race and the SDK does not buffer pre-init identity calls, the user stays unattributed for the whole RUM session since the effect only re-runs on meData change.
  • Fix: Confirm the SDK buffers pre-init setUser/setAttributes, or re-apply identity once init() signals ready.
  • _{frontend-races}
• packages/api/src/tasks/index.ts:117 — The in-app cron task process installs uncaughtException/unhandledRejection handlers and exitAfterShutdown, but no SIGTERM/SIGINT handler, so a normal container stop terminates it without running the new flush-on-exit path and drops the task's own spans and duration metric.
  • Fix: Add SIGTERM/SIGINT handlers that route through exitAfterShutdown(0), unless the SDK already registers its own signal-time flush (verify).
  • _correctness
• packages/app/src/utils/nextServerTelemetry.ts:36 — serviceNameFromResourceAttributes is a second, intentionally stricter parser of the same OTEL_RESOURCE_ATTRIBUTES format already parsed by parseResourceAttributes in config.ts; the two are documented as "twins" but can silently drift, and a mismatch makes the Next server pick a different service.name than the SDK.
  • Fix: Extract one shared parser with strict/lenient modes so service.name selection cannot diverge from the SDK's EnvDetector.
  • _{maintainability, adversarial}
• packages/app/src/utils/nextServerTelemetry.ts:13 — The static-asset span suppression relies on a hand-maintained STATIC_PATH_PREFIXES/STATIC_PATHS/STATIC_PATH_SUFFIXES allowlist whose own comment notes "a new file in public/ needs an entry here," so a forgotten entry silently doubles ingested span volume with nothing in CI to catch it.
  • Fix: Derive the list from public/ contents at build time, or add a regression test asserting known static paths classify as static.
  • _{maintainability}
• packages/api/src/index.ts — The new exitAfterFlush flush-before-exit behavior (the point of this change) has no test, while the sibling exitAfterShutdown in tasks/index.ts got a full test file; there is no test module for this entry point at all.
  • Fix: Add a test that mocks flush and asserts the start-failure and dev uncaughtException paths await flush() then process.exit(1).
  • _testing
• .changeset/clickstack-sdks.md:1 — The changeset bumps @hyperdx/api and @hyperdx/app but not @hyperdx/common-utils, which now ships a new runtime module (dist/selfTelemetry) that @hyperdx/api imports at runtime; an external install resolving @hyperdx/common-utils@^0.33.0 would get a version lacking that module.
  • Fix: Add a @hyperdx/common-utils bump so the published api/app can resolve dist/selfTelemetry, after verifying whether api is consumed outside the Docker/workspace build.
  • _{project-standards}

🔵 P3 nitpicks (8)

• packages/common-utils/src/selfTelemetry.ts:32 — hasAuthorization matches a valueless authorization entry (e.g. a bare OTEL_EXPORTER_OTLP_HEADERS=authorization), so decideSelfTelemetry skips appending the key and telemetry starts but every export is rejected unauthenticated with no error surfaced.
  • Fix: Treat an authorization entry with an empty value after the first = as absent.
• packages/api/src/tasks/usageStats.ts:121 — The per-report LoggerProvider/BatchLogRecordProcessor is created lazily but never shut down, leaving its scheduled-delay timer running for the process lifetime (benign in the long-lived API; no leak across ticks since it is a module singleton).
  • Fix: Shut the provider down on server shutdown, or use a processor without a background timer.
• packages/api/src/middleware/error.ts:95 — Removing recordException means errors from the non-last handlers of a multi-handler router.use() (which the HTTP instrumentation does not wrap, per the code's own comment) are still logged but no longer recorded as span exception events.
  • Fix: Re-record the exception on the active span for the uninstrumented multi-handler case, or confirm losing those from error tracking is acceptable.
• packages/app/src/utils/browserTelemetry.ts:14 — recordAction uses the global trace API, so spans emitted before init() registers a provider become no-ops; the auto-run saved-search search executed emit in DBSearchPage.tsx can fire within the config-fetch window and be lost.
  • Fix: Defer or guard recordAction until init() has resolved if early actions must be captured.
• packages/app/src/components/AppNav/AppNav.components.tsx:182 — Logout runs setUser(null)/setAttributes(null)/startNewSession() synchronously in the onClick immediately before a full-page <Link> navigation, so any async flush/teardown those calls start races the unload and may not complete.
  • Fix: Verify the SDK flushes synchronously (or on pagehide); the startNewSession() here is also partly redundant since the full navigation re-inits the SDK.
• packages/api/src/telemetry.ts:23 — The if (telemetry.otlpHeaders !== undefined) process.env.OTEL_EXPORTER_OTLP_HEADERS = ... block is duplicated verbatim in telemetry.ts and nextServerTelemetry.ts:112.
  • Fix: Fold the env mutation into a shared applySelfTelemetryDecision helper next to decideSelfTelemetry.
• packages/api/src/tasks/index.ts:65 — exitAfterShutdown (task) and exitAfterFlush (index.ts:13) implement the same drain-then-exit concern with inconsistent robustness — only the task variant has the exiting re-entrancy guard.
  • Fix: Extract a shared exitAfter(drain, code) helper with the guard built in so both entry points get once-only exit semantics.
• packages/api/src/utils/runOnceAfterAll.ts:1 — The count=0 degenerate case (where fn would run on the first arrival) is untested; currently safe because callers always pass servers.length >= 1.
  • Fix: Add a count=0 test case or guard against count <= 0 in the implementation.

---

Reviewers (13): correctness, security, adversarial, reliability, testing, maintainability, kieran-typescript, frontend-races, project-standards, performance, previous-comments, learnings-researcher, agent-native.

Testing gaps:

• No end-to-end test asserts that log records exported through the SDK's pino instrumentation (not just pino's local stdout) have Authorization/cookie redacted — the off-box export path is the security-critical sink and is currently unverified.
• No test confirms the browser Resource ends with service.version = NEXT_PUBLIC_APP_VERSION when NEXT_PUBLIC_OTEL_RESOURCE_ATTRIBUTES also sets service.version (precedence now depends on SDK ordering).
• No test proves a hung or unreachable collector cannot wedge process.exit on the flush()/shutdown() exit paths.

Pre-existing (not introduced, not counted): the hardcoded usage-stats ingestion key in tasks/usageStats.ts was moved, not added; decideSelfTelemetry appending the API key to operator-set OTLP headers preserves prior SDK behavior with operator-controlled inputs.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — HDX_EXPORTER_ENABLED=false stops only the Next server, so the API and alert task keep exporting

The shared decideSelfTelemetry is described as the one place that decides whether the SDK starts, but the HDX_EXPORTER_ENABLED check sits outside it, in the Next-only path. In the single-container hyperdx/all-in-one image (docker/hyperdx/entry.prod.sh), setting HDX_EXPORTER_ENABLED=false together with HYPERDX_API_KEY turns off browser and Next telemetry, but packages/api/src/telemetry.ts still starts the SDK for the API and check-alerts. Fix: add an HDX_EXPORTER_ENABLED field to SelfTelemetryEnv and return { start: false } for it inside decideSelfTelemetry, so both entry points share one gate. If the API is meant to ignore the flag, say that explicitly in the changeset.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}