dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

chore(tracer): use simple queue file for extra service names instead of multiprocessing

Open christophe-papazian opened this issue 1 year ago • 3 comments

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 https://github.com/DataDog/dd-trace-py/pull/9626 ensuring that we still report extra service names.

Checklist

  • [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
  • [x] The change includes or references documentation updates if necessary
  • [x] Backport labels are set (if applicable)

Reviewer Checklist

  • [x] Title is accurate
  • [x] All changes are related to the pull request's stated goal
  • [x] Avoids breaking API 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

christophe-papazian avatar Jul 02 '24 14:07 christophe-papazian

Datadog Report

Branch report: christophe-papazian/use_file_queue_for_extra_service_names Commit report: 688d707 Test service: dd-trace-py

:white_check_mark: 0 Failed, 175545 Passed, 1909 Skipped, 10h 28m 44.39s Total duration (41m 4.97s time saved)

Codecov Report

Attention: Patch coverage is 37.50000% with 35 lines in your changes missing coverage. Please review.

Project coverage is 10.46%. Comparing base (b21d904) to head (6635198). Report is 2 commits behind head on main.

Files Patch % Lines
ddtrace/internal/_file_queue.py 39.53% 26 Missing :warning:
...internal/service_name/test_extra_services_names.py 0.00% 6 Missing :warning:
ddtrace/settings/config.py 57.14% 3 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9701       +/-   ##
===========================================
- Coverage   74.60%   10.46%   -64.15%     
===========================================
  Files        1385     1354       -31     
  Lines      128327   126328     -1999     
===========================================
- Hits        95744    13214    -82530     
- Misses      32583   113114    +80531     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 02 '24 15:07 codecov-commenter

Benchmarks

Benchmark execution time: 2024-07-04 09:08:27

Comparing candidate commit 688d7071aa496cbbaec68a9cf462b768d17490da in PR branch christophe-papazian/use_file_queue_for_extra_service_names with baseline commit d10c434d3469e74d9874f5072c70c34955a8d544 in branch main.

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

scenario:httppropagationinject-ids_only

  • 🟩 max_rss_usage [-1.930MB; -1.841MB] or [-8.712%; -8.311%]

scenario:otelspan-add-metrics

  • 🟩 max_rss_usage [-6.096MB; -5.611MB] or [-14.885%; -13.702%]

scenario:otelspan-add-tags

  • 🟩 max_rss_usage [-4.635MB; -4.220MB] or [-11.407%; -10.388%]

scenario:span-add-metrics

  • 🟩 max_rss_usage [-16.962MB; -16.655MB] or [-25.788%; -25.322%]

pr-commenter[bot] avatar Jul 02 '24 16:07 pr-commenter[bot]

The backport to 2.10 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.10 2.10
# Navigate to the new working tree
cd .worktrees/backport-2.10
# Create a new branch
git switch --create backport-9701-to-2.10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f85625adceb18107014c3229b834a1df7e912395
# Push it to GitHub
git push --set-upstream origin backport-9701-to-2.10
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.10

Then, create a pull request where the base branch is 2.10 and the compare/head branch is backport-9701-to-2.10.

github-actions[bot] avatar Jul 23 '24 17:07 github-actions[bot]

If anyone else comes across this, my Python project runs in a container with read_only filesystem and this PR broke it because FileNotFoundError: [Errno 2] No usable temporary directory found in ['/tmp', '/var/tmp', '/usr/tmp','/app']. I've had to set the environment variable DD_REMOTE_CONFIGURATION_ENABLED: 'false' to make it work again.

harm-nedap avatar Aug 01 '24 11:08 harm-nedap

#10004 fixes behavior which was erroneously introduced here. CC @harm-nedap; disabling the configuration manually should be unnecessary after the linked PR is ~merged~ part of a release (thank you for calling out this behavior and directing others to the fix, though!)

sanchda avatar Aug 01 '24 12:08 sanchda

Thanks for linking that PR, I had not seen it yet :)!

harm-nedap avatar Aug 01 '24 12:08 harm-nedap