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: add version metadata #20852

Closed
wants to merge 1 commit into from
Closed

trace_events: add version metadata #20852

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 20, 2018

Use TRACE_EVENT_METADATA1 to include Node.js, v8, and libuv version
details in the trace log (helpful for log analysis when trace detail
may vary from one Node.js version to another).

/cc @ofrobots

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

jasnell commented May 20, 2018

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

@jasnell
Copy link
Member Author

jasnell commented May 20, 2018

Failures in CI are known flakies

cjihrig
cjihrig reviewed May 21, 2018
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Is there a reason not to more closely mimic process.versions?

@jasnell
Copy link
Member Author

jasnell commented May 21, 2018

Mimic in which way?

ofrobots
ofrobots approved these changes May 21, 2018
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM w/ nit.

src/node.cc Outdated
@@ -4234,6 +4234,11 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
Environment env(isolate_data, context, v8_platform.GetTracingAgent());
env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);

TRACE_EVENT_METADATA1("__metadata", "version", "node", NODE_VERSION + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm trying to understand the + 1. If you're skipping the v in the version string: 1) why? 2) it would be good to add a comment explaining what the + 1 does as it is not immediately obvious. I would prefer if the v was included too.

Copy link
Member

Choose a reason for hiding this comment

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

i thought we had a lint rule against messing with pointers like that

Copy link
Member Author

Choose a reason for hiding this comment

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

It is for consistency with the value of process.versions.node (which does not include the v)

Copy link
Member

Choose a reason for hiding this comment

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

If we don't want the v why not use NODE_VERSION_STRING instead?

node/src/node_version.h

Line 65 in 32873c5

#define NODE_VERSION "v" NODE_VERSION_STRING

@jasnell
Copy link
Member Author

jasnell commented May 21, 2018

@ofrobots ... btw, I was going to output all of the versions with a single TRACE_EVENT_METADATA call but the underlying implementation only supports a single argument, and I did not want to have to create and allocate a serializable structure for the version information.

I don't want this to get too out of sync with the V8 version, but it would be helpful to have a TRACE_EVENT_METADATA macro that (a) was not one of the INTERNAL_ macros and (b) supported an arbitrary number of arguments.

@cjihrig
Copy link
Contributor

cjihrig commented May 21, 2018

Mimic in which way?

I meant using the same names (for example n-api vs napi), and including the other missing dependencies.

@jasnell
Copy link
Member Author

jasnell commented May 25, 2018

I meant using the same names (for example n-api vs napi), and including the other missing dependencies.

Certainly can do that, yes.

@jasnell
trace_events: add version metadata
c2e2759 
Use `TRACE_EVENT_METADATA1` to include just the node.js version for
now. Later this can be expanded to include more version and platform
details.
@jasnell jasnell force-pushed the trace-event-metadata-versions branch from 566c386 to c2e2759 Compare May 25, 2018 21:58
@jasnell
Copy link
Member Author

jasnell commented May 25, 2018

Updated .... After talking it over more with @ofrobots ... I removed everything but the node version in this to give us more flexibility later. We need to think about the best approach to handling complex data as opposed to emitting multiple trace events. Emitting the node version covers the most basic use case for this -- that is, knowing what Node.js version generated the trace file -- so it covers the immediate need.

@jasnell
Copy link
Member Author

jasnell commented May 30, 2018

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

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2018
jasnell added a commit that referenced this pull request May 31, 2018
@jasnell
trace_events: add version metadata
e60eed1 
Use `TRACE_EVENT_METADATA1` to include just the node.js version for
now. Later this can be expanded to include more version and platform
details.

PR-URL: #20852
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@jasnell
Copy link
Member Author

jasnell commented May 31, 2018

Landed in e60eed1

@jasnell jasnell closed this May 31, 2018
addaleax pushed a commit that referenced this pull request Jun 1, 2018
@jasnell @addaleax
trace_events: add version metadata
b5b7459 
Use `TRACE_EVENT_METADATA1` to include just the node.js version for
now. Later this can be expanded to include more version and platform
details.

PR-URL: #20852
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

@cjihrig cjihrig cjihrig left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@ofrobots ofrobots ofrobots approved these changes

Assignees
No one assigned
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++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jasnell @cjihrig @ofrobots @richardlau @devsnek @BridgeAR @nodejs-github-bot