Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/api/tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ The available categories are:
The [`async_hooks`] events have a unique `asyncId` and a special `triggerId`
`triggerAsyncId` property.
* `node.bootstrap` - Enables capture of Node.js bootstrap milestones.
* `node.environment` - Enables capture of Node.js Environment milestones.
* `node.fs.sync` - Enables capture of trace data for file system sync methods.
* `node.perf` - Enables capture of [Performance API] measurements.
* `node.perf.usertiming` - Enables capture of only Performance API User Timing
Expand Down
55 changes: 54 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "node_context_data.h"
#include "node_worker.h"
#include "tracing/agent.h"
#include "tracing/traced_value.h"

#include <stdio.h>
#include <algorithm>
Expand All @@ -33,6 +34,25 @@ using worker::Worker;

#define kTraceCategoryCount 1

// TODO(@jasnell): Likely useful to move this to util or node_internal to
// allow reuse. But since we're not reusing it yet...
class TraceEventScope {
public:
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
Copy Markdown
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_?

}
~TraceEventScope() {
TRACE_EVENT_NESTABLE_ASYNC_END0(category_, name_, id_);
}

private:
Comment thread
jasnell marked this conversation as resolved.
const char* category_;
const char* name_;
void* id_;
};

int const Environment::kNodeContextTag = 0x6e6f64;
void* Environment::kNodeContextTagPtr = const_cast<void*>(
static_cast<const void*>(&Environment::kNodeContextTag));
Expand Down Expand Up @@ -223,6 +243,9 @@ Environment::~Environment() {
delete[] heap_statistics_buffer_;
delete[] heap_space_statistics_buffer_;
delete[] http_parser_buffer_;

TRACE_EVENT_NESTABLE_ASYNC_END0(
TRACING_CATEGORY_NODE1(environment), "Environment", this);
}

void Environment::Start(const std::vector<std::string>& args,
Expand All @@ -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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

auto traced_value = tracing::TracedValue::Create();
traced_value->BeginArray("args");
for (const std::string& arg : args)
traced_value->AppendString(arg);
traced_value->EndArray();
traced_value->BeginArray("exec_args");
for (const std::string& arg : exec_args)
traced_value->AppendString(arg);
traced_value->EndArray();
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
TRACING_CATEGORY_NODE1(environment),
"Environment", this,
"args", std::move(traced_value));
}
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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.


CHECK_EQ(0, uv_timer_init(event_loop(), timer_handle()));
uv_unref(reinterpret_cast<uv_handle_t*>(timer_handle()));

Expand Down Expand Up @@ -400,6 +440,8 @@ void Environment::PrintSyncTrace() const {
}

void Environment::RunCleanup() {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"RunCleanup", this);
CleanupHandles();

while (!cleanup_hooks_.empty()) {
Expand Down Expand Up @@ -431,6 +473,8 @@ void Environment::RunCleanup() {
}

void Environment::RunBeforeExitCallbacks() {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"BeforeExit", this);
for (ExitCallback before_exit : before_exit_functions_) {
before_exit.cb_(before_exit.arg_);
}
Expand All @@ -442,6 +486,8 @@ void Environment::BeforeExit(void (*cb)(void* arg), void* arg) {
}

void Environment::RunAtExitCallbacks() {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"AtExit", this);
for (ExitCallback at_exit : at_exit_functions_) {
at_exit.cb_(at_exit.arg_);
}
Expand Down Expand Up @@ -495,13 +541,16 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type,

Environment* env = Environment::GetCurrent(context);
if (env == nullptr) return;

TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"EnvPromiseHook", env);
for (const PromiseHookCallback& hook : env->promise_hooks_) {
hook.cb_(type, promise, parent, hook.arg_);
}
}

void Environment::RunAndClearNativeImmediates() {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"RunAndClearNativeImmediates", this);
size_t count = native_immediate_callbacks_.size();
if (count > 0) {
size_t ref_count = 0;
Expand Down Expand Up @@ -554,6 +603,8 @@ void Environment::ToggleTimerRef(bool ref) {

void Environment::RunTimers(uv_timer_t* handle) {
Environment* env = Environment::from_timer_handle(handle);
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"RunTimers", env);

if (!env->can_call_into_js())
return;
Expand Down Expand Up @@ -614,6 +665,8 @@ void Environment::RunTimers(uv_timer_t* handle) {

void Environment::CheckImmediate(uv_check_t* handle) {
Environment* env = Environment::from_immediate_check_handle(handle);
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"CheckImmediate", env);

if (env->immediate_info()->count() == 0)
return;
Expand Down
49 changes: 49 additions & 0 deletions test/parallel/test-trace-events-environment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';
const common = require('../common');
const assert = require('assert');
Comment thread
jasnell marked this conversation as resolved.
const cp = require('child_process');
const path = require('path');
const fs = require('fs');
const tmpdir = require('../common/tmpdir');

if (!common.isMainThread)
common.skip('process.chdir is not available in Workers');
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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)


const names = [
'Environment',
'RunAndClearNativeImmediates',
'CheckImmediate',
'RunTimers',
'BeforeExit',
'RunCleanup',
'AtExit'
];

if (process.argv[2] === 'child') {
1 + 1;
Comment thread
jasnell marked this conversation as resolved.
} else {
tmpdir.refresh();
process.chdir(tmpdir.path);

const proc = cp.fork(__filename,
[ 'child' ], {
execArgv: [
'--trace-event-categories',
'node.environment'
]
});

proc.once('exit', common.mustCall(() => {
const file = path.join(tmpdir.path, 'node_trace.1.log');

assert(fs.existsSync(file));
fs.readFile(file, common.mustCall((err, data) => {
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.

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

const traces = JSON.parse(data.toString()).traceEvents
.filter((trace) => trace.cat !== '__metadata');
traces.forEach((trace) => {
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.

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

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.

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

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.

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

assert.strictEqual(trace.pid, proc.pid);
assert(names.includes(trace.name));
Comment thread
jasnell marked this conversation as resolved.
Outdated
});
}));
}));
}