Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
trace_events: add node.promises category, rejection counter#22124
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Aug 4, 2018
jasnell commented Aug 4, 2018
src/bootstrapper.cc Outdated
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 commented Aug 6, 2018
jasnell commented Aug 6, 2018
/cc @nodejs/diagnostics @nodejs/promises-debugging @nodejs/trace-events |
There was a problem hiding this comment.
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 ===?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
benjamingr left a comment
There was a problem hiding this 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 commented Aug 6, 2018
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 commented Aug 6, 2018
/cc @a1ph is expert in tracing. |
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 commented Aug 6, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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).
2c0a598 to 825e0d2Comparejasnell commented Aug 6, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
jasnell commented Aug 6, 2018
mcollina left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
jasnell commented 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 commented Aug 7, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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: nodejs/node#22124 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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