perf: AsyncLocalStorageRunContextManager #2786

trentm · 2022-06-24T19:41:08Z

Implement a run-context manager using AsyncLocalStorage. Newer versions
of node (>=14.5, >=12.19) should benefit from significant better perf
compared to AsyncHooksRunContextManager, esp. for promise-heavy apps.

See the many async_hooks-related improvements in:
https://nodejs.org/en/blog/release/v14.5.0/
https://nodejs.org/en/blog/release/v12.19.0/

Refs: #2745

Implement a run-context manager using AsyncLocalStorage. Newer versions of node (>=14.5, >=12.19) should benefit from significant better perf compared to AsyncHooksRunContextManager, esp. for promise-heavy apps. See the many async_hooks-related improvements in: https://nodejs.org/en/blog/release/v14.5.0/ https://nodejs.org/en/blog/release/v12.19.0/

ghost · 2022-06-24T20:02:10Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-07-07T19:43:47.957+0000
Duration: 25 min 56 sec

Test stats 🧪

Test	Results
Failed	0
Passed	257312
Skipped	0
Total	257312

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…ode version

…o allow selecting any of the 3 run context tracking techniques. Values are 'patch', 'asynchooks', and 'asynclocalstorage'.

…nterfere with tests

…ontextManager can inherit from *it* and not from BvasicRunContextManager which has _enter/_leave and _stack handling that the ALS class doesn't need

trentm · 2022-07-05T21:24:50Z

@astorm Some review notes that might help. The initial work for this was to 3 files, but that become 27 files after config, docs, and test changes.

Review notes

There are a few parts to this change that can mostly be reviewed separately:

The implementation of the new contextManager=asynclocalstorage|asynchooks|patch config var that deprecates the asyncHooks=true|false config var. This part is all in:
- lib/config.js
- test/config.test.js
- lib/instrumentation/index.js - The selection of the appropriate RunContextManager based on contextManager.
- docs/configuration.asciidoc
The conversion of Jenkins CI to use ELASTIC_APM_CONTEXT_MANAGER rather than ELASTIC_APM_ASYNC_HOOKS.
- .ci/...
The addition of the AsyncLocalStorageRunContextManager class. The class hierarchy is:
```
AbstractRunContextManager
    BasicRunContextManager (used for the non-async_hooks tracking)
        AsyncHooksRunContextManager
    AsyncLocalStorageRunContextManager
```
The implementation of .root() was moved from Basic... up to Abstract...
so that AsyncLocalStorage... could use it without using other unneeded
parts of Basic....
- lib/instrumentation/run-context/AbstractRunContextManager.js
- lib/instrumentation/run-context/BasicRunContextManager.js
Then the implementation of AsyncLocalStorage... is very similar to OTel's
AsyncLocalStorageContextManager.ts.
- lib/instrumentation/run-context/AsyncLocalStorageContextManager.js
The conversion of the test suite to use conf.contextManager rather than conf.asyncHooks:
- test/... (except "test/config.test.js")

…equivalent

astorm

Code looks good and an AsyncLocalStorage option is a welcome change (especially with the potential perf. improvements for Kibana). The class hierarchy makes sense and the configuration looks like it will correctly honor the asyncHooks:false for folks upgrading.

I don't have a lot of insight on the changes to the CI and what context managers are/aren't tested -- so if you have anything you're nervous about there you should probably get a second opinion but I don't have any objections to committing those changes.

astorm · 2022-07-06T20:54:47Z

docs/configuration.asciidoc

+agent supports a limited tracking mechanism (named "patch") that monkey-patches
+much of the core Node.js API to attempt to follow asynchronous calls. The
+"patch" mechanism is limited in usage and coverage (for example, it cannot
+track `async/await` usage), is not recommended, and may be deprecated and


I'd +1 deprecating patch now to give us/users a long run way in it going away forever.

@astorm I've done so in commit 8af9716. Does that commit look reasonable to you?

astorm · 2022-07-06T22:54:05Z

lib/instrumentation/index.js

-  } else {
+  // Select the appropriate run-context manager.
+  const confContextManager = this._agent._conf.contextManager
+  if (confContextManager === config.CONTEXT_MANAGER_PATCH) {


👍 Appears to preserve a user's desires when asyncHooks: false is set to false with no other configuration -- so we should be good here from a backwards compatibility point of view) (i.e uses the basic run context manager)

astorm · 2022-07-06T23:03:27Z

lib/instrumentation/run-context/BasicRunContextManager.js

 // A basic manager for run context. It handles a stack of run contexts, but does
 // no automatic tracking (via async_hooks or otherwise). In combination with
-// "patch-async.js" it does an adequate job of context tracking for much of the
+// "patch-async.js" it does a limited job of context tracking for much of the


🔥 🔥 🔥 🔥

trentm self-assigned this Jun 24, 2022

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jun 24, 2022

trentm added 11 commits June 24, 2022 13:47

automatically choose appropriate async run-context manager based on n…

64626df

…ode version

fix missing import

9e7d89c

Merge branch 'main' into trentm/kperf2

d58757b

add lic header

8025493

first stab at a 'contextManager' config var to replace 'asyncHooks' t…

6029561

…o allow selecting any of the 3 run context tracking techniques. Values are 'patch', 'asynchooks', and 'asynclocalstorage'.

drop debugging .only; ensure CI-set ELASTIC_APM_ASYNC_HOOKS doesn't i…

e82c9eb

…nterfere with tests

finish configuration doc updates; add changelog entry

34202d9

test updates for the switch to 'contextManager'

6848cca

Merge branch 'main' into trentm/kperf2

64cca27

move .root() up to AbstractRunContextManager so AsyncLocalStorageRunC…

055795e

…ontextManager can inherit from *it* and not from BvasicRunContextManager which has _enter/_leave and _stack handling that the ALS class doesn't need

this isn't an issue. the RunContext's stack handles this

016fff3

trentm marked this pull request as ready for review July 5, 2022 21:23

trentm requested a review from astorm July 5, 2022 21:23

update another mention of asyncHooks=false to the new contextManager …

61ffc18

…equivalent

astorm approved these changes Jul 6, 2022

View reviewed changes

trentm added 2 commits July 7, 2022 10:36

deprecate 'patch' run-context tracking as part of this change

8af9716

fix docs reference

ae4d4ac

trentm merged commit 1fe68b7 into main Jul 7, 2022

trentm deleted the trentm/kperf2 branch July 7, 2022 21:37

trentm added this to the 8.4 milestone Jul 14, 2022

This was referenced Jul 14, 2022

Performance investigation #2745

Closed

3.37.0 #2830

Merged

Does APM contextPropagationOnly limit server capacity? elastic/kibana#129585

Closed

Update dependency elastic-apm-node to ^3.37.0 (main) elastic/kibana#136657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: AsyncLocalStorageRunContextManager #2786

perf: AsyncLocalStorageRunContextManager #2786

Uh oh!

trentm commented Jun 24, 2022 •

edited

Loading

Uh oh!

ghost commented Jun 24, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

Uh oh!

trentm commented Jul 5, 2022 •

edited by astorm

Loading

Uh oh!

astorm left a comment

Uh oh!

astorm Jul 6, 2022

Uh oh!

trentm Jul 7, 2022

Uh oh!

astorm Jul 6, 2022

Uh oh!

astorm Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

perf: AsyncLocalStorageRunContextManager #2786

perf: AsyncLocalStorageRunContextManager #2786

Uh oh!

Conversation

trentm commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 24, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

trentm commented Jul 5, 2022 • edited by astorm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review notes

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

astorm Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

trentm Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

astorm Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

astorm Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trentm commented Jun 24, 2022 •

edited

Loading

ghost commented Jun 24, 2022 •

edited by ghost

Loading

trentm commented Jul 5, 2022 •

edited by astorm

Loading