Skip to content
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

Retain async context when notifying PerformanceObservers #36343

Closed

Conversation

ZauberNerd
Copy link
Contributor

@ZauberNerd ZauberNerd commented Dec 1, 2020

Problem description:
Using the perf_hooks module (specifically the User Timing API), to collect timings in an async context (i.e. to collect metrics during the lifetime of an HTTP request), it is not possible to correlate the PerformanceEntrys with a single request (except for using workarounds, such as prefixing all mark names with a unique id).
This is required, when implementing e.g. tracing solutions, where one wants to have timings per requests.

Solution:
With the changes in this PR we are able to retain async contexts when calling the PerformanceObserver, which enables us to, for example, access an AsyncLocalStorage in the observer callback to store the collected metrics per request.

Additional notes:

  • The above mentioned solution won't work when the PerformanceObserver is buffered.

Questions:

  • What, if anything, should go into the documentation? I suppose we should mention that buffered observers do not retain the async context?
  • Would this change get back-ported to the v12 release?
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Dec 1, 2020
@jasnell jasnell requested a review from addaleax December 1, 2020 16:39
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll mark this as request changes only because it already has an approval but still needs tests in order to land :) But I’m fine with the approach here

src/node.h Outdated Show resolved Hide resolved
src/api/callback.cc Show resolved Hide resolved
@ZauberNerd ZauberNerd force-pushed the retain-async-context-perf-hooks branch from d5353bf to f24f3fa Compare December 2, 2020 22:10
@ZauberNerd ZauberNerd force-pushed the retain-async-context-perf-hooks branch from f24f3fa to 88d44a7 Compare December 2, 2020 22:41
@ZauberNerd
Copy link
Contributor Author

Hi @addaleax could you please take another look here?
I've added a test and @nicstange answered your comment with a question: #36343 (comment) it would be great if you could give us some directions on what to do :)

@ZauberNerd ZauberNerd force-pushed the retain-async-context-perf-hooks branch from 88d44a7 to 64e45a9 Compare December 4, 2020 22:59
@ZauberNerd ZauberNerd force-pushed the retain-async-context-perf-hooks branch from 64e45a9 to dbe0e87 Compare December 5, 2020 09:45
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2020
@nodejs-github-bot
Copy link
Collaborator

nicstange and others added 4 commits December 11, 2020 04:43
There are situations where one wants to invoke a JS callback's ->Call()
from C++ and in particular retain any existing async_context state, but
where it's not obvious that a plain ->Call() would be safe at the point
in question.

Such callsites usually resort to
node::MakeCallback(..., async_context{0, 0}), which unconditionally
pushes the async_context{0, 0} and takes the required provisions for the
->Call() itself such as triggering the tick after its return, if needed.

An example would be the PerformanceObserver invocation from
PerformanceEntry::Notify(): this can get called when coming from JS
through e.g. perf_hooks.performance.mark() and alike, but perhaps also
from nghttp2 (c.f. EmitStatistics() in node_http2.cc).

In the former case, a plain ->Call() would be safe and it would be
desirable to retain the current async_context so that
PerformanceObservers can access it resp. the associated
AsyncLocalStorage. However, in the second case the additional provisions
taken by node::MakeCallback() might potentially be strictly required.

So PerformanceEntry::Notify() bites the bullet and invokes the
PerformanceObservers through node::MakeCallback() unconditionally,
thereby always rendering any possibly preexisting async_context
inaccessible.

Introduce the convenience node::MakeSyncCallback() for such usecases,
which would basically forward to ->Call() if safe and to
node::MakeCallback(..., async_context{0, 0}) otherwise.

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
It's desirable to retain async_contexts active at callsites of
perf_hooks.performance.mark() and alike in the subsequent
PerformanceObserver invocations such that the latter can access e.g.
associated AsyncLocalStorage instances.

In working towards this goal replace the node::MakeCallback(...,
async_context{0, 0}) in PerformanceEntry::doNotify() by the new
node::MakeSyncCallback() introduced specifically for this purpose.

This change will retain the original async_context, if any, in
perf_hook's observersCallback() and thus, for the subsequent doNotify()
on unbuffered PerformanceObservers.

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This reverts commit 009e418.

AFAIU the discussion at [1], PerformanceObserver had been made to
inherit from AsyncResource more or less as a band-aid in lack of a
better async_context candidate to invoke it in. In order to enable
access to AsyncLocalStores from PerformanceObservers invoked
synchronously through e.g. measure() or mark(), the current
async_context, if any, should be retained.

Note that this is a breaking change, but
- as has been commented at [1], PerformanceObserver being derived from
  AsyncResource is a "minor divergence from the spec" anyway,
- to my knowledge this is an internal implementation detail which has
  never been documented and
- I can't think of a good reason why existing PerformanceObserver
  implementations would possibly rely on it.

OTOH, it's probably worthwhile to not potentially invoke before() and
after() async_hooks for each and every PerformanceObserver notification.

[1] nodejs#18789

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This test proves that the PerformanceObserver callback gets called with
the async context of the callsite of performance.mark()/measure() and
therefore AsyncLocalStorage can be used inside a PerformanceObserver.

PR: nodejs#36343

PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the retain-async-context-perf-hooks branch from dbe0e87 to ef0f5b1 Compare December 11, 2020 12:47
@Trott
Copy link
Member

Trott commented Dec 11, 2020

Landed in f49cef5...ef0f5b1

@Trott Trott closed this Dec 11, 2020
targos pushed a commit that referenced this pull request Dec 21, 2020
It's desirable to retain async_contexts active at callsites of
perf_hooks.performance.mark() and alike in the subsequent
PerformanceObserver invocations such that the latter can access e.g.
associated AsyncLocalStorage instances.

In working towards this goal replace the node::MakeCallback(...,
async_context{0, 0}) in PerformanceEntry::doNotify() by the new
node::MakeSyncCallback() introduced specifically for this purpose.

This change will retain the original async_context, if any, in
perf_hook's observersCallback() and thus, for the subsequent doNotify()
on unbuffered PerformanceObservers.

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: #36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Dec 21, 2020
This reverts commit 009e418.

AFAIU the discussion at [1], PerformanceObserver had been made to
inherit from AsyncResource more or less as a band-aid in lack of a
better async_context candidate to invoke it in. In order to enable
access to AsyncLocalStores from PerformanceObservers invoked
synchronously through e.g. measure() or mark(), the current
async_context, if any, should be retained.

Note that this is a breaking change, but
- as has been commented at [1], PerformanceObserver being derived from
  AsyncResource is a "minor divergence from the spec" anyway,
- to my knowledge this is an internal implementation detail which has
  never been documented and
- I can't think of a good reason why existing PerformanceObserver
  implementations would possibly rely on it.

OTOH, it's probably worthwhile to not potentially invoke before() and
after() async_hooks for each and every PerformanceObserver notification.

[1] #18789

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: #36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Dec 21, 2020
This test proves that the PerformanceObserver callback gets called with
the async context of the callsite of performance.mark()/measure() and
therefore AsyncLocalStorage can be used inside a PerformanceObserver.

PR: #36343

PR-URL: #36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants