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.

Sign up for GitHub

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

Jump to bottom

trace_events: fix trace events JS API not writing traces #24945

Closed
wants to merge 1 commit into from
Closed

trace_events: fix trace events JS API not writing traces #24945

wants to merge 1 commit into from

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Dec 10, 2018 •
edited
Loading

Fixes #24944

I believe Inspector-based tracing probably faces the same issue, but I wasn't yet able to verify it because of a possibly unrelated V8 issue. So this PR only fixes the JS API. (edit: it seems that this does not affect inspector as it uses its own trace writer)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 10, 2018
@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Dec 10, 2018
addaleax
addaleax approved these changes Dec 10, 2018
View reviewed changes
src/node_internals.h Outdated Show resolved Hide resolved
src/node_trace_events.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

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

@kjin
Copy link
Contributor Author

kjin commented Dec 10, 2018

@addaleax BTW, should a PR like this have a src or a tracing prefix? I wasn't sure what to put.

@addaleax
Copy link
Member

@kjin I think tracing: would be good, yes… I don’t think it would be an issue if you keep it though :)

@joyeecheung
Copy link
Member

@kjin Currently core-validate-commit only supports trace_events

https://github.com/nodejs/core-validate-commit/blob/b3a89a152e5c144d49918551fdc3a5bad36f73b0/lib/rules/subsystem.js#L61

@richardlau
Copy link
Member

Does the test in the first commit fail unless the second commit is applied? If so then we probably want to squash on landing (or swap the ordering) to avoid issues with future git bisects.

@kjin
Copy link
Contributor Author

kjin commented Dec 11, 2018 •
edited
Loading

@joyeecheung I've seen a mix of tracing and trace_events, I assumed the former would apply to C++ and the latter would apply to the JS API. I'll try to use trace_events

@richardlau Yes, it does. Good idea, I'll do that. (It looks like the GitHub UI still shows them in the former order, though)

@addaleax I fixed an issue in my test which should resolve the currently failing CI.

@kjin kjin changed the title src: fix trace events JS API not writing traces trace_events: fix trace events JS API not writing traces Dec 11, 2018
@kjin
Copy link
Contributor Author

kjin commented Dec 11, 2018

@joyeecheung It looks like using trace_events is causing the commit message linter to emit an error. Do you know why this might be the case?

@richardlau
Copy link
Member

@joyeecheung It looks like using trace_events is causing the commit message linter to emit an error. Do you know why this might be the case?

@kjin You missed off the : after trace_events.

@kjin
Copy link
Contributor Author

kjin commented Dec 11, 2018

@richardlau Thanks for the catch (again) :)

@addaleax
Copy link
Member

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

Trott
Trott previously requested changes Dec 16, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

CI failures on Windows, AIX, and SmartOS all seem to be relevant. (Collaborators, feel free to dismiss this review once addressed. I'm only leaving this here to someone doesn't land this in error.)

@kjin
Copy link
Contributor Author

kjin commented Dec 18, 2018

On Windows there seems to be an issue with the temporary directory not being refreshed due to being a locked resource (EBUSY). I'm working through this and should hopefully get a fix in within the next day.

@kjin
Copy link
Contributor Author

kjin commented Dec 18, 2018 •
edited
Loading

@Trott I believe I've fixed the issue (tested it on Windows). (It was because my test was trying to delete the temp directory when it was the parent process's cwd)

@Trott
Copy link
Member

Trott commented Dec 22, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/19750/ ✔️

@Trott Trott dismissed their stale review December 22, 2018 17:35

test fixed; could use a re-review or two, though

@kjin kjin force-pushed the trace-events-fix branch from fde17fd to 14796a8 Compare January 2, 2019 23:30
@kjin
Copy link
Contributor Author

kjin commented Jan 2, 2019

I've rebased to resolve conflicts. cc @addaleax

@vmarchaud vmarchaud mentioned this pull request Jan 26, 2019
Merged
2 tasks
@vmarchaud
Copy link
Contributor

@kjin could you rebase again so this PR can go forward ? Thanks !

@kjin kjin force-pushed the trace-events-fix branch from 14796a8 to eff875b Compare February 5, 2019 23:28
@kjin
Copy link
Contributor Author

kjin commented Feb 5, 2019

@vmarchaud I've rebased the PR!

@ofrobots
Copy link
Contributor

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

@kjin
trace_events: fix trace events JS API not writing traces
6d6d36f 
The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the case where no CLI
args are provided. This commit adds that test.

Fixes nodejs#24944
@ofrobots
Copy link
Contributor

The CI is green and this is good to go. However, I think the commits needs to be squashed. The latter is not a test-only change is dependent/overlapping with the former.

@ofrobots
Copy link
Contributor

Landed in 582c0d5

@ofrobots ofrobots closed this Feb 14, 2019
ofrobots pushed a commit that referenced this pull request Feb 14, 2019
@kjin @ofrobots
trace_events: fix trace events JS API writing
582c0d5 
The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the
casewhere no CLI args are provided. This commit adds that test.

Fixes #24944

PR-URL: #24945
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
targos pushed a commit that referenced this pull request Feb 15, 2019
@kjin @targos
trace_events: fix trace events JS API writing
e74541e 
The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the
casewhere no CLI args are provided. This commit adds that test.

Fixes #24944

PR-URL: #24945
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@kjin @rvagg
trace_events: fix trace events JS API writing
1766b8c 
The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the
casewhere no CLI args are provided. This commit adds that test.

Fixes #24944

PR-URL: #24945
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@ofrobots ofrobots ofrobots approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trace Events JS API doesn't work w/o CLI args
10 participants
@kjin @addaleax @joyeecheung @richardlau @Trott @vmarchaud @ofrobots @jasnell @cjihrig @nodejs-github-bot