From 6d6d36f8f543d8fa342bbf37b653c802065589d0 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 10 Dec 2018 20:44:18 +0000 Subject: [PATCH] trace_events: fix trace events JS API not writing traces 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 https://github.com/nodejs/node/issues/24944 --- src/node_trace_events.cc | 3 +++ src/node_v8_platform-inl.h | 16 ++++++++++---- src/tracing/agent.h | 6 ++++++ test/parallel/test-trace-events-api.js | 30 +++++++++++++++++++++----- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index 9538e75d2c54ed..6a0d4f037a76c4 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -74,6 +74,9 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo& 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; } diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index ed91ee3a022551..2c89ee9e5244b5 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -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_); @@ -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 categories = SplitString(per_process::cli_options->trace_event_categories, ','); @@ -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(); } diff --git a/src/tracing/agent.h b/src/tracing/agent.h index 62073ba6b214ef..840b78868a3064 100644 --- a/src/tracing/agent.h +++ b/src/tracing/agent.h @@ -59,6 +59,8 @@ class AgentWriterHandle { inline void Enable(const std::set& categories); inline void Disable(const std::set& categories); + inline bool IsDefaultHandle(); + inline Agent* agent() { return agent_; } inline v8::TracingController* GetTracingController(); @@ -174,6 +176,10 @@ void AgentWriterHandle::Disable(const std::set& 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; } diff --git a/test/parallel/test-trace-events-api.js b/test/parallel/test-trace-events-api.js index 346f943b9c36c5..cf11daa6edf869 100644 --- a/test/parallel/test-trace-events-api.js +++ b/test/parallel/test-trace-events-api.js @@ -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) => { @@ -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); @@ -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']; @@ -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, @@ -160,6 +178,8 @@ if (isChild) { assert.fail('Unexpected trace event phase'); } }); + process.chdir(parentDir); + cb && process.nextTick(cb); })); })); }