fix(core): mint the fallback external trace id per run - #4533
Conversation
Runs that carry no external trace context (schedules, task-to-task triggers) fall back to a trace id generated once in the TracingSDK constructor. With `experimental_processKeepAlive` the TracingSDK outlives the run, so every run on a warm process was exported to the external OTLP endpoint under that one id — merging unrelated runs into a single trace. This is the same warm-start hazard c043c4a fixed for the external context path, which read the context live but deliberately left the fallback captured at construction. Remint the fallback when the trace context manager's context object is reassigned, which is the run boundary. An empty configured id still means external export is off and is left alone rather than switched on. The test harness needed a fix too: `setGlobalManager` delegates to `registerGlobal`, which ignores a second registration, so every test after the first was mutating the first test's manager. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address review feedback on the per-run fallback. Giving each exporter wrapper its own FallbackExternalTraceId reintroduced the problem it was meant to fix, one signal down: before, every wrapper got the same generated string, so a run's spans and logs agreed. With per-wrapper state each one reminted independently, so from the second run on a warm process the logs carried a different trace id than the spans and stopped correlating. Construct one instance in the TracingSDK and pass it to every span and log wrapper. Also stop treating an empty trace context as a run boundary. The noop manager returns a fresh object on every call, so its identity always differs and would remint on every export batch, shattering one run's trace into many. Not reachable today (the wrappers are only built where a StandardTraceContextManager is registered) but the invariant was implicit. Rewrite the changeset for users per AGENTS.md, and format with oxfmt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Detecting the run boundary by reference-identity of the trace context was
a heuristic, and guarding it against the noop manager's fresh `{}` meant
testing the context for emptiness. That traded an unreachable bug for a
reachable one: `traceContext` is `z.record(z.unknown())`, so a run whose
context is empty would stop reminting and silently merge back into the
previous run's trace.
Replace the inference with a fact. `StandardTraceContextManager.traceContext`
becomes an accessor pair that advances an epoch whenever the context is
replaced, which is exactly what starting a run does, so no call site
changes. The noop manager reports a constant epoch, so with no manager
registered there are no boundaries to react to and nothing churns.
Drops the emptiness heuristic entirely, and covers the case it would have
broken. Also renames `get()` to `forCurrentRun()` and the shared instance
to `fallbackTraceId`, since the old name read as a string, and exports the
log wrapper so a test can prove a run's spans and logs stay on one id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: f837ccc The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
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 |
|
Thanks for your contribution! We require all external PRs to be opened in draft status first so you can address CodeRabbit review comments and ensure CI passes before requesting a review. Please re-open this PR as a draft. See CONTRIBUTING.md for details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe trace context API now exposes a context replacement epoch. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| set traceContext(value: Record<string, unknown>) { | ||
| this.#traceContext = value; | ||
| this.#epoch++; | ||
| } |
There was a problem hiding this comment.
🔍 Epoch bumps per execution message, so retry attempts of one run get different fallback trace ids
The run boundary is defined as "the trace context object was replaced". In the workers, standardTraceContextManager.traceContext = traceContext runs once per EXECUTE_TASK_RUN message (packages/cli-v3/src/entryPoints/managed-run-worker.ts:403, packages/cli-v3/src/entryPoints/dev-run-worker.ts:426), and reset() also assigns (packages/core/src/v3/traceContext/manager.ts:31), so the epoch advances at least twice per execution message. Since each retry attempt of the same run arrives as its own EXECUTE_TASK_RUN, a run that retries on the same warm process will mint a new fallback external trace id per attempt, i.e. the granularity is per-attempt rather than per-run (the changeset text says "Each run now appears as its own trace"). This is consistent with the cold-start behaviour (a fresh process always generated a fresh id), so it is not a regression, but it's worth confirming the intended granularity — the runs whose attempts land on the same warm process will no longer be grouped, and there is currently no signal in the trace context to distinguish "new attempt of the same run" from "new run".
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Problem
Runs that carry no external trace context (schedules, task-to-task triggers, anything not started from an incoming
traceparent) fall back to a generated external trace id. That id is generated once, in theTracingSDKconstructor:https://github.com/triggerdotdev/trigger.dev/blob/main/packages/core/src/v3/otel/tracingSDK.ts#L165
With
experimental_processKeepAliveenabled, theTracingSDKoutlives the run, so every run that executes on a warm process is exported to the external OTLP endpoint under that same trace id and unrelated runs get merged into one trace on the receiving backend.This is the same warm-start hazard that c043c4a fixed for the external-context path. That commit made the wrappers read
traceContext.getExternalTraceContext()live instead of capturing it at construction, but deliberately left the fallback captured, so the bug survives for exactly the runs that have no external context.What it looks like in production
We export to a self-hosted Langfuse via
telemetry.exporters. Measured over our production traces:Per-trace cost and latency attribution is meaningless as a result: a trace shows an unrelated mix of workloads, and drilling into one run is impossible.
Disabling
experimental_processKeepAliveavoids it, but that is a significant throughput regression and not a real option for us.Fix
FallbackExternalTraceIdholds the generated id and remints it when the run changes. TheTracingSDKconstructs one instance and passes it to everyExternalSpanExporterWrapperandExternalLogRecordExporterWrapper, so a run's spans and logs agree on the id after a remint. That matches the old behaviour, where all wrappers received one identical string.The run boundary comes from the manager rather than being inferred.
StandardTraceContextManager.traceContextbecomes an accessor pair that advances an epoch whenever the context is replaced, which is exactly what starting a run does, so no call site changes.getTraceContextEpoch()is added toTraceContextManager, and the noop manager reports a constant, so with no manager registered there are no boundaries to react to.I did first try inferring the boundary from reference-identity of
getTraceContext(). It works, but guarding it against the noop manager's freshly allocated{}requires testing the context for emptiness, and sincetraceContextisz.record(z.unknown())that silently stops reminting for a run whose context is legitimately empty, merging it back into the previous run's trace. The epoch avoids the heuristic entirely.One behaviour held deliberately: an empty configured id still means external export is off, so the empty seed short-circuits rather than minting an id and switching the feature on for a deployment that never asked for it.
Tests
packages/core/test/externalSpanExporterWrapper.test.tsgains six cases:Mutation-checked. Removing the epoch bump fails three of them, and reinstating the emptiness heuristic fails the empty-context case with the exact symptom it describes. Full
packages/coresuite passes (674 tests).The harness needed one fix to make these meaningful.
traceContext.setGlobalManager()delegates toregisterGlobal, which ignores a second registration, so thebeforeEachonly ever installed the first test's manager and every later test was mutating an object that was no longer global. CallingtraceContext.disable()first makes each test's manager actually take effect.Happy to split the
traceContextepoch into its own commit or PR if you'd rather review it separately, or to take a different approach to the boundary entirely.🤖 Generated with Claude Code
Supersedes #4526, which the vouching bot auto-closed before I was on the list and which GitHub will no longer let me reopen (its head is stuck at the first commit). Devin's three findings on that PR are addressed here; see the notes on the closed PR and the
Fixsection above.