-
Notifications
You must be signed in to change notification settings - Fork 291
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
use AsyncLocalStorage instead of our home-grown solutions #4201
base: master
Are you sure you want to change the base?
Conversation
The comment in the file that selected a storage implementation suggested just using AsyncLocalStorage once it supports triggerAsyncResource(). That said, literally zero of our code uses triggerAsyncResource(), so this is assumed to be historical and no longer relevant. Switching to stock AsyncLocalStorage will enable the usage of TracingChannel in the future.
Overall package sizeSelf size: 6.24 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4201 +/- ##
==========================================
- Coverage 85.23% 84.53% -0.70%
==========================================
Files 247 247
Lines 10956 10956
Branches 33 33
==========================================
- Hits 9338 9262 -76
- Misses 1618 1694 +76 ☔ View full report in Codecov by Sentry. |
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.
profiler test failing otherwise LGTM
Ah, right. Profiler is using those channels to build their code hotspots linkage to trace info. We'll need to coordinate with them to switch that to their own separate use of async_hooks for those lifecycle events. Ideally I'd like to have a better way to do that linking. 🤔 |
We'll obviously have to investigate this, but off the top of your head do you have an inkling what behavioral difference in stock AsyncLocalStorage vs. our current implementation causes the failure? |
Basically you just need to replace your use of |
What does this PR do?
Switches from our custom implementation(s) of AsyncLocalStorage to the one provided by Node.js.
Motivation
There's no need to maintain our own implementation of AsyncLocalStorage when Node.js provides it for us. Also, switching to stock AsyncLocalStorage will enable the usage of TracingChannel in the future.
Additional Notes
The comment in the file that selected a storage implementation suggested just using AsyncLocalStorage once it supports triggerAsyncResource(). That said, literally zero of our code uses triggerAsyncResource(), so this is assumed to be historical and no longer relevant.
Security
Datadog employees:
@DataDog/security-design-and-guidance
.