src: refactor default trace writer out of agent

The agent code is supposed to manage multiple writers/clients.
Adding the default writer into the mix breaks that encapsulation.
Instead, manage default options through a separate "virtual"
default client handle, and keep the file writer management
all to the actual users.

PR-URL: nodejs#21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Author: addaleax

src/env-inl.h

@@ -325,8 +325,8 @@ inline v8::Isolate* Environment::isolate() const {
   return isolate_;
 }
 
-inline tracing::Agent* Environment::tracing_agent() const {
-  return tracing_agent_;
+inline tracing::AgentWriterHandle* Environment::tracing_agent_writer() const {
+  return tracing_agent_writer_;
 }
 
 inline Environment* Environment::from_timer_handle(uv_timer_t* handle) {

src/env.cc

@@ -105,10 +105,10 @@ void InitThreadLocalOnce() {
 
 Environment::Environment(IsolateData* isolate_data,
                          Local<Context> context,
-                         tracing::Agent* tracing_agent)
+                         tracing::AgentWriterHandle* tracing_agent_writer)
     : isolate_(context->GetIsolate()),
       isolate_data_(isolate_data),
-      tracing_agent_(tracing_agent),
+      tracing_agent_writer_(tracing_agent_writer),
       immediate_info_(context->GetIsolate()),
       tick_info_(context->GetIsolate()),
       timer_base_(uv_now(isolate_data->event_loop())),

src/env.h

@@ -55,7 +55,7 @@ class performance_state;
 }
 
 namespace tracing {
-class Agent;
+class AgentWriterHandle;
 }
 
 namespace worker {
@@ -587,7 +587,7 @@ class Environment {
 
   Environment(IsolateData* isolate_data,
               v8::Local<v8::Context> context,
-              tracing::Agent* tracing_agent);
+              tracing::AgentWriterHandle* tracing_agent_writer);
   ~Environment();
 
   void Start(int argc,
@@ -625,7 +625,7 @@ class Environment {
   inline bool profiler_idle_notifier_started() const;
 
   inline v8::Isolate* isolate() const;
-  inline tracing::Agent* tracing_agent() const;
+  inline tracing::AgentWriterHandle* tracing_agent_writer() const;
   inline uv_loop_t* event_loop() const;
   inline uint32_t watched_providers() const;
 
@@ -879,7 +879,7 @@ class Environment {
 
   v8::Isolate* const isolate_;
   IsolateData* const isolate_data_;
-  tracing::Agent* const tracing_agent_;
+  tracing::AgentWriterHandle* const tracing_agent_writer_;
   uv_timer_t timer_handle_;
   uv_check_t immediate_check_handle_;
   uv_idle_t immediate_idle_handle_;

src/inspector/tracing_agent.cc

@@ -74,10 +74,11 @@ DispatchResponse TracingAgent::start(
   if (categories_set.empty())
     return DispatchResponse::Error("At least one category should be enabled");
 
-  trace_writer_ = env_->tracing_agent()->AddClient(
+  trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient(
       categories_set,
       std::unique_ptr<InspectorTraceWriter>(
-          new InspectorTraceWriter(frontend_.get())));
+          new InspectorTraceWriter(frontend_.get())),
+      tracing::Agent::kIgnoreDefaultCategories);
   return DispatchResponse::OK();
 }
 

src/node.cc

@@ -61,6 +61,7 @@
 #include "req_wrap-inl.h"
 #include "string_bytes.h"
 #include "tracing/agent.h"
+#include "tracing/node_trace_writer.h"
 #include "util.h"
 #include "uv.h"
 #if NODE_USE_V8_PLATFORM
@@ -427,17 +428,24 @@ static struct {
 #endif  // HAVE_INSPECTOR
 
   void StartTracingAgent() {
-    tracing_file_writer_ = tracing_agent_->AddClient(
-        trace_enabled_categories,
-        new tracing::NodeTraceWriter(trace_file_pattern));
+    if (trace_enabled_categories.empty()) {
+      tracing_file_writer_ = tracing_agent_->DefaultHandle();
+    } else {
+      tracing_file_writer_ = tracing_agent_->AddClient(
+          ParseCommaSeparatedSet(trace_enabled_categories),
+          std::unique_ptr<tracing::AsyncTraceWriter>(
+              new tracing::NodeTraceWriter(trace_file_pattern,
+                                           tracing_agent_->loop())),
+          tracing::Agent::kUseDefaultCategories);
+    }
   }
 
   void StopTracingAgent() {
     tracing_file_writer_.reset();
   }
 
-  tracing::Agent* GetTracingAgent() const {
-    return tracing_agent_.get();
+  tracing::AgentWriterHandle* GetTracingAgentWriter() {
+    return &tracing_file_writer_;
   }
 
   NodePlatform* Platform() {
@@ -466,7 +474,9 @@ static struct {
   }
   void StopTracingAgent() {}
 
-  tracing::Agent* GetTracingAgent() const { return nullptr; }
+  tracing::AgentWriterHandle* GetTracingAgentWriter() {
+    return nullptr;
+  }
 
   NodePlatform* Platform() {
     return nullptr;
@@ -3590,7 +3600,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
   HandleScope handle_scope(isolate);
   Context::Scope context_scope(context);
   auto env = new Environment(isolate_data, context,
-                             v8_platform.GetTracingAgent());
+                             v8_platform.GetTracingAgentWriter());
   env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
   return env;
 }
@@ -3649,7 +3659,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   HandleScope handle_scope(isolate);
   Local<Context> context = NewContext(isolate);
   Context::Scope context_scope(context);
-  Environment env(isolate_data, context, v8_platform.GetTracingAgent());
+  Environment env(isolate_data, context, v8_platform.GetTracingAgentWriter());
   env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
 
   const char* path = argc > 1 ? argv[1] : nullptr;

src/node_trace_events.cc

@@ -58,7 +58,7 @@ void NodeCategorySet::New(const FunctionCallbackInfo<Value>& args) {
     if (!*val) return;
     categories.emplace(*val);
   }
-  CHECK_NOT_NULL(env->tracing_agent());
+  CHECK_NOT_NULL(env->tracing_agent_writer());
   new NodeCategorySet(env, args.This(), std::move(categories));
 }
 
@@ -69,7 +69,7 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (!category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent()->Enable(categories);
+    env->tracing_agent_writer()->Enable(categories);
     category_set->enabled_ = true;
   }
 }
@@ -81,14 +81,15 @@ void NodeCategorySet::Disable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent()->Disable(categories);
+    env->tracing_agent_writer()->Disable(categories);
     category_set->enabled_ = false;
   }
 }
 
 void GetEnabledCategories(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  std::string categories = env->tracing_agent()->GetEnabledCategories();
+  std::string categories =
+      env->tracing_agent_writer()->agent()->GetEnabledCategories();
   if (!categories.empty()) {
     args.GetReturnValue().Set(
       String::NewFromUtf8(env->isolate(),

src/tracing/agent.cc

@@ -1,23 +1,24 @@
 #include "tracing/agent.h"
 
-#include <sstream>
 #include <string>
 #include "tracing/node_trace_buffer.h"
-#include "tracing/node_trace_writer.h"
 
 namespace node {
 namespace tracing {
 
-namespace {
-
-class ScopedSuspendTracing {
+class Agent::ScopedSuspendTracing {
  public:
-  ScopedSuspendTracing(TracingController* controller, Agent* agent)
-                       : controller_(controller), agent_(agent) {
-    controller->StopTracing();
+  ScopedSuspendTracing(TracingController* controller, Agent* agent,
+                       bool do_suspend = true)
+    : controller_(controller), agent_(do_suspend ? agent : nullptr) {
+    if (do_suspend) {
+      CHECK(agent_->started_);
+      controller->StopTracing();
+    }
   }
 
   ~ScopedSuspendTracing() {
+    if (agent_ == nullptr) return;
     TraceConfig* config = agent_->CreateTraceConfig();
     if (config != nullptr) {
       controller_->StartTracing(config);
@@ -29,8 +30,10 @@ class ScopedSuspendTracing {
   Agent* agent_;
 };
 
+namespace {
+
 std::set<std::string> flatten(
-    const std::unordered_map<int, std::set<std::string>>& map) {
+    const std::unordered_map<int, std::multiset<std::string>>& map) {
   std::set<std::string> result;
   for (const auto& id_value : map)
     result.insert(id_value.second.begin(), id_value.second.end());
@@ -43,18 +46,17 @@ using v8::platform::tracing::TraceConfig;
 using v8::platform::tracing::TraceWriter;
 using std::string;
 
-Agent::Agent(const std::string& log_file_pattern)
-    : log_file_pattern_(log_file_pattern) {
+Agent::Agent() {
   tracing_controller_ = new TracingController();
   tracing_controller_->Initialize(nullptr);
+
+  CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
 }
 
 void Agent::Start() {
   if (started_)
     return;
 
-  CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
-
   NodeTraceBuffer* trace_buffer_ = new NodeTraceBuffer(
       NodeTraceBuffer::kBufferChunks, this, &tracing_loop_);
   tracing_controller_->Initialize(trace_buffer_);
@@ -71,18 +73,30 @@ void Agent::Start() {
 
 AgentWriterHandle Agent::AddClient(
     const std::set<std::string>& categories,
-    std::unique_ptr<AsyncTraceWriter> writer) {
+    std::unique_ptr<AsyncTraceWriter> writer,
+    enum UseDefaultCategoryMode mode) {
   Start();
+
+  const std::set<std::string>* use_categories = &categories;
+
+  std::set<std::string> categories_with_default;
+  if (mode == kUseDefaultCategories) {
+    categories_with_default.insert(categories.begin(), categories.end());
+    categories_with_default.insert(categories_[kDefaultHandleId].begin(),
+                                   categories_[kDefaultHandleId].end());
+    use_categories = &categories_with_default;
+  }
+
   ScopedSuspendTracing suspend(tracing_controller_, this);
   int id = next_writer_id_++;
   writers_[id] = std::move(writer);
-  categories_[id] = categories;
+  categories_[id] = { use_categories->begin(), use_categories->end() };
 
   return AgentWriterHandle(this, id);
 }
 
-void Agent::Stop() {
-  file_writer_.reset();
+AgentWriterHandle Agent::DefaultHandle() {
+  return AgentWriterHandle(this, kDefaultHandleId);
 }
 
 void Agent::StopTracing() {
@@ -99,54 +113,30 @@ void Agent::StopTracing() {
 }
 
 void Agent::Disconnect(int client) {
+  if (client == kDefaultHandleId) return;
   ScopedSuspendTracing suspend(tracing_controller_, this);
   writers_.erase(client);
   categories_.erase(client);
 }
 
-void Agent::Enable(const std::string& categories) {
+void Agent::Enable(int id, const std::set<std::string>& categories) {
   if (categories.empty())
     return;
-  std::set<std::string> categories_set;
-  std::istringstream category_list(categories);
-  while (category_list.good()) {
-    std::string category;
-    getline(category_list, category, ',');
-    categories_set.emplace(std::move(category));
-  }
-  Enable(categories_set);
-}
 
-void Agent::Enable(const std::set<std::string>& categories) {
-  if (categories.empty())
-    return;
-
-  file_writer_categories_.insert(categories.begin(), categories.end());
-  std::set<std::string> full_list(file_writer_categories_.begin(),
-                                  file_writer_categories_.end());
-  if (file_writer_.empty()) {
-    // Ensure background thread is running
-    Start();
-    std::unique_ptr<NodeTraceWriter> writer(
-        new NodeTraceWriter(log_file_pattern_, &tracing_loop_));
-    file_writer_ = AddClient(full_list, std::move(writer));
-  } else {
-    ScopedSuspendTracing suspend(tracing_controller_, this);
-    categories_[file_writer_.id_] = full_list;
-  }
+  ScopedSuspendTracing suspend(tracing_controller_, this,
+                               id != kDefaultHandleId);
+  categories_[id].insert(categories.begin(), categories.end());
 }
 
-void Agent::Disable(const std::set<std::string>& categories) {
+void Agent::Disable(int id, const std::set<std::string>& categories) {
+  ScopedSuspendTracing suspend(tracing_controller_, this,
+                               id != kDefaultHandleId);
+  std::multiset<std::string>& writer_categories = categories_[id];
   for (const std::string& category : categories) {
-    auto it = file_writer_categories_.find(category);
-    if (it != file_writer_categories_.end())
-      file_writer_categories_.erase(it);
+    auto it = writer_categories.find(category);
+    if (it != writer_categories.end())
+      writer_categories.erase(it);
   }
-  if (file_writer_.empty())
-    return;
-  ScopedSuspendTracing suspend(tracing_controller_, this);
-  categories_[file_writer_.id_] = { file_writer_categories_.begin(),
-                                    file_writer_categories_.end() };
 }
 
 TraceConfig* Agent::CreateTraceConfig() const {
@@ -178,5 +168,6 @@ void Agent::Flush(bool blocking) {
   for (const auto& id_writer : writers_)
     id_writer.second->Flush(blocking);
 }
+
 }  // namespace tracing
 }  // namespace node