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

etw: add event messages #5936

Closed

Conversation

joaocgreis
Copy link
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

ETW

Description of change

Note: #5657 should land before this, because this does not include the generated files.

A log of ETW events can be produced with:

logman start NodejsDCS -p NodeJS-ETW-provider -o node_log.etl -ets
:: Do something with node.exe
logman stop NodejsDCS -ets

The resulting etl file can be open in Event Viewer.

Without this change, the description box shows an error message:

The description for Event ID 23 from source NodeJS-ETW-provider cannot be found. Either the component that raises this event is not installed on your local computer or the installation is corrupted. You can install or repair the component on the local computer.

If the event originated on another computer, the display information had to be saved with the event.

The following information was included with the event: 


The message id for the desired message could not be found

This adds descriptions to all events, replacing the error message in the Event Viewer.

etw

cc @nodejs/platform-windows

@joaocgreis joaocgreis added the windows Issues and PRs related to the Windows platform. label Mar 28, 2016
@bnoordhuis
Copy link
Member

LGTM. Just a suggestion: "Method Load" is kind of a bad name, "Function Compiled" is closer to the truth.

joaocgreis added a commit that referenced this pull request Mar 31, 2016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #5936
@joaocgreis
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2110/ all ok except a slave failure

Landed in: 1845c4f

Thanks @bnoordhuis !

@joaocgreis joaocgreis closed this Mar 31, 2016
@benjamingr
Copy link
Member

Thanks for the fix - I tested this locally and I like the change, forgot to comment.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

is this relevant to v4 at all?

@joaocgreis
Copy link
Member Author

@jasnell I don't think so, this is a new feature.

Added the don't land label, feel free to remove if that was not the correct thing or not consensual.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

Works for me.

MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #5936
This was referenced Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants