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

Do not init the tracer by default in Worker threads #5124

Closed
wants to merge 2 commits into from

Conversation

uurien
Copy link
Collaborator

@uurien uurien commented Jan 17, 2025

What does this PR do?

Prevent the tracer init in Worker threads by default.

Motivation

Additional Notes

Copy link

github-actions bot commented Jan 17, 2025

Overall package size

Self size: 8.49 MB
Deduped: 94.84 MB
No deduping: 95.35 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.6.1 | 2.59 MB | 2.73 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@pr-commenter
Copy link

pr-commenter bot commented Jan 17, 2025

Benchmarks

Benchmark execution time: 2025-01-17 16:08:41

Comparing candidate commit 6102f21 in PR branch ugaitz/do-not-init-in-wt-bydefault with baseline commit 3b8a6b9 in branch master.

Found 1 performance improvements and 1 performance regressions! Performance is the same for 904 metrics, 27 unstable metrics.

scenario:appsec-iast-with-vulnerability-iast-enabled-always-active-22

  • 🟩 max_rss_usage [-10.929MB; -8.176MB] or [-8.110%; -6.067%]

scenario:startup-with-tracer-20

  • 🟥 cpu_user_time [+12.532ms; +21.022ms] or [+5.237%; +8.784%]

@uurien uurien marked this pull request as ready for review January 17, 2025 15:42
@uurien uurien requested a review from a team as a code owner January 17, 2025 15:42
@juan-fernandez
Copy link
Collaborator

juan-fernandez commented Jan 17, 2025

@uurien could you to add this logic to ci/init.js as well? I'm just curious whether the newest DI + Test optimization logic would work with this change.

Also: are we worried at all about this being a breaking change?

@uurien
Copy link
Collaborator Author

uurien commented Jan 17, 2025

Also: are we worried at all about this being a breaking change?

As we are checking that in initialize.mjs I consider this change more a fix rather than a breaking change.

@uurien uurien marked this pull request as draft January 17, 2025 16:13
@watson
Copy link
Collaborator

watson commented Jan 18, 2025

@uurien Most of the Dynamic Instrumentation code runs in Worker Thread. It doesn't need to instrument the code in the Worker Thread, but it expects to access certain parts of the tracer - e.g. the logger. I can see that debugger tests are failing. Let me know if you need my input or review on this PR.

@uurien uurien closed this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants