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

chore(tracer): use simple queue file for extra service names instead of multiprocessing [backport 2.10] #9910

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

tabgok
Copy link
Contributor

@tabgok tabgok commented Jul 23, 2024

…of multiprocessing (#9701)

To prevent further problems with multiprocessing in config:

  • remove multiprocessing queue from main config file
  • add a new File_Queue using low level locking to write into temporary file
  • Use the new queue to exchange messages between multiple processes for extra service names
  • update tests
  • added a new unit-tests workflow on github to launch any tests on linux(x86), macos(arm64) and windows(x86). (For now, only the test relevant to that PR is used).

This is an alternative solution to
#9626 ensuring that we still report extra service names.

APPSEC-53927

  • 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)

  • 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

(cherry picked from commit f85625a)

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

…of multiprocessing (#9701)

To prevent further problems with multiprocessing in config:
- remove multiprocessing queue from main config file
- add a new File_Queue using low level locking to write into temporary
file
- Use the new queue to exchange messages between multiple processes for
extra service names
- update tests
- added a new unit-tests workflow on github to launch any tests on
linux(x86), macos(arm64) and windows(x86). (For now, only the test
relevant to that PR is used).

This is an alternative solution to
#9626 ensuring that we still
report extra service names.

[APPSEC-53927]

- [x] The PR description includes an overview of the change
- [x] The PR description articulates the motivation for the change
- [x] The change includes tests OR the PR description describes a
testing strategy
- [x] The PR description notes risks associated with the change, if any
- [x] Newly-added code is easy to change
- [x] The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- [x] The change includes or references documentation updates if
necessary
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Newly-added code is easy to change
- [x] Release note makes sense to a user of the library
- [x] If necessary, author has acknowledged and discussed the
performance implications of this PR as reported in the benchmarks PR
comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

[APPSEC-53927]:
https://datadoghq.atlassian.net/browse/APPSEC-53927?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

(cherry picked from commit f85625a)
@tabgok tabgok marked this pull request as ready for review July 23, 2024 17:15
@tabgok tabgok requested review from a team as code owners July 23, 2024 17:15
@tabgok tabgok added the changelog/no-changelog A changelog entry is not required for this PR. label Jul 23, 2024
@tabgok tabgok changed the title chore(tracer): use simple queue file for extra service names instead … chore(tracer): use simple queue file for extra service names instead of multiprocessing [backport 2.10] Jul 23, 2024
@pr-commenter
Copy link

pr-commenter bot commented Jul 23, 2024

Benchmarks

Benchmark execution time: 2024-07-23 18:18:47

Comparing candidate commit 6aa2384 in PR branch backport-9701-to-2.10 with baseline commit 07604aa in branch 2.10.

Found 7 performance improvements and 0 performance regressions! Performance is the same for 214 metrics, 9 unstable metrics.

scenario:httppropagationextract-full_t_id_datadog_headers

  • 🟩 max_rss_usage [-1.872MB; -1.588MB] or [-8.602%; -7.297%]

scenario:otelspan-add-metrics

  • 🟩 max_rss_usage [-6.323MB; -6.266MB] or [-15.418%; -15.280%]

scenario:otelspan-add-tags

  • 🟩 max_rss_usage [-6.322MB; -5.973MB] or [-15.449%; -14.596%]

scenario:ratelimiter-high_rate_limit

  • 🟩 max_rss_usage [-1.754MB; -1.690MB] or [-7.925%; -7.634%]

scenario:ratelimiter-low_rate_limit

  • 🟩 max_rss_usage [-1.780MB; -1.599MB] or [-8.055%; -7.239%]

scenario:span-add-metrics

  • 🟩 max_rss_usage [-17.033MB; -16.824MB] or [-25.648%; -25.335%]

scenario:span-add-tags

  • 🟩 max_rss_usage [-6.079MB; -6.026MB] or [-17.401%; -17.251%]

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Jul 23, 2024

Datadog Report

Branch report: backport-9701-to-2.10
Commit report: cf93900
Test service: dd-trace-py

✅ 0 Failed, 117592 Passed, 59066 Skipped, 4h 19m 43.18s Total duration (54m 26.82s time saved)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 34.61538% with 51 lines in your changes missing coverage. Please review.

Project coverage is 10.21%. Comparing base (23e1da9) to head (6aa2384).
Report is 20 commits behind head on 2.10.

Files Patch % Lines
ddtrace/internal/_file_queue.py 37.09% 39 Missing ⚠️
...internal/service_name/test_extra_services_names.py 0.00% 9 Missing ⚠️
ddtrace/settings/config.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             2.10    #9910       +/-   ##
===========================================
- Coverage   27.69%   10.21%   -17.48%     
===========================================
  Files        1309     1309               
  Lines      124119   124564      +445     
===========================================
- Hits        34371    12725    -21646     
- Misses      89748   111839    +22091     

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

@tabgok tabgok enabled auto-merge (squash) July 24, 2024 13:14
@tabgok tabgok merged commit 6ee9093 into 2.10 Jul 24, 2024
156 of 166 checks passed
@tabgok tabgok deleted the backport-9701-to-2.10 branch July 24, 2024 13:44
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.

4 participants