fix(internal/orchestrion): fix span parenting with GSL#4528
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: a58ca9a | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
|
Reverting to draft because it needs the PR in Orchestrion to be merged. |
BenchmarksBenchmark execution time: 2026-03-20 09:32:04 Comparing candidate commit a58ca9a in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 214 metrics, 8 unstable metrics.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba4ce9212
ℹ️ 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".
rarguelloF
left a comment
There was a problem hiding this comment.
(not 100% sure of the changes to the graphql-go/graphql integration)
…anHierarchy to reflect the current behaviour
…ion but without it
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 749f0e3759
ℹ️ 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".
| var __dd_span_ctx context.Context = ctx | ||
| {{- else -}} | ||
| {{- $ctx = "ctx" -}} | ||
| var ctx context.Context = {{ $impl }} |
There was a problem hiding this comment.
Guard nil context-like args before interface cast
The new ArgumentThatImplements "context.Context" path unconditionally casts a concrete argument into a context.Context variable. If the concrete argument is a nil pointer (for example *CustomContext(nil)), this produces a non-nil interface value, so StartSpanFromContext treats it as valid and eventually calls Value on it, which can panic for embedded-context wrappers before the function body runs. Previously these signatures fell back to context.TODO(), so this change introduces a crash path for nil custom context inputs.
Useful? React with 👍 / 👎.
20c3dd9 to
b7b6c99
Compare
GLS propagates the active span through goroutine-local storage even when context.TODO() is used, so spanWithNilNamedCtx and spanWithNilOtherCtx end up as children of test.root rather than separate root traces. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
This PR fixes three independent bugs where the wrong span was used as the parent. Relates to DataDog/orchestrion#798. Orchestrion instruments Go code by injecting span creation logic and using goroutine-local storage (GLS) as a side-channel to propagate the active span through call sites that don't have a `context.Context` in their signature. Spans are stored both in the `context.Context` chain (explicit) and in the GLS stack (implicit). `glsContext` wraps a `context.Context` and intercepts `.Value()` calls to consult both sources. The repro project used to replicate the issue described in the JIRA ticket implements a `CustomContext` type that embeds `context.Context` and adds a `UserID` field, mimicking how frameworks like Fiber wrap request contexts: ```go type CustomContext struct { context.Context UserID string } //dd:span func DoThingWithCtx(ctx context.Context) error { return nil } //dd:span func DoThingWithCustomCtx(ctx *CustomContext) error { return nil } ``` `theOperation` (excluded from instrumentation with `//orchestrion:ignore`) does the following: ```go // Creates span1 explicitly, wraps it in a CustomContext span1, nativeCtx := tracer.StartSpanFromContext(context.Background(), "theOperation") customCtx := &CustomContext{nativeCtx, "user-id"} DoThingWithCtx(customCtx) // SPAN 0 — should be child of span1 DoThingWithCustomCtx(customCtx) // SPAN 1 — should be child of span1 // Creates span2 with context.Background() — should be a root span span2, _ := tracer.StartSpanFromContext(context.Background(), "theOperation2") // SPAN 4 // customCtx still carries span1 — both calls should still be children of span1 DoThingWithCustomCtx(customCtx) // SPAN 2 — should be child of span1 DoThingWithCtx(customCtx) // SPAN 3 — should be child of span1 ``` **Observed output (before fixes):** ``` SPAN 0 [ DoThingWithCtx ] parent=span1 ✓ correct SPAN 1 [ DoThingWithCustomCtx ] parent=span1 ✓ correct (coincidence — GLS has span1 at this point) SPAN 2 [ DoThingWithCustomCtx ] parent=span2 ✗ wrong (should be span1) SPAN 3 [ DoThingWithCtx ] parent=span2 ✗ wrong (should be span1) SPAN 4 [ theOperation2 ] parent=span1 ✗ wrong (should be root) SPAN 5 [ theOperation ] parent=0 ✓ correct ``` **How each bug manifests:** **SPANs 2 & 3** — After `span2` is created, GLS holds `span2` as the active span. The second calls to both functions receive `customCtx` which carries `span1` in its context chain, but GLS takes priority, so `span2` becomes the parent. This is **Bug 2** (GLS priority). Additionally, **SPAN 1** appearing correct is coincidental: without **Bug 3** fixed, `DoThingWithCustomCtx` can't extract a context from `*CustomContext` and falls back to `context.TODO()`. At that point GLS holds `span1`, which happens to be the right answer. After `span2` is created, the same fallback to GLS explains why SPAN 2 is wrong: `context.TODO()` has no span, GLS returns `span2`. **SPAN 4** — `theOperation2` is created with `context.Background()`. Since Background has no values, the GLS fallback kicks in and returns the active `span1`, making it appear as a child of span1 instead of a root span. This is **Bug 1** (`context.Background()` sentinel). --- **Symptom**: A span started with `context.Background()` (explicit intent: no parent) would incorrectly inherit the active GLS span as its parent. **Root cause**: `SpanFromContext(context.Background())` would call `WrapContext(context.Background())`, producing a `glsContext` wrapping an empty context. When `.Value(activeSpanKey)` is called on it, the context chain returns `nil` (Background has no values), which triggers the GLS fallback — returning whatever span happens to be active. The span ends up with an unintended parent. **Fix**: Early-return `(nil, false)` in `SpanFromContext` when `ctx == context.Background()`. `context.Background()` is the conventional "no context" sentinel; a caller using it is explicitly opting out of context-based trace propagation. --- **Symptom**: When a function received an explicit `context.Context` carrying span X, but GLS held span Y (from an outer scope), span Y was used as the parent instead of X. **Root cause**: `glsContext.Value` consulted GLS **first**, then fell back to the context chain. Any value present in GLS would silently override the explicitly-propagated one. **Fix**: Reverse the lookup order in `glsContext.Value`: check the context chain first; fall back to GLS only if the chain returns `nil`. GLS is designed for implicit propagation through *un-instrumented* call sites that lack a `context.Context` — it must not override contexts that were explicitly propagated. --- **Symptom**: Functions annotated with `//dd:span` whose first context-like argument was a custom type implementing `context.Context` (e.g. `*fiber.Ctx`) would fall back to `context.TODO()`, breaking trace propagation. **Root cause**: The `//dd:span` injection template only looked for an argument of the **exact** type `context.Context`. If the function's parameter was a custom type that satisfied the interface, it was ignored, and the template fell back to `context.TODO()`. **Fix**: Added an `ArgumentThatImplements "context.Context"` lookup path. When found, the argument is assigned with an explicit interface type declaration (`var ctx context.Context = $impl`) so that the subsequent reassignment from `StartSpanFromContext` (which returns `context.Context`) compiles correctly against the concrete type. - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. You can check this by running `make generate` locally. Unsure? Have a question? Request a review! Co-authored-by: kakkoyun <[email protected]> Co-authored-by: dario.castane <[email protected]>
What does this PR do?
This PR fixes three independent bugs where the wrong span was used as the parent.
Relates to DataDog/orchestrion#798.
Motivation
Orchestrion instruments Go code by injecting span creation logic and using goroutine-local storage (GLS) as a side-channel to propagate the active span through call sites that don't have a
context.Contextin their signature. Spans are stored both in thecontext.Contextchain (explicit) and in the GLS stack (implicit).glsContextwraps acontext.Contextand intercepts.Value()calls to consult both sources.Reproduction Scenario
The repro project used to replicate the issue described in the JIRA ticket implements a
CustomContexttype that embedscontext.Contextand adds aUserIDfield, mimicking how frameworks like Fiber wrap request contexts:theOperation(excluded from instrumentation with//orchestrion:ignore) does the following:Observed output (before fixes):
How each bug manifests:
SPANs 2 & 3 — After
span2is created, GLS holdsspan2as the active span. The second calls to both functions receivecustomCtxwhich carriesspan1in its context chain, but GLS takes priority, sospan2becomes the parent. This is Bug 2 (GLS priority).Additionally, SPAN 1 appearing correct is coincidental: without Bug 3 fixed,
DoThingWithCustomCtxcan't extract a context from*CustomContextand falls back tocontext.TODO(). At that point GLS holdsspan1, which happens to be the right answer. Afterspan2is created, the same fallback to GLS explains why SPAN 2 is wrong:context.TODO()has no span, GLS returnsspan2.SPAN 4 —
theOperation2is created withcontext.Background(). Since Background has no values, the GLS fallback kicks in and returns the activespan1, making it appear as a child of span1 instead of a root span. This is Bug 1 (context.Background()sentinel).Fixes
Bug 1 —
context.Background()inherits active GLS spanSymptom: A span started with
context.Background()(explicit intent: no parent) would incorrectly inherit the active GLS span as its parent.Root cause:
SpanFromContext(context.Background())would callWrapContext(context.Background()), producing aglsContextwrapping an empty context. When.Value(activeSpanKey)is called on it, the context chain returnsnil(Background has no values), which triggers the GLS fallback — returning whatever span happens to be active. The span ends up with an unintended parent.Fix: Early-return
(nil, false)inSpanFromContextwhenctx == context.Background().context.Background()is the conventional "no context" sentinel; a caller using it is explicitly opting out of context-based trace propagation.Bug 2 — GLS overrides an explicitly-propagated context
Symptom: When a function received an explicit
context.Contextcarrying span X, but GLS held span Y (from an outer scope), span Y was used as the parent instead of X.Root cause:
glsContext.Valueconsulted GLS first, then fell back to the context chain. Any value present in GLS would silently override the explicitly-propagated one.Fix: Reverse the lookup order in
glsContext.Value: check the context chain first; fall back to GLS only if the chain returnsnil. GLS is designed for implicit propagation through un-instrumented call sites that lack acontext.Context— it must not override contexts that were explicitly propagated.Bug 3 —
*CustomContextnot recognized as a context source in//dd:spanSymptom: Functions annotated with
//dd:spanwhose first context-like argument was a custom type implementingcontext.Context(e.g.*fiber.Ctx) would fall back tocontext.TODO(), breaking trace propagation.Root cause: The
//dd:spaninjection template only looked for an argument of the exact typecontext.Context. If the function's parameter was a custom type that satisfied the interface, it was ignored, and the template fell back tocontext.TODO().Fix: Added an
ArgumentThatImplements "context.Context"lookup path. When found, the argument is assigned with an explicit interface type declaration (var ctx context.Context = $impl) so that the subsequent reassignment fromStartSpanFromContext(which returnscontext.Context) compiles correctly against the concrete type.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.Unsure? Have a question? Request a review!