FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

trace_events: add node.promises category, rejection counter by jasnell · Pull Request #22124 · nodejs/node · GitHub

/ node Public

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

Closed
jasnell wants to merge 4 commits into
nodejs:masterfrom
jasnell:trace-event-promise-rejections
ConversationCommits (4)ChecksFiles changed
Closed

trace_events: add node.promises category, rejection counter#22124
jasnell wants to merge 4 commits into
nodejs:masterfrom
jasnell:trace-event-promise-rejections

Conversation

jasnell commented Aug 4, 2018

Copy link
Copy Markdown
Member

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot commented Aug 4, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/422/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 4, 2018

jasnell commented Aug 4, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16187/

addaleax reviewed Aug 4, 2018
View reviewed changes
Comment thread src/bootstrapper.cc Outdated

addaleax Aug 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell Aug 4, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16234/

jasnell commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

/cc @nodejs/diagnostics @nodejs/promises-debugging @nodejs/trace-events

benjamingr reviewed Aug 6, 2018
View reviewed changes
Comment thread test/parallel/test-trace-event-promises.js Outdated

benjamingr Aug 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why are these <= and not ===?

jasnell Aug 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

benjamingr Aug 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

benjamingr approved these changes Aug 6, 2018
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

/cc @a1ph is expert in tracing.

jasnell commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown

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

jasnell added 2 commits August 6, 2018 16:09
trace_events: add node.promises category, rejection counter
9ca9452 
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).
[Squash] use atomic_uint64_t
825e0d2
jasnell force-pushed the trace-event-promise-rejections branch from 2c0a598 to 825e0d2 Compare August 6, 2018 23:22

jasnell commented Aug 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16238/

jasnell commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16239/

mcollina approved these changes Aug 7, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jasnell commented Aug 7, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16250/

jasnell added a commit that referenced this pull request Aug 7, 2018
trace_events: add node.promises category, rejection counter
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>

jasnell commented Aug 7, 2018

Copy link
Copy Markdown
Member Author

Landed in 080316b

jasnell closed this Aug 7, 2018
targos pushed a commit that referenced this pull request Aug 11, 2018
trace_events: add node.promises category, rejection counter
fd318e7 
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>
rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
trace_events: add node.promises category, rejection counter
ad46cca 
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 <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
benjamingr benjamingr approved these changes
mcollina mcollina approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL