Skip to content

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

Open
wants to merge 99 commits into
base: main
Choose a base branch
from

Conversation

taegyunkim
Copy link
Contributor

@taegyunkim taegyunkim commented Mar 10, 2025

Use PyO3 generated bindings instead of datadog-crashtracker-ffi and handwritten C++/CMake shim layers.

Notable changes

  1. Use datadog_crashtracker::on_fork with forksafe to handle forks
    We used on_runtime_id_change callback to update runtime_id.

    @on_runtime_id_change
    def _update_runtime_id(runtime_id: str) -> None:
    crashtracker.set_runtime_id(runtime_id)

    This only updated runtime_id in our C++ intermediary code and did not re-configure crashtracker using datadog_crashtracker::on_fork

  2. Removes crashtracker related code from ddtrace/internal/datadog/profiling 🎉

  3. Set abi3-cp38 feature for PyO3 to not increase lib-injection image size. This results in generating a single lib_native.so for all Python versions that we support. When creating lib-injection image, we can deduplicate all lib_native.so files for different Python versions. Then we avoid putting multiple large .so file there, reducing lib-injection image size.

  4. Also sets opt-level='s' in Cargo.toml to reduce size, previously set to 3.

  5. Installs dd_crashtracker_receiver command, as the receiver binary instead of crashtracker_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

  • How would dd_crashtracker_receiver or python crashtracker_exe.py would interact with SSI?

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

Copy link
Contributor

github-actions bot commented Mar 10, 2025

CODEOWNERS have been resolved as:

ddtrace/commands/crashtracker_exe.py                                    @DataDog/apm-core-python
src/native/crashtracker.rs                                              @DataDog/apm-core-python
.gitignore                                                              @DataDog/apm-core-python
ddtrace/internal/core/crashtracking.py                                  @DataDog/apm-core-python
ddtrace/internal/datadog/profiling/build_standalone.sh                  @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt            @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/include/constants.hpp     @DataDog/profiling-python
ddtrace/internal/datadog/profiling/util.py                              @DataDog/profiling-python
ddtrace/internal/native/_native.pyi                                     @DataDog/apm-core-python
ddtrace/settings/crashtracker.py                                        @DataDog/apm-core-python
pyproject.toml                                                          @DataDog/python-guild
scripts/gen_gitlab_config.py                                            @DataDog/apm-core-python
setup.py                                                                @DataDog/python-guild
src/native/Cargo.lock                                                   @DataDog/apm-core-python
src/native/Cargo.toml                                                   @DataDog/apm-core-python
src/native/lib.rs                                                       @DataDog/apm-core-python
tests/internal/crashtracker/test_crashtracker.py                        @DataDog/apm-core-python
tests/internal/crashtracker/utils.py                                    @DataDog/apm-core-python

@taegyunkim taegyunkim changed the title conditional compile Use PyO3 for crashtracker module and crashtraker_exe Mar 10, 2025
@taegyunkim taegyunkim changed the title Use PyO3 for crashtracker module and crashtraker_exe Use PyO3 for crashtracker module and crashtracker_exe Mar 10, 2025
@taegyunkim taegyunkim changed the title Use PyO3 for crashtracker module and crashtracker_exe feat(crashtracker): Use PyO3 for crashtracker module and crashtracker_exe Mar 10, 2025
@taegyunkim taegyunkim changed the title feat(crashtracker): Use PyO3 for crashtracker module and crashtracker_exe feat(crashtracker): use PyO3 for crashtracker module and crashtracker_exe Mar 10, 2025
@pr-commenter
Copy link

pr-commenter bot commented Mar 11, 2025

Benchmarks

Benchmark execution time: 2025-06-08 18:54:44

Comparing candidate commit 4e58e3a in PR branch taegyunkim/prof-11491-crasht-pyo3 with baseline commit 10e09e0 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 553 metrics, 3 unstable metrics.

@taegyunkim taegyunkim changed the title feat(crashtracker): use PyO3 for crashtracker module and crashtracker_exe refactor(crashtracker): use PyO3 for crashtracker module and crashtracker_exe Mar 11, 2025
@taegyunkim taegyunkim added the changelog/no-changelog A changelog entry is not required for this PR. label May 8, 2025
Copy link
Member

@brettlangdon brettlangdon left a 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-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented May 12, 2025

Datadog Summary

✅ Code Quality    ✅ Code Security    ✅ Dependencies


Was this helpful? Give us feedback!

Copy link
Member

@brettlangdon brettlangdon left a 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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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?

Suggested change
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 ?

Copy link
Contributor Author

@taegyunkim taegyunkim Jun 10, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants