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

trace_events: add node.promises category, rejection counter #22124

Closed
wants to merge 4 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 4, 2018

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:

image

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 the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 4, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 4, 2018

@@ -51,6 +51,9 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
}

void PromiseRejectCallback(PromiseRejectMessage message) {
static uint64_t unhandledRejections = 0;
static uint64_t rejectionsHandledAfter = 0;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

/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);
Copy link
Member

Choose a reason for hiding this comment

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

Why are these <= and not ===?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there are two of these that are tested, one with a value of 1 another with a value of 2.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still confused about why it's done this way rather than two explicit checks, but won't press it

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Sure and LGTM, definitely sounds like something that's interesting to have in trace_events, might want to add more details about the rejections at a later PR.

@misterdjules
Copy link

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

@alexkozy
Copy link
Member

alexkozy commented Aug 6, 2018

/cc @a1ph is expert in tracing.

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

Right now, collecting the stack trace would be a bit difficult, but it is doable. I'd prefer to explore that separately tho

@misterdjules
Copy link

misterdjules commented Aug 6, 2018

@jasnell

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 jasnell force-pushed the trace-event-promise-rejections branch from 2c0a598 to 825e0d2 Compare August 6, 2018 23:22
@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Aug 7, 2018

jasnell added a commit that referenced this pull request Aug 7, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 7, 2018

Landed in 080316b

@jasnell jasnell closed this Aug 7, 2018
targos pushed a commit that referenced this pull request Aug 11, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants