Skip to content

Commit

Permalink
trace_events: fix trace events JS API not writing traces
Browse files Browse the repository at this point in the history
The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the case where no CLI
args are provided. This commit adds that test.

Fixes nodejs#24944
  • Loading branch information
kjin committed Feb 14, 2019
1 parent b1cc1af commit 6d6d36f
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 9 deletions.
3 changes: 3 additions & 0 deletions src/node_trace_events.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo<Value>& args) {
CHECK_NOT_NULL(category_set);
const auto& categories = category_set->GetCategories();
if (!category_set->enabled_ && !categories.empty()) {
// Starts the Tracing Agent if it wasn't started already (e.g. through
// a command line flag.)
StartTracingAgent();
GetTracingAgentWriter()->Enable(categories);
category_set->enabled_ = true;
}
Expand Down
16 changes: 12 additions & 4 deletions src/node_v8_platform-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ struct V8Platform {
tracing_agent_->GetTracingController();
trace_state_observer_.reset(new NodeTraceStateObserver(controller));
controller->AddTraceStateObserver(trace_state_observer_.get());
StartTracingAgent();
tracing_file_writer_ = tracing_agent_->DefaultHandle();
// Only start the tracing agent if we enabled any tracing categories.
if (!per_process::cli_options->trace_event_categories.empty()) {
StartTracingAgent();
}
// Tracing must be initialized before platform threads are created.
platform_ = new NodePlatform(thread_pool_size, controller);
v8::V8::InitializePlatform(platform_);
Expand All @@ -111,9 +115,9 @@ struct V8Platform {
}

inline void StartTracingAgent() {
if (per_process::cli_options->trace_event_categories.empty()) {
tracing_file_writer_ = tracing_agent_->DefaultHandle();
} else {
// Attach a new NodeTraceWriter only if this function hasn't been called
// before.
if (tracing_file_writer_.IsDefaultHandle()) {
std::vector<std::string> categories =
SplitString(per_process::cli_options->trace_event_categories, ',');

Expand Down Expand Up @@ -163,6 +167,10 @@ namespace per_process {
extern struct V8Platform v8_platform;
}

inline void StartTracingAgent() {
return per_process::v8_platform.StartTracingAgent();
}

inline tracing::AgentWriterHandle* GetTracingAgentWriter() {
return per_process::v8_platform.GetTracingAgentWriter();
}
Expand Down
6 changes: 6 additions & 0 deletions src/tracing/agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class AgentWriterHandle {
inline void Enable(const std::set<std::string>& categories);
inline void Disable(const std::set<std::string>& categories);

inline bool IsDefaultHandle();

inline Agent* agent() { return agent_; }

inline v8::TracingController* GetTracingController();
Expand Down Expand Up @@ -174,6 +176,10 @@ void AgentWriterHandle::Disable(const std::set<std::string>& categories) {
if (agent_ != nullptr) agent_->Disable(id_, categories);
}

bool AgentWriterHandle::IsDefaultHandle() {
return agent_ != nullptr && id_ == Agent::kDefaultHandleId;
}

inline v8::TracingController* AgentWriterHandle::GetTracingController() {
return agent_ != nullptr ? agent_->GetTracingController() : nullptr;
}
Expand Down
30 changes: 25 additions & 5 deletions test/parallel/test-trace-events-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,17 @@ const {
getEnabledCategories
} = require('trace_events');

function getEnabledCategoriesFromCommandLine() {
const indexOfCatFlag = process.execArgv.indexOf('--trace-event-categories');
if (indexOfCatFlag === -1) {
return undefined;
} else {
return process.execArgv[indexOfCatFlag + 1];
}
}

const isChild = process.argv[2] === 'child';
const enabledCategories = isChild ? 'foo' : undefined;
const enabledCategories = getEnabledCategoriesFromCommandLine();

assert.strictEqual(getEnabledCategories(), enabledCategories);
[1, 'foo', true, false, null, undefined].forEach((i) => {
Expand Down Expand Up @@ -51,7 +60,9 @@ tracing.enable(); // purposefully enable twice to test calling twice
assert.strictEqual(tracing.enabled, true);

assert.strictEqual(getEnabledCategories(),
isChild ? 'foo,node.perf' : 'node.perf');
[
...[enabledCategories].filter((_) => !!_), 'node.perf'
].join(','));

tracing.disable();
assert.strictEqual(tracing.enabled, false);
Expand Down Expand Up @@ -106,7 +117,15 @@ if (isChild) {
}
}

testApiInChildProcess([], () => {
testApiInChildProcess(['--trace-event-categories', 'foo']);
});
}

function testApiInChildProcess(execArgs, cb) {
tmpdir.refresh();
// Save the current directory so we can chdir back to it later
const parentDir = process.cwd();
process.chdir(tmpdir.path);

const expectedMarks = ['A', 'B'];
Expand All @@ -121,15 +140,14 @@ if (isChild) {

const proc = cp.fork(__filename,
['child'],
{ execArgv: [ '--expose-gc',
'--trace-event-categories',
'foo' ] });
{ execArgv: [ '--expose-gc', ...execArgs ] });

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) => {
assert.ifError(err);
const traces = JSON.parse(data.toString()).traceEvents
.filter((trace) => trace.cat !== '__metadata');
assert.strictEqual(traces.length,
Expand Down Expand Up @@ -160,6 +178,8 @@ if (isChild) {
assert.fail('Unexpected trace event phase');
}
});
process.chdir(parentDir);
cb && process.nextTick(cb);
}));
}));
}

0 comments on commit 6d6d36f

Please sign in to comment.