Slow Reports - Embed Metadata in Traces

content/browser/tracing/background_tracing_manager_impl.cc

Index: content/browser/tracing/background_tracing_manager_impl.cc
diff --git a/content/browser/tracing/background_tracing_manager_impl.cc b/content/browser/tracing/background_tracing_manager_impl.cc
index 74d63800e6a63aa4c7a6225d162dcd5494e79298..7c06cbbd2a13db7f21caa44c40d0cac8e9d1ec35 100644
--- a/content/browser/tracing/background_tracing_manager_impl.cc
+++ b/content/browser/tracing/background_tracing_manager_impl.cc
@@ -4,8 +4,10 @@
 
 #include "content/browser/tracing/background_tracing_manager_impl.h"
 
+#include "base/json/json_writer.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_split.h"
 #include "base/time/time.h"
 #include "content/public/browser/background_tracing_preemptive_config.h"
 #include "content/public/browser/background_tracing_reactive_config.h"
@@ -21,6 +23,9 @@ namespace {
 base::LazyInstance<BackgroundTracingManagerImpl>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
 
+const char kMetaDataConfigKey[] = "config";
+const char kMetaDataVersionKey[] = "version";
+
 // These values are used for a histogram. Do not reorder.
 enum BackgroundTracingMetrics {
   SCENARIO_ACTIVATION_REQUESTED = 0,
@@ -370,11 +375,25 @@ void BackgroundTracingManagerImpl::OnFinalizeStarted(
   UMA_HISTOGRAM_MEMORY_KB("Tracing.Background.FinalizingTraceSizeInKB",
                           file_contents->size() / 1024);
 
-  if (!receive_callback_.is_null())
+  if (!receive_callback_.is_null()) {
+    std::map<std::string, std::string> metadata;
+
+    auto metadata_dict = GenerateMetadataDict();
+    if (metadata_dict) {
+      for (base::DictionaryValue::Iterator it(*metadata_dict); !it.IsAtEnd();
+           it.Advance()) {
+        std::string value;
+        if (!it.value().GetAsString(&value))
+          continue;
+        metadata.insert(std::make_pair(it.key(), value));
+      }
+    }
+
     receive_callback_.Run(
-        file_contents,
+        file_contents, metadata,
         base::Bind(&BackgroundTracingManagerImpl::OnFinalizeComplete,
                    base::Unretained(this)));
+  }
 }
 
 void BackgroundTracingManagerImpl::OnFinalizeComplete() {
@@ -398,6 +417,34 @@ void BackgroundTracingManagerImpl::OnFinalizeComplete() {
   RecordBackgroundTracingMetric(FINALIZATION_COMPLETE);
 }
 
+scoped_ptr<base::DictionaryValue>
+BackgroundTracingManagerImpl::GenerateMetadataDict() const {
+  // Grab the product version.
+  std::string product_version = GetContentClient()->GetProduct();
+  std::vector<std::string> product_components;
+  base::SplitString(product_version, '/', &product_components);
+  DCHECK_EQ(2U, product_components.size());
+  if (product_components.size() != 2U)
+    return nullptr;
+
+  product_version = product_components[1];

[oystein (OOO til 10th of July), 2015/06/12 19:29:16]

Is it worth cutting out the "Chrome/" part? Could we just as well include the
whole string from GetProduct() instead? The crash service uploader strips it to
be compatible with normal crash uploads, but we don't need to do that for
metadata embedded in the trace JSON.

[shatch, 2015/06/12 20:53:47]

Done.

+
+  // Serialize the config into json.
+  scoped_ptr<base::DictionaryValue> config_dict(new base::DictionaryValue());
+
+  BackgroundTracingConfig::IntoDict(config_.get(), config_dict.get());
+
+  std::string config_string;
+  if (!base::JSONWriter::Write(*config_dict.get(), &config_string))
+    return nullptr;
+
+  scoped_ptr<base::DictionaryValue> metadata_dict(new base::DictionaryValue());
+  metadata_dict->SetString(kMetaDataConfigKey, config_string);
+  metadata_dict->SetString(kMetaDataVersionKey, product_version);
+
+  return metadata_dict.Pass();
+}
+
 void BackgroundTracingManagerImpl::BeginFinalizing(
     StartedFinalizingCallback callback) {
   is_gathering_ = true;
@@ -413,6 +460,13 @@ void BackgroundTracingManagerImpl::BeginFinalizing(
     trace_data_sink = content::TracingController::CreateCompressedStringSink(
         data_endpoint_wrapper_);
     RecordBackgroundTracingMetric(FINALIZATION_ALLOWED);
+
+    auto metadata_dict = GenerateMetadataDict();
+    if (metadata_dict) {
+      std::string results;
+      if (base::JSONWriter::Write(*metadata_dict.get(), &results))

[dsinclair, 2015/06/12 19:38:51]

This is going to produce strange levels of encoding in the trace file.

On 438 we encoded the JSON  for the config into a string. We then wrote that
string into the metadata_dict map.

We then take that map and encode that into JSON. So, the JSON config will get
escaped a second time.

We're then going to take this string and encode it into JSON a third time when
we append it into the trace, giving another level of encoding.

That seems ..... odd. If we can have the JSON written correctly into the output
trace then it will make it a lot easier to deal with on the other side because
we parse the file and we're good. This way we have to parse the file then parse
this bit again to turn it into JS.

[shatch, 2015/06/12 20:53:48]

Ah didn't think about the double encoding, checked that the latest version just
writes this directly into the output trace and seems all good.

+        trace_data_sink->SetMetadata(results);
+    }
   } else {
     RecordBackgroundTracingMetric(FINALIZATION_DISALLOWED);
   }