-
Notifications
You must be signed in to change notification settings - Fork 323
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
Remove legacyStorage in favor of namespaced storages #5206
Conversation
Overall package sizeSelf size: 8.63 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.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 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 | | ttl-set | 1.0.0 | 4.61 kB | 9.69 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5206 +/- ##
==========================================
- Coverage 81.16% 81.12% -0.05%
==========================================
Files 482 481 -1
Lines 21522 21448 -74
==========================================
- Hits 17468 17399 -69
+ Misses 4054 4049 -5 ☔ View full report in Codecov by Sentry. |
Datadog ReportBranch report: ✅ 0 Failed, 628 Passed, 0 Skipped, 12m 19.57s Total Time |
BenchmarksBenchmark execution time: 2025-02-06 02:02:04 Comparing candidate commit 7b79ef7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 911 metrics, 22 unstable metrics. |
e48ac5a
to
20d81a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First thank you for taking on this massive PR to address tech debt.
With that said, one of the goals of this API is to decouple storages. Using a shared constant actually introduces coupling. It also differs of usage in other places for the similar channel
API. A more consistent pattern would be to simply use a string, and possibly combining that with a single instance per file (or even an instance shared in multiple files of the same component). This would also help with performance by avoiding multiple lookups at runtime. Basically, a storage instance should be ideally looked up only at most once per file, and it should not pollute a core module with information about the caller.
It's also worth noting that removing getHandle
would require an actual span store, which this isn't. In that sense even the namespace name is incorrect. It would be called something like "default" or "global", but I think calling it "legacy" explicitly would be better to make it obvious that its usage going forward is discouraged and alternatives are to be considered.
Bypassing merge protections bc mongoose is a broken test (cc @rochdev ) |
What does this PR do?
legacyStorage
in favor of a namespaced'legacy'
storagestorage.js
so my IDE knows how to code for meMotivation
This gets us one step closer to getting rid of
getHandle
as we are now fully converted to namespaced storagesPlugin Checklist
Additional Notes