pkg/dyninst: Fix context implementation capture#53746
Conversation
82b651f to
d1b0d10
Compare
grantseltzer
changed the title
grantseltzer
added
changelog/no-changelog
When the dynamic instrumentation debugger captured a Go value that implements the context.Context interface, it always applied its context handling: it walked the value's internal context chain to pull out the trace and span IDs and, in doing so, replaced the capture of the value's own fields. That is correct for real context plumbing, but many ordinary structs implement the interface only through methods that delegate elsewhere (for example rapid.Context, a web request wrapper) and are captured to inspect their fields. For those, the field capture was swapped for the chain walk, which produced bogus type information for the struct's interface fields (errors like "UnknownType(0x...) out of bounds") and, because the value delegates its context behavior, recovered no trace information anyway. This change applies the context handling only to values that are actually a link in a walkable chain, meaning they have a parent context to follow; a value that implements the interface but is really just a struct is now captured normally so its fields decode correctly, while trace and span IDs from genuine contexts are still extracted and promoted to the top of the snapshot as before. Validated with a new reproduction test that captures such a struct across Go 1.23 through 1.26 and by confirming the existing context trace-correlation output is unchanged.
d1b0d10 to
f351926
Compare
![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.
Reviewed commit: d1b0d104d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ImplementationType Concrete context.Context implementations are wrapped in the type catalog as GoContextImplementationType after finalizeTypes has already bound every PointerType.Pointee and struct field to the pre-wrap StructureType, so the wrapper and the orphaned raw struct end up as two distinct objects sharing one type ID; because the compiler keys each type's ProcessType handler by object identity while deduping by ID, whichever object it happened to process first won the enqueue_pc registration, and the loader's lookup for the wrapper instance intermittently returned 0, which dropped the impl's field-descent program entirely and surfaced as flaky UnknownType-out-of-bounds or depth output depending on map iteration order. This wraps delegating impls (those that satisfy the interface only by forwarding, with no parent context or key/value payload) as GoContextImplementationType too so the IR label is descriptive, gates the chain-walk machinery in the compiler and loader behind a HasChainData predicate so delegating impls are captured as ordinary structs, and adds a rebindTypeReferences pass after annotateSpecialGoTypes so pointees, fields, and variables point at the wrapper instances, removing the object-identity race for good. Because that rebind now routes the wrapper types into the SymDB memory-layout path as well, typeMemoryLayout learns to treat the context-impl and dd-trace-span wrappers like the struct they wrap. Validated by regenerating the irgen snapshots and running the irgen, compiler, decode and symdb package tests plus TestDyninst for the sample service forty times on the previously flaky go1.25.0 build and the full four-toolchain suite eight times, all green, with the integration decoded golden unchanged.
f351926 to
ba9c1b6
Compare
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit ba9c1b6 |
Files inventory check summaryFile checks results against ancestor c24fada1: Results for datadog-agent_7.83.0~devel.git.103.ba9c1b6.pipeline.125039327-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
20 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: c24fada ❌ Experiments with retried target crashesThis is a critical error. One or more replicates failed with a non-zero exit code. These replicates may have been retried. See Replicate Execution Details for more information.
Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +0.36 | [-0.62, +1.34] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.23 | [+0.19, +0.27] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.22 | [+0.17, +0.27] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.14 | [+0.08, +0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.09 | [+0.02, +0.16] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.02 | [-0.07, +0.12] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.70 | [-0.95, -0.44] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 148.42MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 733.68KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 492.13MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 183.36MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.15MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 355.29 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 406.62MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.93GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.34 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 300.47MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 63.81 ≤ 80 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 281.07MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.72 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 280.72MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Normal Replicate Execution Failures (non-profiling)
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 1 | Oom killed | Debug Dashboard |
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_idle | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_mean_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
andreimatei
left a comment
andreimatei
left a comment
There was a problem hiding this comment.
I was very confused by the PR description and the .md note, until I I think I figured out the deal:
a "delegating" wrapper, with no real parent context to follow
but many ordinary structs implement the interface only through methods that delegate elsewhere (for example rapid.Context, a web request wrapper)
It's the other way around, isn't it? The fix is about implementations of context.Context who are... not wrappers. Like, I would consider a context.Context implementation that we recognize fine because it contains a context.Context field to be a wrapper.
If I understand correctly, the fix is for context.Context impls that we don't recognize properly, right? Like, types that implement context.Context but do not have a context.Context field and are also not one of the well known ones (e.g. cancelCtx, valueCtx, etc). Is that right? If so, let's stay away from "wrapper" and talk in terms of "supported implementations of context" and "unsupported ...". The fix would be about the unsupported ones.
| // "UnknownType(...) out of bounds" when such a struct was wrongly handled as a | ||
| // context. | ||
| type contextImpl struct { | ||
| logger fmt.Stringer // time.Duration (foreign) |
There was a problem hiding this comment.
what does "foreign" mean?
|
The distinction is whether the type is a link in a walkable context chain (has a parent Context or key/value, like cancelCtx/valueCtx) or not. A walkable context stores its parent context as a field that eBPF can read at a fixed offset (like cancelCtx), so it can hop parent to parent looking for a trace ID. The fix is for the ones that aren't chain links, which we were mishandling. I'll drop "wrapper"/"delegating" and reword everything (PR description, the .md note, and the code comments) in terms of chain-linked vs not. |
2 participants
What does this PR do?
When the debugger captures a value that implements Go's context.Context but only forwards those methods elsewhere (a "delegating" wrapper, with no real parent context to follow), it now captures the value's own fields normally instead of mistaking it for a trace-carrying context and trying to walk a chain that isn't there.
It also fixes a deeper bug that made this behavior unreliable: after the debugger re-labeled these context types internally, it left some references pointing at stale copies of the same type. That mismatch caused the "how to capture this" instructions to occasionally go missing, so the value's fields would silently drop out and the snapshot would show garbage type names. Which runs hit it varied from run to run and between Go versions. The fix re-links all references to the single correct copy right after re-labeling, so the mismatch can't happen.
Motivation
Snapshots for these values were intermittently broken: fields showed up as UnknownType(...) out of bounds / missing type information, or the whole value was reported as not captured. Because it depended on internal ordering that shuffles each run, it looked random and was hard to trust. This makes the capture correct and stable, and gives the type a descriptive internal label instead of pretending it's a plain struct.
Describe how you validated your changes
Additional Notes
While fixing the reference re-linking, one downstream routine (the SymDB memory-layout code) had a hand-written list of type shapes it understood and hadn't been told about the newly-reaching wrapper type; it now treats that wrapper like the plain struct it wraps. I audited the other similar spots and confirmed the rest either run before the labeling step or already handle it.