fix: close any existing active span when extracting the tracecontext #688
Conversation
|
|
||
| public closeScope(): void { | ||
| try { | ||
| const activeSpan = this.currentSpan; |
There was a problem hiding this comment.
this.currentSpan will check
if (!this.isTracerAvailable) {
return;
}
| try { | ||
| const activeSpan = this.currentSpan; | ||
| if (activeSpan && typeof activeSpan.finish === "function") { | ||
| logDebug("Detected stale span from previous invocation, finishing it to prevent trace context leakage"); |
There was a problem hiding this comment.
Honestly, I was thinking this should be logWarning because this is not expected.
But the customer is actively running into this issue and we simply cannot reproduce, warning logs will spam their logs right away.
Since we are still actively working with them. My proposal is that we keep it at DEBUG level for this release => then check with the customer to understand which span was that leaked span => have a good reproduction case and fix it => update this to WARNING level.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const activeSpan = this.currentSpan; | ||
| if (activeSpan && typeof activeSpan.finish === "function") { | ||
| logDebug("Detected stale span from previous invocation, finishing it to prevent trace context leakage"); | ||
| activeSpan.finish(); |
There was a problem hiding this comment.
The new closeScope() only calls finish() on whatever span is currently active, but it never deactivates or clears that span from the tracer’s scope. TracerWrapper.traceContext() still returns the active span unconditionally, and TraceContextService.currentTraceContext (src/trace/trace-context-service.ts:73-79) always prefers that dd-trace span when injecting headers. In the warm-start scenario this change is trying to fix—an unfinished span left active from a prior invocation—closeScope() will finish the span but leave it active, so currentTraceHeaders will still emit the stale trace IDs. Deactivating the scope or skipping finished spans is needed to prevent the leakage the commit is addressing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Now the extract will return the extracted context and won't use the current context. later the extracted context will be used to create the inferred span or the lambda span and resetting the tracecontext there. Here the finish() is really trying to finish the still active span.
|
|
||
| return this.currentTraceContext; | ||
| // Return the extracted context, not the current context which may not be related to the event or context | ||
| return this.rootTraceContext; |
There was a problem hiding this comment.
this is the fix that codex missed.
Motivation
https://datadoghq.atlassian.net/browse/APMS-17080
TLDR of this ticket: Based on the customer's debug log, we see that the
currentTraceContextis coming from an active span from previous invocation, even though the extraction of trace context worked.also.. previous PR didn't fix the issue as the tracecontext was not cached as
rootTraceContext.What does this PR do?
rootTraceContextandcurrentTraceContext.rootTraceContextshould come from the lambda invocation'seventandcontextcurrentTraceContextcan come from the customers' manual instrumentations.trace-context-service.extractrootTraceContextshould not use any stale trace context from previous invocations at all.currentTraceContextshould not be set at this point and if detected, we'll try tocloseScope(This is the customer's issue)Types of Changes
Check all that apply