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

trace_events: add more process metadata #21785

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 12, 2018

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

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 nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 12, 2018
@jasnell
Copy link
Member Author

jasnell commented Jul 12, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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?

@@ -32,6 +32,7 @@
#include "uv.h"
#include "v8.h"
#include "tracing/trace_event.h"
#include "tracing/traced_value.h"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jul 12, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

jasnell commented Jul 12, 2018

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
Copy link
Member Author

jasnell commented Jul 12, 2018

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
Copy link
Member Author

jasnell commented Jul 12, 2018

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
Copy link
Member Author

jasnell commented Jul 12, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks!

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 13, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jul 18, 2018
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
Copy link
Member

Landed in ededb4b 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

4 participants