Slow Reports - Embed Metadata in Traces

Slow Reports - Embed Metadata in Traces

This CL adds the ability to set metadata to be included in the finalized version of the trace, before it gets compressed. Additionally, we can also set metadata to be included with a trace when it's uploaded by the TraceCrashServiceUploader.

Right now we just include version and the serialized config with the uploaded trace.

BUG=499873
Committed: https://crrev.com/0cc9fee6a184d34aa38e9930a9d77a4fa833d720
Cr-Commit-Position: refs/heads/master@{#334477}

[shatch, 2015-06-12 16:47:31]

Patchset #1 (id:1) has been deleted

[oystein (OOO til 10th of July), 2015-06-12 18:14:15]

oysteine@chromium.org changed reviewers:
+ oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-06-12 18:14:17]

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:72:
base::SplitString(version_info.ProductNameAndVersionForUserAgent(), '/',
Why have this twice now?

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.h (right):

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.h:63: void SetupMultipart(const
std::map<std::string, std::string> metadata,
&?

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:380: auto
metadata_dict = GenerateMetadataDict();
Why not pass the base::DictionaryValue around directly, instead of converting to
map<string, string>?

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:387:
metadata.insert(std::pair<std::string, std::string>(it.key(), value));
nit: std::make_pair() maybe?

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:422: if
(!delegate_->GetChromeVersion(&chrome_version))
Shouldn't be "chrome"; maybe product_version.

https://codereview.chromium.org/1181213002/diff/20001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1181213002/diff/20001/content/public/browser/...
content/public/browser/tracing_delegate.h:35: virtual bool
GetChromeVersion(std::string*) = 0;
We have this in the content client already, I'm pretty sure? GetProduct(). Also,
"Chrome" shouldn't be mentioned in content/.

[shatch, 2015-06-12 19:00:23]

Patchset #2 (id:40001) has been deleted

[shatch, 2015-06-12 19:05:55]

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:72:
base::SplitString(version_info.ProductNameAndVersionForUserAgent(), '/',
On 2015/06/12 18:14:16, Oystein wrote:
> Why have this twice now?

This was actually just moved from earlier in the file. Needed 2 since
TracingCrashServiceUploader wants to add version info, and since this is
technically also used by something else we need to supply version info here. The
one in the chrome_tracing_delegate is gone now, replaced with your ContentClient
suggestion.

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.h (right):

https://codereview.chromium.org/1181213002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.h:63: void SetupMultipart(const
std::map<std::string, std::string> metadata,
On 2015/06/12 18:14:16, Oystein wrote:
> &?

Done.

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:380: auto
metadata_dict = GenerateMetadataDict();
On 2015/06/12 18:14:17, Oystein wrote:
> Why not pass the base::DictionaryValue around directly, instead of converting
to
> map<string, string>?

Figured it looked cleaner and easier to work with vs passing the dict around.
TraceCrashServiceUploader just wants a bunch of string/string pairs to attach to
the upload, so felt like it shouldn't have to deal with some json dict. We could
pass the dict up there as a param of DoUpload(), and move this iteration code
into the uploader if you feel strongly about this.

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:387:
metadata.insert(std::pair<std::string, std::string>(it.key(), value));
On 2015/06/12 18:14:17, Oystein wrote:
> nit: std::make_pair() maybe?

Done.

https://codereview.chromium.org/1181213002/diff/20001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1181213002/diff/20001/content/public/browser/...
content/public/browser/tracing_delegate.h:35: virtual bool
GetChromeVersion(std::string*) = 0;
On 2015/06/12 18:14:17, Oystein wrote:
> We have this in the content client already, I'm pretty sure? GetProduct().
Also,
> "Chrome" shouldn't be mentioned in content/.

Sweet, tried to find where I could get it in content.

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

lgtm with comments!

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:380: auto
metadata_dict = GenerateMetadataDict();
On 2015/06/12 19:05:54, shatch wrote:
> On 2015/06/12 18:14:17, Oystein wrote:
> > Why not pass the base::DictionaryValue around directly, instead of
converting
> to
> > map<string, string>?
> 
> Figured it looked cleaner and easier to work with vs passing the dict around.
> TraceCrashServiceUploader just wants a bunch of string/string pairs to attach
to
> the upload, so felt like it shouldn't have to deal with some json dict. We
could
> pass the dict up there as a param of DoUpload(), and move this iteration code
> into the uploader if you feel strongly about this.

Up to you, it just feels like a bit of an unnecessary step atm.

Basically, right now it's:
loop through to convert -> pass -> loop through
instead of just
pass -> loop through (assuming the uploader gets a json dict member too.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:430: product_version
= product_components[1];
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.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl_data_sinks.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:269: if
(!metadata_.empty())
nit: braces

https://codereview.chromium.org/1181213002/diff/60001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1181213002/diff/60001/content/public/browser/...
content/public/browser/tracing_delegate.h:8: #include <string>
nit: remove, if no longer used?

[dsinclair, 2015-06-12 19:38:50]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

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

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:467: if
(base::JSONWriter::Write(*metadata_dict.get(), &results))
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.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl_data_sinks.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:141:
AddTraceChunkAndPassToEndpoint(",\"metadata\": " + metadata_);
Does this produce a valid file? This works above for system_trace_ because it's
an encoded array, so has [] around the content. With metadata_ it's just a
string, does it have the correct "" around the content? (And, if it doesn't, we
have to make sure it's escaped before we can add it)

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

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:380: auto
metadata_dict = GenerateMetadataDict();
On 2015/06/12 19:29:16, Oystein wrote:
> On 2015/06/12 19:05:54, shatch wrote:
> > On 2015/06/12 18:14:17, Oystein wrote:
> > > Why not pass the base::DictionaryValue around directly, instead of
> converting
> > to
> > > map<string, string>?
> > 
> > Figured it looked cleaner and easier to work with vs passing the dict
around.
> > TraceCrashServiceUploader just wants a bunch of string/string pairs to
attach
> to
> > the upload, so felt like it shouldn't have to deal with some json dict. We
> could
> > pass the dict up there as a param of DoUpload(), and move this iteration
code
> > into the uploader if you feel strongly about this.
> 
> Up to you, it just feels like a bit of an unnecessary step atm.
> 
> Basically, right now it's:
> loop through to convert -> pass -> loop through
> instead of just
> pass -> loop through (assuming the uploader gets a json dict member too.

Done.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:430: product_version
= product_components[1];
On 2015/06/12 19:29:16, Oystein wrote:
> 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.

Done.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:467: if
(base::JSONWriter::Write(*metadata_dict.get(), &results))
On 2015/06/12 19:38:51, dsinclair wrote:
> 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.

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.

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl_data_sinks.cc (right):

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:141:
AddTraceChunkAndPassToEndpoint(",\"metadata\": " + metadata_);
On 2015/06/12 19:38:51, dsinclair wrote:
> Does this produce a valid file? This works above for system_trace_ because
it's
> an encoded array, so has [] around the content. With metadata_ it's just a
> string, does it have the correct "" around the content? (And, if it doesn't,
we
> have to make sure it's escaped before we can add it)

It's actually a dict, so the contents the results are more like:

"metadata": {"foo1": "1"}

https://codereview.chromium.org/1181213002/diff/60001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:269: if
(!metadata_.empty())
On 2015/06/12 19:29:16, Oystein wrote:
> nit: braces

Done.

https://codereview.chromium.org/1181213002/diff/60001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1181213002/diff/60001/content/public/browser/...
content/public/browser/tracing_delegate.h:8: #include <string>
On 2015/06/12 19:29:16, Oystein wrote:
> nit: remove, if no longer used?

Done.

[dsinclair, 2015-06-13 00:41:21]

lgtm w/ nit

https://codereview.chromium.org/1181213002/diff/80001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1181213002/diff/80001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:440: if
(metadata_dict) {
if (auto metadata_dict = GenerateMetadataDict()) {

[shatch, 2015-06-15 21:13:07]

simonhatch@chromium.org changed reviewers:
+ sievers@chromium.org

[shatch, 2015-06-15 21:14:25]

+sievers for content/public/browser

[no sievers, 2015-06-15 21:45:52]

On 2015/06/15 21:14:25, shatch wrote:
> +sievers for content/public/browser

lgtm

[shatch, 2015-06-15 21:49:18]

The CQ bit was checked by simonhatch@chromium.org

[shatch, 2015-06-15 21:49:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1181213002/#ps100001
(title: "Comments addressed.")

[commit-bot: I haz the power, 2015-06-15 21:50:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181213002/100001

[commit-bot: I haz the power, 2015-06-15 22:26:56]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-06-15 22:28:46]

Patchset 4 (id:??) landed as
https://crrev.com/0cc9fee6a184d34aa38e9930a9d77a4fa833d720
Cr-Commit-Position: refs/heads/master@{#334477}