-
Notifications
You must be signed in to change notification settings - Fork 440
refactor(crashtracker): use PyO3 for crashtracker module and crashtracker_exe #12690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
BenchmarksBenchmark execution time: 2025-06-08 18:54:44 Comparing candidate commit 4e58e3a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 553 metrics, 3 unstable metrics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very minor feedback/thoughts, otherwise lgtm
Datadog Summary✅ Code Quality ✅ Code Security ✅ Dependencies Was this helpful? Give us feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits on the command name, otherwise lgtm
@@ -49,6 +49,7 @@ openai = [ | |||
|
|||
[project.scripts] | |||
ddtrace-run = "ddtrace.commands.ddtrace_run:main" | |||
dd_crashtracker_receiver = "ddtrace.commands.crashtracker_exe:main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dd_crashtracker_receiver = "ddtrace.commands.crashtracker_exe:main" | |
_dd_crashtracker_receiver = "ddtrace.commands.crashtracker_exe:main" |
? add "internal" to the name?
I am probably over thinking it, but is this now part of the public API?
dd_crashtracker_receiver = "ddtrace.commands.crashtracker_exe:main" | |
dd-crashtracker-receiver = "ddtrace.commands.crashtracker_exe:main" |
for consistency? or ddtrace-crashtracker-receiver
?
will adding something else "dd*" mess with dd<TAB>
for people doing ddtrace-run
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, I don't want to bother others with autocompletion, I'd put _
in front of it.
And adding dd_crashtracker_receiver
to https://github.com/DataDog/dd-trace-py/blob/0e795abf636c6f81ff3e911db1273fe8a555770a/lib-injection/sources/denied_executables.txt
this list would prevent problems trying to autoinject ddtrace for the receiver, right?
Use PyO3 generated bindings instead of
datadog-crashtracker-ffi
and handwritten C++/CMake shim layers.Notable changes
Use
datadog_crashtracker::on_fork
withforksafe
to handle forksWe used
on_runtime_id_change
callback to updateruntime_id
.dd-trace-py/ddtrace/internal/core/crashtracking.py
Lines 19 to 21 in 2a92230
This only updated
runtime_id
in our C++ intermediary code and did not re-configure crashtracker usingdatadog_crashtracker::on_fork
Removes crashtracker related code from
ddtrace/internal/datadog/profiling
🎉Set
abi3-cp38
feature for PyO3 to not increase lib-injection image size. This results in generating a singlelib_native.so
for all Python versions that we support. When creating lib-injection image, we can deduplicate alllib_native.so
files for different Python versions. Then we avoid putting multiple large.so
file there, reducing lib-injection image size.Also sets
opt-level='s'
inCargo.toml
to reduce size, previously set to 3.Installs
dd_crashtracker_receiver
command, as the receiver binary instead ofcrashtracker_exe_*
. @brettlangdon initially suggested not to expose a new command, as this is for datadog internal use cases. I agree with it and the alternative is simply invoking a Python script which calls receiver routine. For the alternative implementation, see the left side on commit 4c36b80. With the alternative method, crashtracker test on parametric system-tests suite failed to send telemetry log, see https://github.com/DataDog/dd-trace-py/actions/runs/15499177384/job/43642941238. Since I'm going on vacation from June 11 Wednesday and @hoolioh plans to work on Merge profiling and libnative libraries. #13603 with this PR, I updated to this PR as is to pass all tests.Thing to check
dd_crashtracker_receiver
orpython crashtracker_exe.py
would interact with SSI?Checklist
Reviewer Checklist