feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

vmarchaud · 2020-09-13T12:18:13Z

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

codecov · 2020-09-13T12:26:17Z

Codecov Report

Merging #1525 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            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

Impacted Files	Coverage Δ
...kages/opentelemetry-node/src/NodeTracerProvider.ts	`96.55% <83.33%> (-3.45%)`	⬇️

…rting Node 14.8

obecny · 2020-09-14T18:09:07Z

packages/opentelemetry-node/test/registration.test.ts

 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')


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 ?

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.

sry haven't spotted that

johanneswuerbach · 2020-09-15T09:36:30Z

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.

vmarchaud · 2020-09-15T12:47:11Z

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.

@johanneswuerbach I stated why i didn't include it on the PR description:

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

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

johanneswuerbach · 2020-09-16T11:02:39Z

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.

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

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.

obecny

lgtm

vmarchaud · 2020-09-16T14:07:06Z

What I'm not sure about is how long I would support a specific minor/patch version of a major release line.

Not sure to follow here, we would just support all versions starting the 14.8.0 ?

dyladan · 2020-09-21T17:57:36Z

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.

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

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.

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.

johanneswuerbach · 2020-09-21T22:15:47Z

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

johanneswuerbach · 2020-09-22T19:32:35Z

Looks like >= 12.18.3 has a bug-free AsyncLocalStorage implementation nodejs/node#35287 (comment) and 12.19 will mainly get some performance fixes nodejs/node#35287 (comment)

Would it make sense to also enable AsyncLocalStorageContextManager for >= 12.18.3 then?

dyladan · 2020-09-23T19:56:10Z

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.

vmarchaud requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny and OlivierAlbertini as code owners September 13, 2020 12:18

vmarchaud self-assigned this Sep 13, 2020

vmarchaud added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 13, 2020

feat(node-tracer): use AsyncLocalStorageContextManager by default sta…

75e551d

…rting Node 14.8

vmarchaud force-pushed the use-als-by-default branch from dd701fc to 75e551d Compare September 13, 2020 12:33

obecny reviewed Sep 14, 2020

View reviewed changes

obecny approved these changes Sep 16, 2020

View reviewed changes

Merge branch 'master' into use-als-by-default

7834677

dyladan approved these changes Sep 21, 2020

View reviewed changes

johanneswuerbach approved these changes Sep 21, 2020

View reviewed changes

mwear approved these changes Sep 22, 2020

View reviewed changes

Merge branch 'master' into use-als-by-default

02c2997

markwolff approved these changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into use-als-by-default

ec17416

dyladan merged commit aac3f56 into open-telemetry:master Oct 1, 2020

dyladan added the enhancement New feature or request label Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

vmarchaud commented Sep 13, 2020 •

edited

codecov bot commented Sep 13, 2020 •

edited

obecny Sep 14, 2020

dyladan Sep 14, 2020

obecny Sep 16, 2020

johanneswuerbach commented Sep 15, 2020

vmarchaud commented Sep 15, 2020 •

edited

johanneswuerbach commented Sep 16, 2020 •

edited

obecny left a comment

vmarchaud commented Sep 16, 2020

dyladan commented Sep 21, 2020

johanneswuerbach commented Sep 21, 2020 •

edited

johanneswuerbach commented Sep 22, 2020 •

edited

dyladan commented Sep 23, 2020

feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

Conversation

vmarchaud commented Sep 13, 2020 • edited

codecov bot commented Sep 13, 2020 • edited

Codecov Report

obecny Sep 14, 2020

Choose a reason for hiding this comment

dyladan Sep 14, 2020

Choose a reason for hiding this comment

obecny Sep 16, 2020

Choose a reason for hiding this comment

johanneswuerbach commented Sep 15, 2020

vmarchaud commented Sep 15, 2020 • edited

johanneswuerbach commented Sep 16, 2020 • edited

obecny left a comment

Choose a reason for hiding this comment

vmarchaud commented Sep 16, 2020

dyladan commented Sep 21, 2020

johanneswuerbach commented Sep 21, 2020 • edited

johanneswuerbach commented Sep 22, 2020 • edited

dyladan commented Sep 23, 2020

vmarchaud commented Sep 13, 2020 •

edited

codecov bot commented Sep 13, 2020 •

edited

vmarchaud commented Sep 15, 2020 •

edited

johanneswuerbach commented Sep 16, 2020 •

edited

johanneswuerbach commented Sep 21, 2020 •

edited

johanneswuerbach commented Sep 22, 2020 •

edited