-
Notifications
You must be signed in to change notification settings - Fork 39
fix: close any existing active span when extracting the tracecontext #688
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
base: main
Are you sure you want to change the base?
Changes from all commits
2a2f475
7cc12a1
f5cd928
2aef2e7
cb3a3d6
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 |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { logDebug } from "../utils"; | ||
| import { logDebug, logWarning } from "../utils"; | ||
| import { SpanContextWrapper } from "./span-context-wrapper"; | ||
| import { TraceSource } from "./trace-context-service"; | ||
|
|
||
|
|
@@ -94,6 +94,20 @@ export class TracerWrapper { | |
| return new SpanContextWrapper(span.context(), TraceSource.DdTrace); | ||
| } | ||
|
|
||
| public closeScope(): void { | ||
| try { | ||
| const activeSpan = this.currentSpan; | ||
|
Contributor
Author
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. this.currentSpan will check |
||
| if (activeSpan && typeof activeSpan.finish === "function") { | ||
| logDebug("Detected stale span from previous invocation, finishing it to prevent trace context leakage"); | ||
|
Contributor
Author
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. Honestly, I was thinking this should be 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. |
||
| activeSpan.finish(); | ||
|
Comment on lines
+99
to
+102
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.
The new Useful? React with 👍 / 👎.
Contributor
Author
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. 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. |
||
| } | ||
| } catch (err) { | ||
| if (err instanceof Object || err instanceof Error) { | ||
| logDebug("Failed to close dd-trace scope", err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public injectSpan(span: SpanContext): any { | ||
| const dest = {}; | ||
| this.tracer.inject(span, "text_map", dest); | ||
|
|
||
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.
this is the fix that codex missed.