Skip to content

Turn async ID gathering off by default #5621

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

Merged
merged 3 commits into from
Apr 30, 2025
Merged

Conversation

szegedi
Copy link
Contributor

@szegedi szegedi commented Apr 28, 2025

What does this PR do?

Sets DD_PROFILING_ASYNC_ID_ENABLED to disabled by default.

Motivation

Customers have problems with it enabled by default. See the Jira issue for more details. It mostly hinges on non-safety of calling node::AsyncHooksGetExecutionAsyncId(v8::Isolate*) under certain circumstances. We should re-enable this after we can call node::AsyncHooksGetExecutionAsyncId(v8::Local<v8::Context>) from nodejs/node#57820 with a safely pre-obtained v8::Context object.

Additional Notes

Jira: PROF-11628

@szegedi szegedi requested a review from a team as a code owner April 28, 2025 10:35
@szegedi szegedi requested a review from nsavoire April 28, 2025 10:36
Copy link

codecov bot commented Apr 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.03%. Comparing base (3da675e) to head (e026a3f).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5621      +/-   ##
==========================================
- Coverage   79.25%   79.03%   -0.22%     
==========================================
  Files         513      506       -7     
  Lines       23412    23373      -39     
==========================================
- Hits        18556    18474      -82     
- Misses       4856     4899      +43     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

github-actions bot commented Apr 28, 2025

Overall package size

Self size: 9.3 MB
Deduped: 102.55 MB
No deduping: 103.07 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.1 | 29.73 MB | 29.73 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 3.3.1 | 13.99 MB | 13.99 MB | | @datadog/pprof | 5.7.1 | 9.51 MB | 9.88 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 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.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.8 | 25.08 kB | 25.08 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 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

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Apr 28, 2025

Datadog Report

Branch report: szegedi/disable-async-id
Commit report: cf6e067
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 927 Passed, 0 Skipped, 11m 59.1s Total Time

@szegedi szegedi marked this pull request as draft April 28, 2025 13:15
@szegedi szegedi force-pushed the szegedi/disable-async-id branch 2 times, most recently from 6ce3ca9 to 0e9a60d Compare April 28, 2025 16:12
@pr-commenter
Copy link

pr-commenter bot commented Apr 28, 2025

Benchmarks

Benchmark execution time: 2025-04-30 10:04:13

Comparing candidate commit e026a3f in PR branch szegedi/disable-async-id with baseline commit 3da675e in branch master.

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

@szegedi szegedi marked this pull request as ready for review April 29, 2025 13:36
Copy link
Collaborator

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

If the feature is actually unstable in all current Node.js versions, let's just plain out prevent usage for now. I don't think we should enable this.

After looking at recent github issues, I fear this might be related to these reports:

#5565
#5612

nsavoire
nsavoire previously approved these changes Apr 30, 2025
@szegedi
Copy link
Contributor Author

szegedi commented Apr 30, 2025

If the feature is actually unstable in all current Node.js versions, let's just plain out prevent usage for now. I don't think we should enable this.

Okay, I reverted all related commits.

Copy link
Collaborator

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thanks for the quick follow-up :)
Reverting does indeed seem like the cleanest way!

@BridgeAR BridgeAR merged commit c1d53e2 into master Apr 30, 2025
440 checks passed
@BridgeAR BridgeAR deleted the szegedi/disable-async-id branch April 30, 2025 11:43
dd-trace-js bot pushed a commit that referenced this pull request May 1, 2025
* Revert "Constrain async ID collection to main thread only (#5559)"

This reverts commit 5fd6690.

* Revert "Start collecting async IDs in profiles (#5524)"

This reverts commit 9ec7ef9.

* Restore actually useful test code change
@dd-trace-js dd-trace-js bot mentioned this pull request May 1, 2025
szegedi added a commit that referenced this pull request May 1, 2025
* Revert "Constrain async ID collection to main thread only (#5559)"

This reverts commit 5fd6690.

* Revert "Start collecting async IDs in profiles (#5524)"

This reverts commit 9ec7ef9.

* Restore actually useful test code change
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