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

src: add trace events for env.cc #23674

Closed
wants to merge 5 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 15, 2018

Adds a new node.environment trace event category that captures simple lifecycle events for an Environment:

This will be most useful for our own diagnostics around timers and such but it captures useful timing information for end users also.

image

image

image

Note: Will add test and docs in a bit

/cc @nodejs/diagnostics

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@jasnell jasnell added lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Oct 15, 2018
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 15, 2018
src/env.cc Show resolved Hide resolved
src/env.cc Outdated Show resolved Hide resolved
TRACING_CATEGORY_NODE1(environment),
"Environment", this,
"args", std::move(traced_value));
}
Copy link
Member

Choose a reason for hiding this comment

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

Heads up … there’s a very good chance of the args/exec_args handling being moved to another method as part of better embedding support

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was wondering about that. Hmm... perhaps we should make args have their own dedicated trace event category... or even make it part of the process __metadata so that it's always there if tracing is enabled. It's useful context either way.

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell I think that depends… We’ve had a similar conversation in the node-report PR at #22712 – what arguments do we actually care about here? The ones passed on the original command line, to the Node.js CLI, or the one that this Node.js instance explicitly uses?

The distinction matters for embedder use cases; But maybe, as a more practical example: I’d like to add support for something similar to execArgv to Workers. I guess it makes sense to emit these per-Environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking that the one that this Node.js instance explicitly uses... whichever are relevant to each specific Environment instance.

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.

Test also LGTM :)

src/env.cc Outdated Show resolved Hide resolved
src/env.cc Outdated
TraceEventScope(const char* category,
const char* name,
void* id) : category_(category), name_(name), id_(id) {
TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(category_, name_, id);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps also use member field id_?

fs.readFile(file, common.mustCall((err, data) => {
const traces = JSON.parse(data.toString()).traceEvents
.filter((trace) => trace.cat !== '__metadata');
traces.forEach((trace) => {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to chain this and remove intermediate traces variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

just consistency with the other trace event tests. We can simplify all of them in a single batch with a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Since there are some other changes to make in the test anyway, I'll just go ahead and simplify :-)

test/parallel/test-trace-events-environment.js Outdated Show resolved Hide resolved
src/env.cc Outdated
@@ -231,6 +254,23 @@ void Environment::Start(const std::vector<std::string>& args,
HandleScope handle_scope(isolate());
Context::Scope context_scope(context());

if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
TRACING_CATEGORY_NODE1(environment))) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: != 0 for clarity? IIUC

Copy link
Member

Choose a reason for hiding this comment

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

@lundibundi I’m not sure, but quoting the V8 tracing header:

// The TRACE_EVENT macros should only use the value as a bool.

(I know this isn’t technically in a macro, but I think the recommendation from V8 makes sense in the same way?)

Copy link
Member

@lundibundi lundibundi Oct 15, 2018

Choose a reason for hiding this comment

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

🤔 makes sense but we have

node/src/node_contextify.cc

Lines 661 to 662 in 561e30d

if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
TRACING_CATEGORY_NODE2(vm, script)) != 0) {
and

node/src/node_file.cc

Lines 92 to 93 in 561e30d

(*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED \
(TRACING_CATEGORY_NODE2(fs, sync)) != 0)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't hurt to have the != 0 ... we can refactor them all out later if needed.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Seems like a very useful feature.
Left some nits for the test

const file = path.join(tmpdir.path, 'node_trace.1.log');

assert(fs.existsSync(file));
fs.readFile(file, common.mustCall((err, data) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, if this is not significant to the tested code, could this be fs.readFileSync or await fs.promise.readfile

const tmpdir = require('../common/tmpdir');

if (!common.isMainThread)
common.skip('process.chdir is not available in Workers');
Copy link
Member

@richardlau richardlau Oct 15, 2018

Choose a reason for hiding this comment

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

Does that also apply to cwd for child_process.fork (as an alternative if it does not)?

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 shouldn't, but that's a question for @addaleax and is a separate concern from this PR.

Copy link
Member

Choose a reason for hiding this comment

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

When using cwd, chdir() is executed in the child process, so, yes, I think this test could be written without process.chdir() and the need to skip it in Workers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The chdir is there because of the generated trace event file. I'm inclined to leave the test as it is and refactor all of the trace event tests as a whole if this is the direction we want to go (they all follow this same pattern)

@jasnell
Copy link
Member Author

jasnell commented Oct 15, 2018

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2018
@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2018

});

let name;
while (name = names.shift())
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we have initial names as Set too and then use deepStrictEqual(checkSet, names) here?

@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2018

@jasnell
Copy link
Member Author

jasnell commented Oct 17, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 18, 2018
PR-URL: nodejs#23674
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@Trott
Copy link
Member

Trott commented Oct 18, 2018

Landed in 72a48a2

@Trott Trott closed this Oct 18, 2018
danbev pushed a commit that referenced this pull request Oct 19, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

jasnell added a commit that referenced this pull request Oct 21, 2018
PR-URL: #23674
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
richardlau added a commit to richardlau/node-1 that referenced this pull request Nov 2, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Backport-PR-URL: nodejs#23882
PR-URL: nodejs#23698
Refs: nodejs#23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js
MylesBorins pushed a commit that referenced this pull request Nov 4, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. 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