Skip to content
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

refactor: implement native periodic thread #7659

Merged
merged 33 commits into from
May 22, 2024

Conversation

P403n1x87
Copy link
Contributor

@P403n1x87 P403n1x87 commented Nov 17, 2023

This PR re-implements the periodic service to use a periodic thread implemented in C++. The reason for doing this is to reduce the dependency of the library on the threading module, which is the source of issues when it comes to frameworks such as gevent.

This is the first step towards a refactoring that is aimed at removing the dependency on the threading module entirely, potentially leading to phasing out the module cloning mechanism, which is what we currently have in place to support frameworks such as the already mentioned gevent.

Testing strategy

The existing test suite provides already many reliable test cases that can give us good confidence about the proposed changes. Further testing is being performed in internal environments.

Performance

We do not expect any particular performance impact from this change

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly 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
  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

@P403n1x87 P403n1x87 added the changelog/no-changelog A changelog entry is not required for this PR. label Nov 17, 2023
@P403n1x87 P403n1x87 force-pushed the refactor/native-periodic-thread branch 8 times, most recently from 07bc612 to 14028af Compare November 25, 2023 11:43
tests/conftest.py Outdated Show resolved Hide resolved
@P403n1x87 P403n1x87 force-pushed the refactor/native-periodic-thread branch 19 times, most recently from f43432f to f9e1df3 Compare November 30, 2023 12:23
@brettlangdon
Copy link
Member

Running flask-simple locally:

Benchmark ddtrace==2.8.3 native periodic thread
flasksimple-profiler 701 us 686 us: 1.02x faster
flasksimple-debugger 595 us 557 us: 1.07x faster
flasksimple-iast-get 560 us 550 us: 1.02x faster
flasksimple-appsec-get 1.21 ms 1.13 ms: 1.07x faster
flasksimple-appsec-post 1.01 ms 1.03 ms: 1.02x slower
Geometric mean (ref) 1.02x faster

Benchmark hidden because not significant (3): flasksimple-baseline, flasksimple-tracer, flasksimple-appsec-telemetry

@emmettbutler emmettbutler self-requested a review May 9, 2024 14:49
@emmettbutler emmettbutler force-pushed the refactor/native-periodic-thread branch from 2fe19d7 to f1e12a6 Compare May 10, 2024 15:10
@P403n1x87 P403n1x87 force-pushed the refactor/native-periodic-thread branch from 636ede5 to 0d56915 Compare May 16, 2024 13:47
@P403n1x87 P403n1x87 force-pushed the refactor/native-periodic-thread branch from 0d56915 to e812ad8 Compare May 16, 2024 13:54
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2024

Codecov Report

Attention: Patch coverage is 2.85714% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 10.19%. Comparing base (33b1a94) to head (b2a1664).

Files Patch % Lines
ddtrace/internal/periodic.py 7.14% 13 Missing ⚠️
tests/internal/test_forksafe.py 0.00% 10 Missing ⚠️
tests/appsec/appsec/test_remoteconfiguration.py 0.00% 4 Missing ⚠️
tests/internal/test_periodic.py 0.00% 3 Missing ⚠️
tests/conftest.py 0.00% 2 Missing ⚠️
tests/subprocesstest.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7659       +/-   ##
===========================================
- Coverage   76.15%   10.19%   -65.96%     
===========================================
  Files        1287     1257       -30     
  Lines      121888   120004     -1884     
===========================================
- Hits        92818    12235    -80583     
- Misses      29070   107769    +78699     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brettlangdon brettlangdon enabled auto-merge (squash) May 20, 2024 12:20
@brettlangdon brettlangdon merged commit beca788 into DataDog:main May 22, 2024
174 of 180 checks passed
@emmettbutler
Copy link
Collaborator

@P403n1x87 this seems to be a proximate cause of failures like this on main.

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.

7 participants