-
Notifications
You must be signed in to change notification settings - Fork 810
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
feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1525 +/- ##
==========================================
- Coverage 92.92% 92.91% -0.02%
==========================================
Files 156 156
Lines 4851 4855 +4
Branches 978 980 +2
==========================================
+ Hits 4508 4511 +3
- Misses 343 344 +1
|
dd701fc
to
75e551d
Compare
import { NoopContextManager } from '@opentelemetry/context-base'; | ||
import { CompositePropagator } from '@opentelemetry/core'; | ||
import * as assert from 'assert'; | ||
import { NodeTracerProvider } from '../src'; | ||
import * as semver from 'semver'; | ||
|
||
const DefaultContextManager = semver.gte(process.version, '14.8.0') |
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.
should we rather check if AsyncLocalStorageContextManager is available (feature detection) instead of checking the version using semver. If you already require that it means it will be undefined for lower versions
const DefaultContextManager = AsyncLocalStorageContextManager || AsyncHooksContextManager
should be enough ?
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.
In the description he mentioned that there is a bug in versions before 14.8. Feature detection would still detect that local storage is available, but he suggests we would want to use the async hooks version anyways.
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.
sry haven't spotted that
Any reason why Node 12 is excluded? We run AsyncLocalStorageContextManager successfully on Node 12 for a couple of months and the performance improvements should be comparable to the AsyncHooksContextManager. Node 12 will be supported for ~1,5 years so I guess it could be worth it. |
@johanneswuerbach I stated why i didn't include it on the PR description:
I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ? |
Sorry I had assumed nodejs/node#34573 was already back-ported to Node 12, but you are right it isn't :-( Seeing that I would agree with Node 14+ only for now.
What I'm not sure about is how long I would support a specific minor/patch version of a major release line. Should there be 1 month post release, 3 months, forever? I would expect people to follow minors in their major lines relatively closely to also rollout security fixes quickly (e.g. the just release 14.11.0 fixes a critical and a high vulnerability), but I'm not sure how realistic this is and whether it is okay for this project to require user to be on a recent minor version. If different minor versions have require different behaviour than those should also be covered in CI, which would increase test runtime and costs. Overall I guess it depends more on the project support agreement then on me, I'm fine with tracking the latest of each major line, but we also invested time and effort in my company that we can and do adopt new minors within 24-48h post release. |
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.
lgtm
Not sure to follow here, we would just support all versions starting the 14.8.0 ? |
As stated by @vmarchaud, this is a minimum version not the only minor version supported. Future minor versions and even future major versions will still use the new manager. |
Looks like the PR that broke v14 was actually not back-ported to v12 yet, so v12 shouldn't be broken. Nevertheless I requested a fix to be back-ported to v12 nodejs/node#35287 |
Looks like Would it make sense to also enable |
Discussed at SIG today, enabling for 14.8+ only is fine. It is a simple rule that is easy to document. Users on 12 who need the performance boost can always enable whatever context manager they want manually if they'd like. |
Even though AsyncLocalStorage was added before (and its have been backported in 12.x), there was some important bug fixes released for 14.8 (see https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V14.md#commits-3). Specially those one: nodejs/node#34573 and nodejs/node#34573
Fixes #1511