tracing: the client lib of the tracing service

services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/resource_coordinator/public/cpp/tracing/trace_event_agent.h"
+
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/strings/string_util.h"
+#include "base/trace_event/trace_log.h"
+#include "base/values.h"
+#include "services/service_manager/public/cpp/connector.h"
+
+namespace resource_coordinator {
+namespace tracing {
+
+// static
+TraceEventAgent* TraceEventAgent::GetOrCreateInstance(

[Primiano Tucci (use gerrit), 2017/05/11 15:31:07]

Not sure what the final intended use of this, but I wonder if this should be
really just CreateInstance?
I fully understand that this pattern avoids to have a getinstance + initialize,
which removes lot of problems.
my only question is that if you intend to have the "get" semantic, do the client
have to keep passing the agent_set?
Also, if you really want the "get" semantic this starts looking racy.
But I feel that what you want to do here is just invoking this method once in
the various main()s, in which case is just a naming issue

[chiniforooshan, 2017/05/15 20:44:11]

I need a CreateInstance(agent_registry) which will be called in the main of
different children, like the browser, renderers, or other services. I need a
GetInstance() for adding MetadataGeneratorFunctions, probably just in the
browser and/or service manager processes. The thing is that GetInstance will
always be run right after CreateInstance because we can add
MetadataGeneratorFunctions right at the beginning. So merging them will save a
couple of lines of code.

The client lib is not thread safe. Mojo calls are always made from the same
thread on which the client is bound (i.e. the thread that ran the ctor). I added
a ThreadChecker to make sure that AddMetadataGeneratorFunctions is called on the
same thread, too.

[Primiano Tucci (use gerrit), 2017/05/16 04:45:11]

If this is the case can you just make this TraceEventAgent*
CreateInstance(agent_set) and inside that check that this is called only once.
If we then need an extra Get, we can create a separate getter-only
TraceEventAgent* GetInstance().
The reason I am advocating about this is because in the past we have been quite
bitten by the GetInstance() model that lazily brings up the MemoryDumpManager /
TracingController / TraceLog. Essentially today there are a bounch of places
that invoke that and is impossible to easily understand in which order they are
created. In fact depending on the combination of --enable-heap-profiling and
--startup-trace that you use they get initialized in different order (because
the GetInstance() get called in a different sequence) and that has been a huge
mainteinance burdern.

Hence my suggestion of making it clear when the instance is created (and
returned, that is fine) vs just accessed..
Ack, thanks

[chiniforooshan, 2017/05/16 15:14:44]

But, this is not guaranteed to be called once. This will be called in the start
of services and if we bundle several services in one process, this will be
called several times, until we have a better way of initializing these
one-per-process libs in the service manager (or is there a way today for this?).
I tried to explain this in the design doc, but I should make the explanation
more clear).

[Primiano Tucci (use gerrit), 2017/05/17 00:28:17]

Okay I think I am still missing this point. Realistically, even before we have a
way to initialize this in the service manager, I expect this to be called only
from child_thread::main.
Is there a realistic case where we risk to initialize this more than once? If
not we should just have a CreateInstance(...) which has a DCHECK against
double-initialization, and revisit that if we have an actual use case.

[chiniforooshan, 2017/05/17 06:33:46]

Agree, let's differ this discussion to the CL that needs this. For now, I just
add a GetInstance() method. Creator of this lib, e.g. ChildThreadImpl, should
manage its lifetime, which should be just storing it in an internal
std::unique_ptr variable. There is a DCHECK that verifies no more than one
instance is live at any time in each process.

+    mojom::AgentSetPtr agent_set) {
+  static TraceEventAgent* instance = nullptr;
+  if (!instance) {
+    DCHECK(agent_set);
+    instance = new TraceEventAgent(std::move(agent_set));
+  }
+  return instance;
+}
+
+TraceEventAgent::TraceEventAgent(mojom::AgentSetPtr agent_set)
+    : binding_(this) {
+  // agent_set can be null in tests. The constructor is private and cannot be
+  // directly used in non-test scenarios. GetOrCreateInstance makes sure that
+  // the constructor is called with a non-null agent_set.
+  if (!agent_set)
+    return;
+  agent_set->RegisterAgent(binding_.CreateInterfacePtrAndBind(), "traceEvents",
+                           mojom::TraceDataType::ARRAY,
+                           false /* supports_explicit_clock_sync */);
+}
+
+TraceEventAgent::~TraceEventAgent() {
+  if (base::trace_event::TraceLog::GetInstance()->IsEnabled())
+    StopAndFlush();
+}
+
+void TraceEventAgent::AddMetadataGenerator(MetadataGenerator generator) {
+  metadata_generators_.push_back(generator);
+}
+
+void TraceEventAgent::StartTracing(const std::string& config,
+                                   mojom::RecorderPtr recorder,
+                                   bool report_categories_only,

[Primiano Tucci (use gerrit), 2017/05/11 15:31:07]

Hmm honestly this report_categories logic seems a bit odd here.
I think understand what you are doing, but essentially here you are splashing
the complexity on the client, which has to do start(report=true) and then
immediately stop.
Shouldn't we instead have a dedicated GetCategories method which, under the
hoods, does the start+stop?
Maybe that's a little bit cleaner?

Or are there genuine cases where we want both the trace data AND the categories
together?

[chiniforooshan, 2017/05/15 20:44:11]

Done.

+                                   const StartTracingCallback& callback) {
+  DCHECK(!recorder_);
+  recorder_ = std::move(recorder);
+  report_categories_only_ = report_categories_only;
+  if (!base::trace_event::TraceLog::GetInstance()->IsEnabled()) {
+    base::trace_event::TraceLog::GetInstance()->SetEnabled(
+        base::trace_event::TraceConfig(config),
+        base::trace_event::TraceLog::RECORDING_MODE);
+  }
+  callback.Run();
+}
+
+void TraceEventAgent::StopAndFlush() {
+  base::trace_event::TraceLog::GetInstance()->SetDisabled();
+  if (!recorder_)
+    return;
+  if (report_categories_only_) {
+    std::vector<std::string> category_vector;
+    base::trace_event::TraceLog::GetInstance()->GetKnownCategoryGroups(
+        &category_vector);
+    recorder_->AddCategories(base::JoinString(category_vector, ","));
+  }
+  base::trace_event::TraceLog::GetInstance()->Flush(
+      base::Bind(&TraceEventAgent::SendChunk, base::Unretained(this)));
+}
+
+void TraceEventAgent::RequestClockSyncMarker(
+    const std::string& sync_id,
+    const RequestClockSyncMarkerCallback& callback) {
+  NOTREACHED() << "This agent does not support explicit clock sync";
+}
+
+void TraceEventAgent::RequestBufferStatus(
+    const RequestBufferStatusCallback& callback) {
+  auto status = base::trace_event::TraceLog::GetInstance()->GetStatus();
+  callback.Run(status.event_capacity, status.event_count);
+}
+
+void TraceEventAgent::SendChunk(
+    const scoped_refptr<base::RefCountedString>& events_str,
+    bool has_more_events) {
+  DCHECK(recorder_);
+  if (!events_str->data().empty() && !report_categories_only_)
+    recorder_->AddChunk(events_str->data());
+  if (!has_more_events) {
+    if (!report_categories_only_) {
+      for (const auto& generator : metadata_generators_) {
+        auto metadata = generator.Run();
+        if (metadata)
+          recorder_->AddMetadata(std::move(metadata));
+      }
+    }
+    recorder_.reset();
+  }
+}
+
+}  // namespace tracing
+}