-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(core): mint the fallback external trace id per run #4526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -393,21 +393,67 @@ function setLogLevel(level: TracingDiagnosticLogLevel) { | |
| diag.setLogger(new DiagConsoleLogger(), diagLogLevel); | ||
| } | ||
|
|
||
| /** | ||
| * The external trace id used by runs that carry no external trace context, | ||
| * minted once per run. | ||
| * | ||
| * It has to change per run for the same reason the wrappers read the external | ||
| * context live: with `processKeepAlive` the `TracingSDK` — and so the wrappers | ||
| * — outlive the run, so an id captured at construction merges every run on the | ||
| * process into one trace. The manager's trace context object is reassigned per | ||
| * run, which makes its identity the run boundary. | ||
| */ | ||
| class FallbackExternalTraceId { | ||
| private traceId: string; | ||
| private seenTraceContext: unknown; | ||
|
|
||
| constructor( | ||
| private seed: string, | ||
| private traceIdGenerator: Pick<RandomIdGenerator, "generateTraceId"> = idGenerator | ||
| ) { | ||
| this.traceId = seed; | ||
| this.seenTraceContext = traceContext.getTraceContext(); | ||
| } | ||
|
|
||
| get(): string { | ||
| // An empty seed means external export is disabled — leave it that way | ||
| // rather than minting an id and switching the feature on. | ||
| if (!this.seed) { | ||
| return this.seed; | ||
| } | ||
|
|
||
| const currentTraceContext = traceContext.getTraceContext(); | ||
|
|
||
| if (currentTraceContext !== this.seenTraceContext) { | ||
| this.seenTraceContext = currentTraceContext; | ||
| this.traceId = this.traceIdGenerator.generateTraceId(); | ||
| } | ||
|
Comment on lines
+425
to
+430
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Run-boundary detection degrades to "every export" when no trace context manager is registered The remint trigger is reference-identity of Today this is not reachable in production: the only Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| return this.traceId; | ||
| } | ||
| } | ||
|
|
||
| export class ExternalSpanExporterWrapper { | ||
| private fallback: FallbackExternalTraceId; | ||
|
|
||
| constructor( | ||
| private underlyingExporter: SpanExporter, | ||
| private externalTraceId: string | ||
| ) {} | ||
| externalTraceId: string, | ||
| traceIdGenerator?: Pick<RandomIdGenerator, "generateTraceId"> | ||
| ) { | ||
| this.fallback = new FallbackExternalTraceId(externalTraceId, traceIdGenerator); | ||
| } | ||
|
|
||
| private transformSpan(span: ReadableSpan): ReadableSpan | undefined { | ||
| // Read external context live, so per-run reassignment of | ||
| // standardTraceContextManager.traceContext is honoured on warm-started | ||
| // workers that reuse a single TracingSDK across runs. | ||
| const externalTraceContext = traceContext.getExternalTraceContext(); | ||
| const fallbackTraceId = this.fallback.get(); | ||
|
|
||
| const isExternallySampled = externalTraceContext | ||
| ? isTraceFlagSampled(externalTraceContext.traceFlags) | ||
| : !!this.externalTraceId; | ||
| : !!fallbackTraceId; | ||
|
|
||
| if (!isExternallySampled) { | ||
| return; | ||
|
|
@@ -419,7 +465,7 @@ export class ExternalSpanExporterWrapper { | |
|
|
||
| const externalTraceId = externalTraceContext | ||
| ? externalTraceContext.traceId | ||
| : this.externalTraceId; | ||
| : fallbackTraceId; | ||
|
|
||
| const isAttemptSpan = span.attributes[SemanticInternalAttributes.SPAN_ATTEMPT]; | ||
|
|
||
|
|
@@ -478,25 +524,33 @@ export class ExternalSpanExporterWrapper { | |
| } | ||
|
|
||
| class ExternalLogRecordExporterWrapper { | ||
| private fallback: FallbackExternalTraceId; | ||
|
|
||
| constructor( | ||
| private underlyingExporter: LogRecordExporter, | ||
| private externalTraceId: string | ||
| ) {} | ||
| externalTraceId: string, | ||
| traceIdGenerator?: Pick<RandomIdGenerator, "generateTraceId"> | ||
| ) { | ||
| this.fallback = new FallbackExternalTraceId(externalTraceId, traceIdGenerator); | ||
| } | ||
|
Comment on lines
+527
to
+535
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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 ( 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 runBefore this change, Now each wrapper constructs its own A shared Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| export(logs: any[], resultCallback: (result: any) => void): void { | ||
| const externalTraceContext = traceContext.getExternalTraceContext(); | ||
| const fallbackTraceId = this.fallback.get(); | ||
|
|
||
| const isExternallySampled = externalTraceContext | ||
| ? isTraceFlagSampled(externalTraceContext.traceFlags) | ||
| : !!this.externalTraceId; | ||
| : !!fallbackTraceId; | ||
|
|
||
| if (!isExternallySampled) { | ||
| this.underlyingExporter.export([], resultCallback); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const modifiedLogs = logs.map((log) => this.transformLogRecord(log, externalTraceContext)); | ||
| const modifiedLogs = logs.map((log) => | ||
| this.transformLogRecord(log, externalTraceContext, fallbackTraceId) | ||
| ); | ||
|
|
||
| this.underlyingExporter.export(modifiedLogs, resultCallback); | ||
| } | ||
|
|
@@ -517,13 +571,13 @@ class ExternalLogRecordExporterWrapper { | |
| logRecord: ReadableLogRecord, | ||
| externalTraceContext: | ||
| | { traceId: string; spanId: string; tracestate?: string; traceFlags: number } | ||
| | undefined | ||
| | undefined, | ||
| fallbackTraceId: string | ||
| ): ReadableLogRecord { | ||
| // Capture externalTraceId for use within the proxy's scope. | ||
| // Use externalTraceContext.traceId if available, otherwise fall back to generated externalTraceId | ||
| const externalTraceId = externalTraceContext | ||
| ? externalTraceContext.traceId | ||
| : this.externalTraceId; | ||
| // Use externalTraceContext.traceId if available, otherwise fall back to the | ||
| // per-run generated id. | ||
| const externalTraceId = externalTraceContext ? externalTraceContext.traceId : fallbackTraceId; | ||
|
|
||
| // If there's no spanContext, or if the externalTraceId is not set, return the original logRecord. | ||
| if (!logRecord.spanContext || !externalTraceId) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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 perTracingSDK"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."
Was this helpful? React with 👍 or 👎 to provide feedback.