fix(core): mint the fallback external trace id per run - #4526
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>
🦋 Changeset detectedLatest commit: 75f95c9 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 |
|
Hi @NERLOE, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more 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 (3)
WalkthroughFallback external trace IDs now generate once per run instead of once per ✨ 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}
| private fallback: FallbackExternalTraceId; | ||
|
|
||
| constructor( | ||
| private underlyingExporter: LogRecordExporter, | ||
| private externalTraceId: string | ||
| ) {} | ||
| externalTraceId: string, | ||
| traceIdGenerator?: Pick<RandomIdGenerator, "generateTraceId"> | ||
| ) { | ||
| this.fallback = new FallbackExternalTraceId(externalTraceId, traceIdGenerator); | ||
| } |
There was a problem hiding this comment.
🔴 Logs and traces sent to external observability tools stop lining up for runs without incoming trace context
Each exporter builds its own private generator of the substitute trace id (new FallbackExternalTraceId(...) at packages/core/src/v3/otel/tracingSDK.ts:534) instead of sharing one, so from the second run onwards on a reused process the logs and the spans of the same run are stamped with different, randomly generated ids.
Impact: In the customer's own observability backend, a run's logs are no longer attached to that run's trace, so they appear orphaned and undiscoverable.
Why the two wrappers diverge after the first run
Before this change, TracingSDK generated one externalTraceId string (packages/core/src/v3/otel/tracingSDK.ts:165) and passed the same string to every ExternalSpanExporterWrapper (packages/core/src/v3/otel/tracingSDK.ts:170,182) and every ExternalLogRecordExporterWrapper (packages/core/src/v3/otel/tracingSDK.ts:234,249). Spans and logs therefore always agreed on the fallback trace id.
Now each wrapper constructs its own FallbackExternalTraceId holding independent state (packages/core/src/v3/otel/tracingSDK.ts:444 and :534). On the first run all instances still return the shared seed, but as soon as traceContext.getTraceContext() identity changes (a new run), each instance independently calls traceIdGenerator.generateTraceId() (packages/core/src/v3/otel/tracingSDK.ts:429), producing a different random id per wrapper. The same divergence occurs when a user configures more than one entry in telemetry.exporters.
A shared FallbackExternalTraceId instance created once in the TracingSDK constructor and passed to all wrappers would keep the per-run remint while preserving cross-signal correlation.
Prompt for agents
In packages/core/src/v3/otel/tracingSDK.ts, the new FallbackExternalTraceId is instantiated separately inside ExternalSpanExporterWrapper and ExternalLogRecordExporterWrapper. Previously all wrappers received one identical externalTraceId string generated once in the TracingSDK constructor, which guaranteed that spans and logs of a run without external trace context shared the same external trace id. With per-wrapper instances, the first remint (second run on a warm process) makes each wrapper generate its own random trace id, so a run's logs and spans no longer correlate in the external backend; multiple configured span exporters diverge too. Consider constructing a single FallbackExternalTraceId in the TracingSDK constructor (seeded with the generated id) and injecting that shared instance into every ExternalSpanExporterWrapper and ExternalLogRecordExporterWrapper, keeping the existing per-run remint semantics and the tests' ability to inject a fake id generator.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "@trigger.dev/core": patch | ||
| --- | ||
|
|
||
| Mint the fallback external trace id per run rather than once per `TracingSDK`. Runs that carry no external trace context fall back to a generated trace id, and with `experimental_processKeepAlive` the `TracingSDK` outlives the run — so every run on a warm process was exported to the external OTLP endpoint under one shared trace id, merging unrelated runs into a single trace. |
There was a problem hiding this comment.
🟡 Release note text describes internals instead of user-visible behaviour
The release note added for this change leads with implementation detail and names an internal component ("Mint the fallback external trace id per run rather than once per TracingSDK" in .changeset/external-trace-id-per-run.md:5), which the repository guidelines forbid for user-facing notes.
Impact: Users reading the release notes see internal jargon rather than a plain description of what changed for them.
Rule reference
AGENTS.md, section "Changesets and Server Changes": "Write the description for users, not maintainers. ... Lead with what changed for the user - one plain sentence describing behavior, not implementation, and never naming internal tools or infra."
| Mint the fallback external trace id per run rather than once per `TracingSDK`. Runs that carry no external trace context fall back to a generated trace id, and with `experimental_processKeepAlive` the `TracingSDK` outlives the run — so every run on a warm process was exported to the external OTLP endpoint under one shared trace id, merging unrelated runs into a single trace. | |
| Runs that don't start from an incoming trace are no longer merged together when they run on the same warm worker process — each run now gets its own trace in your own observability tool. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const currentTraceContext = traceContext.getTraceContext(); | ||
|
|
||
| if (currentTraceContext !== this.seenTraceContext) { | ||
| this.seenTraceContext = currentTraceContext; | ||
| this.traceId = this.traceIdGenerator.generateTraceId(); | ||
| } |
There was a problem hiding this comment.
🔍 Run-boundary detection degrades to "every export" when no trace context manager is registered
The remint trigger is reference-identity of traceContext.getTraceContext(). When no manager has been registered, the API falls back to NoopTraceContextManager, whose getTraceContext() returns a freshly allocated {} on every call (packages/core/src/v3/traceContext/index.ts), so currentTraceContext !== this.seenTraceContext is always true and a brand-new fallback trace id is minted on every single export batch — shattering one logical trace into many.
Today this is not reachable in production: the only TracingSDK instances that receive exporters/logExporters are in packages/cli-v3/src/entryPoints/managed-run-worker.ts:215 and dev-run-worker.ts:243, both of which register StandardTraceContextManager beforehand; the index workers (managed-index-worker.ts:95, dev-index-worker.ts:101) construct TracingSDK without external exporters, so no wrapper is created. Still, the invariant "a manager is always registered" is implicit and unguarded — a cheap defence would be to treat an empty/noop context as "no change".
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