chore: wrapping context support for Python 3.15 - #17849
Conversation
Codeowners resolved asResolved from the full PR diff against |
P403n1x87
added
the
changelog/no-changelog
|
BenchmarksBenchmark execution time: 2026-08-20 10:47:35 Comparing candidate commit e530750 in PR branch Found 0 performance improvements and 8 performance regressions! Performance is the same for 608 metrics, 10 unstable metrics.
|
c96d872 to
a6faddd
Compare
2d33056 to
cd95d95
Compare
|
@P403n1x87 I've taken a look at the PR, and it makes sense to me for the most part. For the parts that I don't have much experience with, I've asked Claude for thoughts. Here are its findings, hopefully they will be helpful: Must be fixed before merging1. Hot-path callbacks iterate a mutable dict without synchronization (monitoring.py) The callbacks (_on_py_start, _on_py_return, etc.) do: Meanwhile register() and unregister() mutate that same inner dict (entries[id(handler)] = entry / existing.pop(...)) while holding _registry_lock -- but the callbacks don't acquire the lock. If a register call adds or removes an entry between two iterations of the for loop (GIL can switch between next calls on the dict view), you get RuntimeError: dictionary changed size during iteration. Fix options: (a) snapshot with list(entries.values()) in callbacks (one small allocation per event), (b) swap to a copy-on-write scheme where register/unregister replace the entire inner dict atomically (the reference assignment is GIL-atomic, so the callback always sees a consistent snapshot), or (c) hold the lock in callbacks (worst for latency). Option (b) is the best tradeoff: zero allocation on the hot path, and register/unregister are cold. Nice to have2. Tool ID allocation starts at 0, competing with debuggers (monitoring.py) IDs 0-2 are conventionally reserved (debugger, coverage, profiler). Starting from 0 means ddtrace could claim the debugger slot if no debugger is attached yet, then a later debugpy or pdb attach would fail to register. Starting from 3 (or iterating range(5, -1, -1) to prefer higher IDs) would be more neighborly. 3. _ENTER_FRAME_DEPTH = 3 is fragile (context.py)
This assumes a fixed call-stack depth from the monitored function through the monitoring dispatch to enter. If CPython changes the monitoring callback invocation depth, or if the multiplexer adds/removes a level, this silently produces the wrong frame. Consider walking the stack looking for the code object that matches self.wrapped.code rather than assuming a depth. |
|
Awesome, thanks!
The reasoning here was that it would be unlikely to have mutation while callbacks are invoked, because these are generally installed on enablement (boot). However RC might violate this assumption, so it won't cost us much to be a bit defensive here.
We are a debugger as a matter of fact 🙁 but I guess it doesn't matter where we start with the ID so we can just comply.
Deliberate choice. This value should be fixed for each Python release, so the cost is at most 1 update every year. Still much better than updating a whole bunch of opcodes 🙂 |
This comment was marked as resolved.
This comment was marked as resolved.
Circular import analysis
|
## Description `build_base_venvs` is OOMKilled for cold-`ext_cache` Python versions. Currently this happens for `v3.15` only, which is new and has no warm cache. This was found while working on #17849. **Root cause** - It sets **no** `KUBERNETES_MEMORY_*` and does not disable the VPA, so the autoscaler tunes the limit down to ~5GB, based on the cheap **warm-cache** history. - A cold version compiles everything at 12-way parallelism, which exceeds the approximated limit. ### Changes Pin CPU/memory and disable the VPA on `build_base_venvs`, matching what `.build_base`, `test sdist` already use for the same build targets: ```yaml KUBERNETES_CPU_REQUEST: '6' KUBERNETES_MEMORY_REQUEST: '10Gi' KUBERNETES_MEMORY_LIMIT: '10Gi' DD_DISABLE_VPA: 'true' ``` ## Test plan * [Next PR](#17849) passes [dd-gitlab/build_base_venvs: [3.15]](https://gitlab.ddbuild.io/datadog/apm-reliability/dd-trace-py/builds/1846141002) <img width="494" height="472" alt="Screenshot 2026-07-09 at 4 25 46 PM" src="https://github.com/user-attachments/assets/c622de21-a232-4f23-a085-728ad4461d25" /> ## Additional Notes * Warm builds are unaffected. * The failing job logs also show the GitLab **runner S3 cache returning 403 AccessDenied**, so `ext_cache` restore fails on every run and forces cold compiles every time. That's a runner/`ddbuild` platform-side credential issue. Fixing this separately would restores warm-build speed. Co-authored-by: vlad.scherbich <vlad.scherbich@datadoghq.com>
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new 3.15 monitoring callback captures the wrong frame: the configured depth stops at the dispatch callback rather than the wrapped function. This breaks WrappingContext.__frame__, so get_local() and frame-based products such as debugger snapshots and lazy module execution can read dispatcher locals or fail on Python 3.15.
🤖 Datadog Autotest · Commit b95b04c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
emmettbutler
left a comment
emmettbutler
left a comment
There was a problem hiding this comment.
I'll wait to do a close review until CI is passing and conversations are resolved
Part of the #17849 split (PR 2/6).
Only evict stale _ctx_registry entries when the prior wrapped function was collected, and register sys.monitoring handlers before swapping __code__ to close the race where calls could run without callbacks.
Resolve conflicts in monitoring.py, context.py, and test_monitoring.py. Preserve remote branch 3.15 monitoring wrap hardening while integrating main template bytecode caching and monitoring multiplexer improvements.
Dependency direction analysis
|
prev: #19247 | next: #19253
Summary
Wrapping context + bytecode injection for 3.15.
Test plan