FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

v7.x backport: src: add tracing controller by matthewloring · Pull Request #11106 · nodejs/node · GitHub

/ node Public

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

Closed
matthewloring wants to merge 1 commit into
nodejs:v7.x-stagingfrom
matthewloring:v7.x-trace-event
ConversationCommits (1)ChecksFiles changed
Closed

v7.x backport: src: add tracing controller#11106
matthewloring wants to merge 1 commit into
nodejs:v7.x-stagingfrom
matthewloring:v7.x-trace-event

Conversation

matthewloring commented Feb 1, 2017

Copy link
Copy Markdown

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 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
matthewloring mentioned this pull request Feb 1, 2017
Closed
3 tasks
mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Feb 1, 2017

richardlau commented Feb 1, 2017

Copy link
Copy Markdown
Member

Should #10959 be included in this or backported separately?

joshgav commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

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 commented Feb 3, 2017

Copy link
Copy Markdown
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 mentioned this pull request Feb 3, 2017
Closed
2 tasks
italoacasas changed the title src: Node Tracing Controller Backport - src: Node Tracing Controller Feb 4, 2017
italoacasas changed the title Backport - src: Node Tracing Controller v7.x bacport - src: Node Tracing Controller Feb 4, 2017
italoacasas changed the title v7.x bacport - src: Node Tracing Controller v7.x backport - src: Node Tracing Controller Feb 4, 2017

italoacasas commented Feb 16, 2017

Copy link
Copy Markdown

@matthewloring can you rebase please

matthewloring commented Feb 16, 2017

Copy link
Copy Markdown
Author

@italoacasas All rebased.

matthewloring commented Feb 27, 2017

Copy link
Copy Markdown
Author

@joshgav Can this land?

matthewloring commented Feb 27, 2017

Copy link
Copy Markdown
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 commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

@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?

joshgav approved these changes Feb 28, 2017
View reviewed changes

joshgav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rubber stamp LGTM from reviewing original PR.

italoacasas commented Feb 28, 2017 •
edited
Loading

Copy link
Copy Markdown

@joshgav same process. we need LGTM here.

italoacasas commented Feb 28, 2017 •
edited
Loading

Copy link
Copy Markdown

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

src: add tracing controller
0461c7b 
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 changed the title v7.x backport - src: Node Tracing Controller v7.x backport: src: add tracing controller Feb 28, 2017

joshgav commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

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

matthewloring commented Feb 28, 2017

Copy link
Copy Markdown
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
src: add tracing controller
bd4ccc8 
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 commented Mar 1, 2017

Copy link
Copy Markdown

Landed

italoacasas closed this Mar 1, 2017

Trott commented Mar 3, 2017

Copy link
Copy Markdown
Member

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 commented Mar 3, 2017

Copy link
Copy Markdown
Member

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 commented Mar 3, 2017

Copy link
Copy Markdown
Author

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

This was referenced Jul 6, 2023
Open
Open
Open
Open
aliscco mentioned this pull request Jul 10, 2023
Open
leonardoadame mentioned this pull request Nov 28, 2023
Open
This was referenced Nov 29, 2023
Open
Open
leonardoadame mentioned this pull request Nov 30, 2023
Open
aliscco mentioned this pull request Dec 2, 2023
Open
leonardoadame mentioned this pull request Dec 5, 2023
Open
aliscco mentioned this pull request Dec 8, 2023
Open
This was referenced Dec 10, 2023
Open
Open
This was referenced Dec 10, 2023
Open
Open
leonardoadame mentioned this pull request Dec 19, 2023
Open
aliscco mentioned this pull request Jan 1, 2024
Open
aliscco mentioned this pull request Jan 31, 2024
Open
leonardoadame mentioned this pull request Feb 12, 2024
Open
leonardoadame mentioned this pull request May 13, 2024
Open
aliscco mentioned this pull request May 15, 2024
Open
leonardoadame mentioned this pull request May 16, 2024
Open
This was referenced May 16, 2024
Open
Open
Open
leonardoadame mentioned this pull request May 19, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
joshgav joshgav approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL