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

src: expose WriteNodeReport to embedders #55076

Closed
wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

Refs electron/electron#43774

This PR allows creating Node.js diagnostic reports from Electron signal handlers. It's currently used by Electron's Utility Process to support error events.

cc @deepak1556

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. labels Sep 23, 2024
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (2cec716) to head (76a7ec4).
Report is 28 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55076   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files         652      652           
  Lines      183911   183920    +9     
  Branches    35858    35863    +5     
=======================================
+ Hits       162271   162290   +19     
  Misses      14916    14916           
+ Partials     6724     6714   -10     
Files with missing lines Coverage Δ
src/node_report.cc 93.15% <100.00%> (ø)
src/node_report.h 100.00% <ø> (ø)

... and 24 files with indirect coverage changes

Refs electron/electron#43774

Allows creating Node.js diagnostic reports from Electron signal handlers.
Currently used by Utility process to support error event.
@codebytere codebytere changed the title src: expose report writing entrypoint to embedders src: expose WriteNodeReport to embedders Sep 23, 2024
@codebytere codebytere changed the title src: expose WriteNodeReport to embedders src: expose WriteNodeReport to embedders Sep 23, 2024
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member

joyeecheung commented Sep 23, 2024

I don't know about the report stuff too well, but the current API looks a bit too verbose to expose to node.h and we might end up changing the signature for some internal use (which would make it semver-major). Are all of the parameters needed by Electron? Can we expose a simplified version of it with only the bits that need to be customized to be exposed as parameters in the public API?

@joyeecheung
Copy link
Member

joyeecheung commented Sep 23, 2024

Also we can consider taking a struct for the parameters, that'll allow us to add more parameters without making the public API changes semver-major due to ABI compatibility breakage.

@deepak1556
Copy link
Contributor

Thanks @codebytere for starting this thread!

@joyeecheung you are right not all parameters are needed today, we would like to capture diagnostics from FatalErrorCallback and eventually might extend it to OOMErrorCallback both of which only need the following params,

Isolate* isolate,
Environment* env,
const char* message,
const char* trigger,
std::ostream& out

Also having an extendable way to exclude parts of the report maybe via enum would be helpful,

enum ExcludeFlags : uint32_t {
  kNativeStacks = 0,
  kNetworkInterfaces = 1 << 0,
...
};

For example in our case nativeStacks doesn't offer much value since we don't have any debug symbols in the build and the crashdumps generated by https://chromium.googlesource.com/crashpad/crashpad/+/main/README.md would be our source of truth.

@addaleax
Copy link
Member

but the current API looks a bit too verbose to expose to node.h

@joyeecheung I agree with this but just to be clear, that's not what this PR is doing – we're not really providing any guarantees, neither ABI nor API stability, to Electron here. This is just making an internal API accessible when using NODE_WANT_INTERNALS.

(I do think that Electron should put some effort into stopping to do that. But there have been PRs like this for years and I don't think expect any changes here.)

@joyeecheung
Copy link
Member

I agree with this but just to be clear, that's not what this PR is doing

Oh right, I misread, this is adding it to node_report.h, not node.h

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM if this makes it easier for Electron to resolve conflicts. Though I agree with @addaleax that reducing the usage of NODE_WANT_INTERNALS would be ideal.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

There is a node::GetNodeReport public API in node.h already. It should provide the functionality.

@deepak1556
Copy link
Contributor

Thanks @legendecas, somehow I missed that. It is sufficient for our use case.

@codebytere codebytere closed this Sep 25, 2024
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++. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants