refactor: consolidate much shared code in ddtrace.internal.utils#18587
refactor: consolidate much shared code in ddtrace.internal.utils#18587emmettbutler wants to merge 33 commits into
Conversation
emmettbutler
requested review from
Chronobreak,
jchrostek-dd,
juanjux,
leoromanovsky,
mabdinur,
quinna-h and
typotter
Circular import analysis🚨 New circular imports detected 🚨195 new circular import(s) have been introduced by this PR: Show new cycles (showing 5 of 195 shortest)
Please consider refactoring your changes in accordance to the Separation of Concerns principle.
|
|
![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: 02e5a7b2d3
ℹ️ 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".
Codeowners resolved as |
BenchmarksBenchmark execution time: 2026-07-09 16:51:45 Comparing candidate commit 7dc57f7 in PR branch Found 0 performance improvements and 3 performance regressions! Performance is the same for 611 metrics, 10 unstable metrics. scenario:iastaspects-swapcase_aspect
scenario:iastaspectsospath-ospathbasename_aspect
scenario:span-start
|
P403n1x87
left a comment
P403n1x87
left a comment
There was a problem hiding this comment.
I love the idea, but I feel that this PR doesn't make a step forward towards modularisation yet. IMO it would give us more value if we addressed the existing problems (like partitioning the monolithic constants.py module, moving context-specific utilities under the more relevant component etc...) first so that we don't end up bloating the new generic package with product-specific stuff. My feeling is that, once we have done this, ddtrace.internal would already contain just the content of the new package, without the need to move things under utils.
emmettbutler
changed the title
P403n1x87
left a comment
P403n1x87
left a comment
There was a problem hiding this comment.
Approving as this is a non-functional change. But I still believe that this work is premature and that the other tasks mentioned in the other review comments should be prioritised over this. I expect that we will have to undo most of the changes here (e.g. remove the new shim modules), so I feel there are a few unnecessary moves and throwaway work that we can spare by preparing the ground a bit better.
| ignore_errors = true | ||
| no_implicit_reexport = false | ||
|
|
||
| # ddtrace.internal.* shims re-export from ddtrace.internal.utils.* via `import *`. |
There was a problem hiding this comment.
do we need to move to __all__ and/or explicit re-exports vs just import *?
it seems with these changes we'll stop doing type checking on anything in the new utils/*.py modules now?
is that why warn_return_any = false is now needed? seems like we might not want that ?
| files = ddtrace, | ||
| docs | ||
| exclude = ddtrace/appsec/_iast/_taint_tracking/cmake-build-debug/|ddtrace/appsec/_iast/_taint_tracking/_vendor/|ddtrace/appsec/_iast/_taint_tracking/_deps/|ddtrace/internal/datadog/profiling/build|ddtrace/profiling/collector/test/build/|tests/profiling/collector/pprof_\d.*_pb2\.py | ||
| exclude = ddtrace/internal/constants\.py|ddtrace/internal/compat\.py|ddtrace/internal/module\.py|ddtrace/internal/threads\.py|ddtrace/internal/logger\.py|ddtrace/internal/_exceptions\.py|ddtrace/internal/schema/__init__\.py|ddtrace/internal/schema/processor\.py|ddtrace/internal/schema/span_attribute_schema\.py|ddtrace/internal/wrapping/__init__\.py|ddtrace/internal/wrapping/context\.py|ddtrace/internal/wrapping/generators\.py|ddtrace/internal/wrapping/asyncs\.py|ddtrace/appsec/_iast/_taint_tracking/cmake-build-debug/|ddtrace/appsec/_iast/_taint_tracking/_vendor/|ddtrace/appsec/_iast/_taint_tracking/_deps/|ddtrace/internal/datadog/profiling/build|ddtrace/profiling/collector/test/build/|tests/profiling/collector/pprof_\d.*_pb2\.py |
There was a problem hiding this comment.
This exclude list adds ddtrace/internal/constants.py, _exceptions.py, and schema/*, but none of those files have any code changes in this branch. Looks like scope creep unrelated to the utils consolidation. Can this be reverted or split into its own PR with its own justification?
| check_untyped_defs = false | ||
|
|
||
| [mypy-ddtrace.internal.threads] | ||
| [mypy-ddtrace.internal.utils.threads] |
There was a problem hiding this comment.
This section points at ddtrace.internal.utils.threads, but that module does not exist on this branch (the threads move was reverted, ddtrace/internal/threads.py has zero diff). Looks like a leftover from the revert, can you remove this section along with the threads.py entry added to exclude above?
| return isinstance(o, wrapt_class) | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
This overwrites the curated __all__ above and re-exports everything, including stdlib imports and typing names. It also rebinds dunder attributes like __name__ and __file__ in any module that does from ... import * on this file (verified locally). Not breaking anything today since the shim is a one liner, but a narrower explicit __all__ would be safer.
| return False | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py, this re-exports everything including stdlib imports and rebinds dunder attributes on star import.
| _globals["__getattr__"] = __getattr__ | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py.
| return None | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py.
| instrs[-1:] = ASYNC_GEN_ASSEMBLY.bind(lineno=lineno) | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py.
| return record.uwc if record is not None else None | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py.
| instrs[-1:] = GENERATOR_ASSEMBLY.bind(lineno=lineno) | ||
|
|
||
|
|
||
| __all__ = list(locals().keys()) |
There was a problem hiding this comment.
Same __all__ = list(locals().keys()) concern as in compat.py.
| env["DD_TRACE_SPAN_ATTRIBUTE_SCHEMA"] = schema | ||
| env["PYTHONWARNINGS"] = "ignore::UserWarning:ddtrace.internal.module" | ||
| env["PYTHONWARNINGS"] = "ignore::UserWarning:ddtrace.internal.utils.module" | ||
| out, err, status, _ = ddtrace_run_python_code_in_subprocess(code, env=env) |
There was a problem hiding this comment.
The assert err == b"", err.decode() check right after this line was deleted rather than fixed. That suggests the move causes something to print to stderr here that was not there before. Worth finding out what and fixing that instead of dropping the check.
9 participants
Description
This change moves a bunch of sources from
ddtrace.internaltoddtrace.internal.utilsin the interest of colocating as much as possible code that's used by multiple product components of the library. This colocation will enable the eventual separation of shared functionality into a separate package.ddtrace.internal.utilsis chosen as the target location due to its high concentration of code that's both shared by multiple products and not specific to any of those products.Existing imports are unchanged because of the presence of reexports at the locations of the moved modules.
Testing
No functional changes, so existing tests are appropriate.