Skip to content

fix(internal/orchestrion): fix span parenting with GSL#4528

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 17 commits into
mainfrom
dario.castane/APMS-18578/fix-orchestrion-context-handling
Mar 20, 2026
Merged

fix(internal/orchestrion): fix span parenting with GSL#4528
gh-worker-dd-mergequeue-cf854d[bot] merged 17 commits into
mainfrom
dario.castane/APMS-18578/fix-orchestrion-context-handling

Conversation

@darccio

@darccio darccio commented Mar 12, 2026

Copy link
Copy Markdown
Member

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.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.

Reproduction Scenario

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:

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:

// 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 4theOperation2 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).


Fixes

Bug 1 — context.Background() inherits active GLS span

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.


Bug 2 — GLS overrides an explicitly-propagated context

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.


Bug 3 — *CustomContext not recognized as a context source in //dd:span

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.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • New code is free of linting errors. You can check this by running make lint locally.
  • New code doesn't break existing tests. You can check this by running make test locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • All generated files are up to date. You can check this by running make generate locally.

Unsure? Have a question? Request a review!

@darccio
darccio requested review from a team as code owners March 12, 2026 11:26
@darccio darccio added team:apm-go AI Assisted AI/LLM assistance used in this PR (partially or fully) labels Mar 12, 2026
@codecov

codecov Bot commented Mar 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.13%. Comparing base (6ae9763) to head (a58ca9a).

Files with missing lines Patch % Lines
ddtrace/tracer/context.go 40.00% 2 Missing and 1 partial ⚠️
internal/orchestrion/context.go 0.00% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
ddtrace/tracer/context.go 58.13% <40.00%> (-22.82%) ⬇️
internal/orchestrion/context.go 63.15% <0.00%> (-11.85%) ⬇️

... and 263 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Mar 12, 2026

Copy link
Copy Markdown

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 25.00%
Overall Coverage: 59.40% (-0.02%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a58ca9a | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

Comment thread ddtrace/tracer/context.go Outdated
@darccio
darccio marked this pull request as draft March 12, 2026 12:51
@darccio

darccio commented Mar 12, 2026

Copy link
Copy Markdown
Member Author

Reverting to draft because it needs the PR in Orchestrion to be merged.

@pr-commenter

pr-commenter Bot commented Mar 12, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-03-20 09:32:04

Comparing candidate commit a58ca9a in PR branch dario.castane/APMS-18578/fix-orchestrion-context-handling with baseline commit 6ae9763 in branch main.

Found 2 performance improvements and 0 performance regressions! Performance is the same for 214 metrics, 8 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:BenchmarkPayloadVersions/metastruct_10spans/v1_0-25

  • 🟩 execution_time [-245.844ns; -182.556ns] or [-3.295%; -2.447%]

scenario:BenchmarkPayloadVersions/metastruct_1spans/v1_0-25

  • 🟩 execution_time [-147.303ns; -104.297ns] or [-3.684%; -2.609%]

@darccio
darccio requested a review from kakkoyun March 12, 2026 16:58
@darccio
darccio marked this pull request as ready for review March 12, 2026 16:58
@darccio
darccio requested review from a team as code owners March 12, 2026 16:58

@kakkoyun kakkoyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darccio

darccio commented Mar 13, 2026

Copy link
Copy Markdown
Member Author

@codex review

@RomainMuller RomainMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/tracer/orchestrion.yml Outdated

@rarguelloF rarguelloF left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not 100% sure of the changes to the graphql-go/graphql integration)

Comment thread contrib/graphql-go/graphql/graphql.go
Comment thread internal/orchestrion/_integration/99designs.gqlgen/gqlgen.go
Comment thread contrib/graphql-go/graphql/graphql_test.go
Comment thread contrib/graphql-go/graphql/graphql.go
@darccio
darccio requested a review from rarguelloF March 17, 2026 11:46
@darccio
darccio requested a review from rarguelloF March 19, 2026 11:15
@darccio

darccio commented Mar 19, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/tracer/orchestrion.yml Outdated
Comment on lines +84 to +87
var __dd_span_ctx context.Context = ctx
{{- else -}}
{{- $ctx = "ctx" -}}
var ctx context.Context = {{ $impl }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@rarguelloF rarguelloF left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@darccio
darccio requested review from a team as code owners March 20, 2026 08:15
@darccio
darccio force-pushed the dario.castane/APMS-18578/fix-orchestrion-context-handling branch from 20c3dd9 to b7b6c99 Compare March 20, 2026 08:46
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]>
@darccio

darccio commented Mar 20, 2026

Copy link
Copy Markdown
Member Author

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Mar 20, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-03-20 09:17:03 UTC ℹ️ Start processing command /merge


2026-03-20 09:17:12 UTC ℹ️ MergeQueue: waiting for PR to be ready

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.
It will be added to the queue as soon as checks pass and/or get approvals. View in MergeQueue UI.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2026-03-20 09:35:07 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 30m (p90).


2026-03-20 09:46:26 UTC ℹ️ MergeQueue: This merge request was merged

@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit f12caf8 into main Mar 20, 2026
137 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the dario.castane/APMS-18578/fix-orchestrion-context-handling branch March 20, 2026 09:46
darccio added a commit that referenced this pull request Mar 27, 2026
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Assisted AI/LLM assistance used in this PR (partially or fully) mergequeue-status: done team:apm-go

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants