trace_events: add node.promises category, rejection counter #22124
Conversation
nodejs-github-bot
added
the
c++
src/bootstrapper.cc
Outdated
| @@ -51,6 +51,9 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) { | |||
| } | |||
|
|
|||
| void PromiseRejectCallback(PromiseRejectMessage message) { | |||
| static uint64_t unhandledRejections = 0; | |||
| static uint64_t rejectionsHandledAfter = 0; | |||
There was a problem hiding this comment.
Are these values supposed to be global? Wouldn’t something per-Node.js-instance (aka per-Environment) make more sense?
(If they are supposed to be global: You probably want std::atomic_uint64_t instead.)
There was a problem hiding this comment.
Currently thinking that global is the right thing for now. This is really intended as a kind of spot check... e.g. let's do a really quick check to see the ratio of unhandled rejections to handled after rejections to see if we may have a problem. I don't think we need more detail than that. +1 on std::atomic_uint64_t
|
/cc @nodejs/diagnostics @nodejs/promises-debugging @nodejs/trace-events |
| traces.forEach((trace) => { | ||
| assert.strictEqual(trace.pid, proc.pid); | ||
| assert(trace.name === 'rejections'); | ||
| assert(trace.args.unhandled <= 2); |
There was a problem hiding this comment.
Why are these <= and not ===?
There was a problem hiding this comment.
Because there are two of these that are tested, one with a value of 1 another with a value of 2.
There was a problem hiding this comment.
I'm still confused about why it's done this way rather than two explicit checks, but won't press it
|
Thanks @jasnell, that definitely seems useful. One thing that seems key to make those events actionable would be to include the rejection's error's stack trace in those event log entries. I'm not familiar with trace events in Node.js, is that possible? If that is possible, are there visualization clients that would allow to read/aggregate those stack traces (maybe Chrome)? |
|
/cc @a1ph is expert in tracing. |
|
Right now, collecting the stack trace would be a bit difficult, but it is doable. I'd prefer to explore that separately tho |
Absolutely, I didn't mean that this PR should consider that now, I was just asking questions out of curiosity, and providing user feedback. Thank you again for doing this :) I'm also happy to provide feedback/use cases or anything you'd need from a user's perspective at any time in the future. |
Allow the trace event log to include a count of unhandled rejections and handled after rejections. This is useful primarily as a quick check to see if rejections may be going unhandled (and unnoticed in a process).
jasnell
force-pushed
the
trace-event-promise-rejections
branch
from
2c0a598 to
825e0d2
Compare
Allow the trace event log to include a count of unhandled rejections and handled after rejections. This is useful primarily as a quick check to see if rejections may be going unhandled (and unnoticed in a process). PR-URL: #22124 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
Landed in 080316b |
Allow the trace event log to include a count of unhandled rejections and handled after rejections. This is useful primarily as a quick check to see if rejections may be going unhandled (and unnoticed in a process). PR-URL: #22124 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Allow the trace event log to include a count of unhandled rejections
and handled after rejections. This is useful primarily as a quick
check to see if rejections may be going unhandled (and unnoticed
in a process).
These can be visualized in the Chrome trace events viewer as a stacked graph:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes