Skip to content

remove async storage from fastify instrumentation #6215

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
Aug 1, 2025

Conversation

wconti27
Copy link
Contributor

@wconti27 wconti27 commented Aug 1, 2025

What does this PR do?

removes async resource storage usage from fastify instrumentation for Node24 compatibility

Motivation

Plugin Checklist

Additional Notes

@wconti27 wconti27 requested review from a team as code owners August 1, 2025 14:54
@wconti27 wconti27 self-assigned this Aug 1, 2025
@pr-commenter
Copy link

pr-commenter bot commented Aug 1, 2025

Benchmarks

Benchmark execution time: 2025-08-01 16:53:03

Comparing candidate commit ed80ffc in PR branch conti/remove-async-storage-fastify with baseline commit 7b9b5cf in branch master.

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

Copy link

github-actions bot commented Aug 1, 2025

Overall package size

Self size: 11.13 MB
Deduped: 110.71 MB
No deduping: 111.1 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.0.1 | 20.3 MB | 20.3 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.9.0 | 9.77 MB | 10.14 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 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 | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.2 | 122.36 kB | 850.93 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 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 | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 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.4 | 3.96 kB | 3.96 kB |

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

@wconti27 wconti27 mentioned this pull request Jul 31, 2025
61 tasks
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.24%. Comparing base (7b9b5cf) to head (ed80ffc).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
packages/datadog-plugin-fastify/src/tracing.js 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6215      +/-   ##
==========================================
+ Coverage   83.15%   83.24%   +0.08%     
==========================================
  Files         477      477              
  Lines       19749    19759      +10     
==========================================
+ Hits        16422    16448      +26     
+ Misses       3327     3311      -16     

☔ 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.

publishError(err, req)

const hasCookies = request.cookies && Object.keys(request.cookies).length > 0
arguments[arguments.length - 1] = addHookStartCh.runStores(ctx, () => {
Copy link
Member

Choose a reason for hiding this comment

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

This hook is not needed as the previous code created the resource right before doneCallback was called, so basically the addHookFinishCh is enough (although I would also argue the name is confusing as addHook is called when the app starts, and the doneCallback is called on every request, so it's not happening at the same time as adding a hook at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive removed the start runStores call, and changed the naming to be callback:exec for the channel. Should be good 👍

@rochdev
Copy link
Member

rochdev commented Aug 1, 2025

Why is this PR marked as semver-minor instead of semver-patch?

@wconti27
Copy link
Contributor Author

wconti27 commented Aug 1, 2025

Why is this PR marked as semver-minor instead of semver-patch?

fixed.

@wconti27 wconti27 requested a review from rochdev August 1, 2025 16:59
@wconti27 wconti27 merged commit 044c5da into master Aug 1, 2025
686 checks passed
@wconti27 wconti27 deleted the conti/remove-async-storage-fastify branch August 1, 2025 20:07
dd-octo-sts bot pushed a commit that referenced this pull request Aug 2, 2025
@dd-octo-sts dd-octo-sts bot mentioned this pull request Aug 2, 2025
BridgeAR pushed a commit that referenced this pull request Aug 6, 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.

2 participants