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

v7.x backport: src: add tracing controller #11106

Closed
wants to merge 1 commit into from
Closed

v7.x backport: src: add tracing controller #11106

wants to merge 1 commit into from

Conversation

matthewloring
Copy link

This commit adds support for trace-event tracing to Node.js. It provides
a mechanism to centralize tracing information generated by V8, Node
core, and userspace code. It includes:

  • A trace writer responsible for serializing traces and cycling the
    output files so that no individual file becomes to large.
  • A buffer for aggregating traces to allow for batched flushes.
  • An agent which initializes the tracing controller and ensures that
    trace serialization is done on a separate thread.
  • A set of macros for generating trace events.
  • Tests and documentation.

Author: Raymond Kang raymondksi@gmail.com
Author: Kelvin Jin kelvinjin@google.com
Author: Matthew Loring mattloring@google.com
Author: Jason Ginchereau jasongin@microsoft.com

PR-URL: #9304

Reviewed-By: Trevor Norris trev.norris@gmail.com
Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com
Reviewed-By: Josh Gavant josh.gavant@outlook.com

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. v7.x labels Feb 1, 2017
@mscdex mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Feb 1, 2017
@richardlau
Copy link
Member

Should #10959 be included in this or backported separately?

@joshgav
Copy link
Contributor

joshgav commented Feb 2, 2017

There were some concerns in #9304 we wanted to address before including this in a release, in particular making the log output location configurable (#9304 (comment)).

/cc @jasongin @nodejs/diagnostics

@matthewloring
Copy link
Author

@joshgav The use of the --trace-events-enabled flag prints Warning: Trace event is an experimental feature and could change at any time.. There are many enhancements that would improve trace event reporting/usage but I'm not sure which should hold it back from a release. I think it provides enough value in its current form to release but I don't mind holding off if others have strong opinions.

@richardlau I think there are a few dependent PRs that can land if this one does. They can probably be opened separately if/once this has gone in.

@italoacasas italoacasas changed the title src: Node Tracing Controller Backport - src: Node Tracing Controller Feb 4, 2017
@italoacasas italoacasas changed the title Backport - src: Node Tracing Controller v7.x bacport - src: Node Tracing Controller Feb 4, 2017
@italoacasas italoacasas changed the title v7.x bacport - src: Node Tracing Controller v7.x backport - src: Node Tracing Controller Feb 4, 2017
@italoacasas
Copy link
Contributor

@matthewloring can you rebase please

@matthewloring
Copy link
Author

@italoacasas All rebased.

@matthewloring
Copy link
Author

@joshgav Can this land?

@matthewloring
Copy link
Author

It also looks like the trace event configuration options landed in the node --help output. Not sure how they got into the release without this PR.

@joshgav
Copy link
Contributor

joshgav commented Feb 28, 2017

@matthewloring

@joshgav Can this land?

LGTM

@italoacasas do the approvals/LGTMs from the original PR count for backports too? Or do backports need another approval?

Copy link
Contributor

@joshgav joshgav left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM from reviewing original PR.

@italoacasas
Copy link
Contributor

italoacasas commented Feb 28, 2017

@joshgav same process. we need LGTM here.

@italoacasas
Copy link
Contributor

italoacasas commented Feb 28, 2017

This is not landing in version 7 staging, because of 4b8b7e9. @matthewloring can you update the backport again please, and sorry for the inconvenience.

This commit adds support for trace-event tracing to Node.js. It provides
a mechanism to centralize tracing information generated by V8, Node
core, and userspace code. It includes:

 - A trace writer responsible for serializing traces and cycling the
   output files so that no individual file becomes to large.
 - A buffer for aggregating traces to allow for batched flushes.
 - An agent which initializes the tracing controller and ensures that
   trace serialization is done on a separate thread.
 - A set of macros for generating trace events.
 - Tests and documentation.

Author: Raymond Kang <raymondksi@gmail.com>
Author: Kelvin Jin <kelvinjin@google.com>
Author: Matthew Loring <mattloring@google.com>
Author: Jason Ginchereau <jasongin@microsoft.com>

PR-URL: #9304
Backport PR-URL: #11106
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
@joshgav joshgav changed the title v7.x backport - src: Node Tracing Controller v7.x backport: src: add tracing controller Feb 28, 2017
@joshgav
Copy link
Contributor

joshgav commented Feb 28, 2017

Rebased cleanly (and force-pushed to @matthewloring's branch). Still need another LGTM I think? @nodejs/diagnostics PTAL.

@matthewloring
Copy link
Author

LGTM but I'm not sure I count since I opened the PR.

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 1, 2017
This commit adds support for trace-event tracing to Node.js. It provides
a mechanism to centralize tracing information generated by V8, Node
core, and userspace code. It includes:

 - A trace writer responsible for serializing traces and cycling the
   output files so that no individual file becomes to large.
 - A buffer for aggregating traces to allow for batched flushes.
 - An agent which initializes the tracing controller and ensures that
   trace serialization is done on a separate thread.
 - A set of macros for generating trace events.
 - Tests and documentation.

Author: Raymond Kang <raymondksi@gmail.com>
Author: Kelvin Jin <kelvinjin@google.com>
Author: Matthew Loring <mattloring@google.com>
Author: Jason Ginchereau <jasongin@microsoft.com>

PR-URL: nodejs#11106
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
@italoacasas
Copy link
Contributor

Landed

@italoacasas italoacasas closed this Mar 1, 2017
@Trott
Copy link
Member

Trott commented Mar 3, 2017

Adding dont-land-on for v4.x and v6.x as this is a backport of a semver-minor. /cc @MylesBorins (2 of 3)

@jasnell
Copy link
Member

jasnell commented Mar 3, 2017

selected semver-minor's can land on LTS branches. It people feel that it is worthwhile, then mention @nodejs/lts in the comments and a decision can be made.

@matthewloring
Copy link
Author

This change is not compatible with the versions of V8 in 4.x and 6.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants