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

trace_events: add more process metadata by jasnell · Pull Request #21785 · nodejs/node · GitHub

/ node Public

trace_events: add more process metadata - #21785

Closed
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:trace-process-metadata
ConversationCommits (3)ChecksFiles changed
Closed

trace_events: add more process metadata#21785
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:trace-process-metadata

Conversation

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member

Now that TracedValue has landed, add more detailed
process __metadata including versions, arch, platform,
release detail, and argv/execArgv to the trace event
log.

This adds an entry like:

{
  "pid":46021,
  "tid":46021,
  "ts":4996201438,
  "tts":31331,
  "ph":"M",
  "cat":"__metadata",
  "name":"node",
  "dur":0,
  "tdur":0,
  "args":{
    "process":{
      "versions":{
        "http_parser":"2.8.0",
        "node":"11.0.0-pre",
        "v8":"6.7.288.46-node.14",
        "uv":"1.22.0",
        "zlib":"1.2.11",
        "ares":"1.14.0",
        "modules":"64",
        "nghttp2":"1.32.0",
        "napi":"3",
        "openssl":"1.1.0h"
      },
      "arch":"x64",
      "platform":"linux",
      "release":{
        "name":"node"
      },
      "argv":["./node"],
      "execArgv":["--trace-event-categories","node.perf"]
    }
  }
}
Checklist
  • make -j4 test(UNIX), orvcbuild test` (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

trace_events: add more process metadata
e55bab0 
Now that TracedValue has landed, add more detailed
process `__metadata` including versions, arch, platform,
release detail, and argv/execArgv to the trace event
log.

nodejs-github-bot commented Jul 12, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/260/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 12, 2018

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member Author

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

addaleax reviewed Jul 12, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

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

I would really like it if we could avoid the overhead of these calls when tracing is not enabled, especially if this is going to be a more common pattern?

Comment thread src/node_internals.h Outdated
#include "uv.h"
#include "v8.h"
#include "tracing/trace_event.h"
#include "tracing/traced_value.h"

addaleax Jul 12, 2018

Copy link
Copy Markdown
Member

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

Can we include these headers where they are used, rather than (almost) globally?

jasnell Jul 12, 2018

Copy link
Copy Markdown
Member Author

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

Sure

addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jul 12, 2018
addaleax reviewed Jul 12, 2018
View reviewed changes

addaleax left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

I would really like it if we could avoid the overhead of these calls when tracing is not enabled, especially if this is going to be a more common pattern?

Retracting my own review comment: I think I have a good idea for how to do that, let me code that out real quick :)

Edit: No, sorry. My idea was to create a custom kind of smart pointer to use instead of std::unique_ptr<TracedValue>, which would mirror these methods and turn them into no-ops if the handle contained nullptr.

But that requires knowing in advance whether a tracing category is enabled, and I don’t really know how to do that :/

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member Author

There's a way. The difficulty with metadata events is that they are always implicitly enabled if any tracing at all is enabled. So we would need to separate them out to allow on demand creation. I can work on that.

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member Author

But that requires knowing in advance whether a tracing category is enabled, and I don’t really know how to do that :/

I'll be pushing a commit in a few minutes... but the way to do this is to attach a v8::TracingController::TraceStateObserver to the TracingController. It will be notified when tracing is enabled/disabled. We can use that as a signal to identify when various categories are dynamically enabled/disabled or when tracing as a whole is enabled. That will allow us to set any state variables we need to determine whether to perform any complex computations or TracedValue building.

jasnell commented Jul 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Ok, @addaleax ... take a look now. I'm using a TraceStateObserver to only output the __metadata events the first time the tracing is enabled. This will mean that the TracedValue for the process metadata is only created the one time when tracing is first enabled. We can use a similar mechanism to selectively enable the building of complex TracedValue instances throughout by implementing a more persistent TraceStateObserver that actively tracks when certain categories are enabled/disabled. This is the same technique that V8 uses internally for exactly this same kind of thing.

Note: by switching to this, just to keep things simple, the process metadata no longer includes the argv and execArgv details since those are not around when the TraceStateObserver is invoked.

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member Author

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

addaleax approved these changes Jul 13, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

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

Thanks!

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 13, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jul 18, 2018
trace_events: add more process metadata
ededb4b 
Now that TracedValue has landed, add more detailed
process `__metadata` including versions, arch, platform,
release detail, and argv/execArgv to the trace event
log.

PR-URL: nodejs#21785
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR commented Jul 18, 2018

Copy link
Copy Markdown
Member

Landed in ededb4b 🎉

BridgeAR closed this Jul 18, 2018
targos pushed a commit that referenced this pull request Jul 19, 2018
trace_events: add more process metadata
d7edee4 
Now that TracedValue has landed, add more detailed
process `__metadata` including versions, arch, platform,
release detail, and argv/execArgv to the trace event
log.

PR-URL: #21785
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
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

addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

4 participants

Back | FazBrowse Home | New Git URL