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: fix descriptors of events 9 and 23 #5742

Closed

Conversation

joaocgreis
Copy link
Member

Pull Request check-list

  • 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]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

ETW

Description of change

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. However, ETW events 9 and 23 fail to display the EventData (under the Details tab), having instead a ProcessingErrorData section with the data in raw hexadecimal. This happens because the events are not generated correctly.

There are two commits for easy review, I plan to squash and use the following commit message:

etw: fix descriptors of events 9 and 23

Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

cc @nodejs/platform-windows

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

jasnell commented Mar 16, 2016

@bnoordhuis

@bnoordhuis
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

LGTM if CI is green: https://ci.nodejs.org/job/node-test-pull-request/1991/

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@joaocgreis is this appropriate for v4?

@joaocgreis
Copy link
Member Author

CI lint fail unrelated, due to #5823.

@jasnell I think this should go in v4. I'll land it in v4.x-staging as well if there's no objection. cc @nodejs/lts

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@joaocgreis .. SGTM. After you land in v4.x-staging, please remove the lts-watch label and add the land-on-v4 label. Thank you!

@MylesBorins
Copy link
Contributor

@joaocgreis can you hold off on landing on v4.x-staging until after the release of v4.4.1 (which should be tomorrow)

joaocgreis added a commit that referenced this pull request Mar 23, 2016
Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

PR-URL: #5742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Mar 23, 2016
Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

PR-URL: nodejs#5742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joaocgreis
Copy link
Member Author

Landed on master in: 387b6b4

CI for v4.x-staging: https://ci.nodejs.org/job/node-test-commit/2668/

@jasnell jasnell closed this Mar 23, 2016
@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

Closed since it landed in master. Still need to get it landed in v4.x-staging tho. CI looks good. Failure there appears unrelated.

@joaocgreis
Copy link
Member Author

Landed on v4.x-staging in: e7e5af9

Thanks!

@MylesBorins
Copy link
Contributor

I have opted to back this out of v4.x-staging due to the sentiment expressed by the working group in nodejs/Release#90

Will re apply when it has time to mature in v5

@jasnell
Copy link
Member

jasnell commented Mar 30, 2016

+1

evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

PR-URL: #5742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

PR-URL: #5742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 8, 2016
Event 9 must include the string terminator in the last descriptor.
Event 23 must be published with no descriptors, in accordance with
the manifest.

PR-URL: #5742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This is now back in v4.x-staging

@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Notable Changes:

deps:
  - Fix `--gdbjit` for embedders. Backported from v8 upstream.
    (Ben Noordhuis) #5577

etw:
  - Correctly display descriptors for ETW events 9 and 23 on the
    windows platform.
    (João Reis) #5742

querystring:
  - Restore throw when attempting to stringify bad surrogate pair.
    (Brian White) #5858
MylesBorins pushed a commit that referenced this pull request Apr 12, 2016
Notable Changes:

deps:
  - Fix `--gdbjit` for embedders. Backported from v8 upstream.
    (Ben Noordhuis) #5577

etw:
  - Correctly display descriptors for ETW events 9 and 23 on the
    windows platform.
    (João Reis) #5742

querystring:
  - Restore throw when attempting to stringify bad surrogate pair.
    (Brian White) #5858
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