Add basic tracing support to the gtest launcher

Add basic tracing support to the gtest launcher

R=primiano
TBR=thakis

BUG=619993
Committed: https://crrev.com/29ec06ac0501b79dc08de753ee9c52b5db7369d1
Cr-Commit-Position: refs/heads/master@{#403449}

[Paweł Hajdan Jr., 2016-07-01 10:18:48]

The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 10:18:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 10:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 10:25:37]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Paweł Hajdan Jr., 2016-07-01 10:59:34]

The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 10:59:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Paweł Hajdan Jr., 2016-07-01 11:18:04]

phajdan.jr@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-07-01 11:34:30]

LGTM with some minor comments.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.cc:254: const CommandLine& command_line,
If you want to you dump the command line as well.
The pattern here is to have an "args" argument to the event. The UI will
automatically show the detail of the event.
In other words:

{"pid": ..., "tid":...., args: {"command_line": "--foo --bar --baz"}}

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.cc:353: g_tracer.Get().RecordProcessExecution(
If this is really what you need it's ok. But if you need more events you can
consider having:
-  a TestLauncherTracer::ScopedEvent object which records one 'B' event in the
ctor and one 'B' event in the dtor
- A method g_tracer.RecordScopedEvent() which returns the ScopedEvent object. 
 so you can just add to your code like we do with TRACE_EVENT macros without
having to diff the time by hand.
But if this is really what you want, it's probably ok as it is.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.cc:1050: LOG(ERROR) << "Failed to save test
launcher trace.";
maybe move this error logging to the Dump method and make it void. Not sure in
how many other cases you really care about the success return code and want to
Dump() but ignore the eventual failure.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher_tracer.cc (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:27: event.thread_id =
PlatformThread::CurrentId();
Not sure you want this, as opposite as passing a hardcoded tid.
The practical difference is that, in the UI, each thread id shows as a separate
track. In other words the TID acts like a "namespace" for the events.
If you use this, all the events started from the same PID and TID will show on a
different track. But maybe this is what you want after all.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:28: events_.push_back(event);
It's not goting to make any major difference but technically you need to acquire
the lock only at this point to insert the event in the list, you don't really
need it before to construct the object.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:34: std::unique_ptr<ListValue>
json_events(new ListValue);
does this import in TraceViewer?
I thought you have to enclose the events in a "traceEvents" dictionary, as
follows:
{"traceEvents":[
  {pid: ..., tid;...},
  {pid: ..., tid;...},
]}

But maybe there is some legacy format that we still support that didn't require
the traceEvents container?

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher_tracer.h (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.h:35: std::string name;  // Displayed
name.
Is this git cl formatted? I thought that it does align the comments.

[commit-bot: I haz the power, 2016-07-01 12:02:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 12:02:36]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2016-07-01 12:47:32]

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.cc:353: g_tracer.Get().RecordProcessExecution(
On 2016/07/01 at 11:34:29, Primiano Tucci wrote:
> If this is really what you need it's ok. But if you need more events you can
consider having:
> -  a TestLauncherTracer::ScopedEvent object which records one 'B' event in the
ctor and one 'B' event in the dtor
> - A method g_tracer.RecordScopedEvent() which returns the ScopedEvent object. 
>  so you can just add to your code like we do with TRACE_EVENT macros without
having to diff the time by hand.
> But if this is really what you want, it's probably ok as it is.

Yeah, it's OK for now. I can take another look if I need something more
advanced.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.cc:1050: LOG(ERROR) << "Failed to save test
launcher trace.";
On 2016/07/01 at 11:34:30, Primiano Tucci wrote:
> maybe move this error logging to the Dump method and make it void. Not sure in
how many other cases you really care about the success return code and want to
Dump() but ignore the eventual failure.

This is consistent with above code. I think as an interface, it's preferred to
have bool return value rather than void and log inside.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher_tracer.cc (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:27: event.thread_id =
PlatformThread::CurrentId();
On 2016/07/01 at 11:34:30, Primiano Tucci wrote:
> Not sure you want this, as opposite as passing a hardcoded tid.
> The practical difference is that, in the UI, each thread id shows as a
separate track. In other words the TID acts like a "namespace" for the events.
> If you use this, all the events started from the same PID and TID will show on
a different track. But maybe this is what you want after all.

Yup, that's actually what I want. When I didn't use TID, the result was often
unreadable. It looks good to me when TID is passed.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:28: events_.push_back(event);
On 2016/07/01 at 11:34:30, Primiano Tucci wrote:
> It's not goting to make any major difference but technically you need to
acquire the lock only at this point to insert the event in the list, you don't
really need it before to construct the object.

Well, events_ is read above to compute event name. I think it's fine as is.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.cc:34: std::unique_ptr<ListValue>
json_events(new ListValue);
On 2016/07/01 at 11:34:30, Primiano Tucci wrote:
> does this import in TraceViewer?
> I thought you have to enclose the events in a "traceEvents" dictionary, as
follows:
> {"traceEvents":[
>   {pid: ..., tid;...},
>   {pid: ..., tid;...},
> ]}
> 
> But maybe there is some legacy format that we still support that didn't
require the traceEvents container?

Yeah, there's a format which takes just a list. It's mentioned in the doc you've
sent me.

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher_tracer.h (right):

https://codereview.chromium.org/2112253002/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher_tracer.h:35: std::string name;  // Displayed
name.
On 2016/07/01 at 11:34:30, Primiano Tucci wrote:
> Is this git cl formatted? I thought that it does align the comments.

Ah, that's a good point. I ran the CL through "git cl format" now.

[Paweł Hajdan Jr., 2016-07-01 12:49:06]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-07-01 12:49:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2112253002/#ps40001
(title: "format")

[commit-bot: I haz the power, 2016-07-01 12:49:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 12:55:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 12:55:30]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Paweł Hajdan Jr., 2016-07-01 12:57:39]

Description was changed from

==========
Add basic tracing support to the gtest launcher

BUG=619993
==========

to

==========
Add basic tracing support to the gtest launcher

R=primiano
TBR=thakis

BUG=619993
==========

[Paweł Hajdan Jr., 2016-07-01 12:58:15]

phajdan.jr@chromium.org changed reviewers:
+ thakis@chromium.org

[Paweł Hajdan Jr., 2016-07-01 12:58:15]

+thakis (TBR) for trivial base/base.gyp change (adding .h and .cc file in
base/test)

[Paweł Hajdan Jr., 2016-07-01 12:58:19]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-07-01 12:58:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2112253002/#ps60001
(title: "sort")

[commit-bot: I haz the power, 2016-07-01 12:58:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 14:11:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-01 14:11:25]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 14:13:21]

Description was changed from

==========
Add basic tracing support to the gtest launcher

R=primiano
TBR=thakis

BUG=619993
==========

to

==========
Add basic tracing support to the gtest launcher

R=primiano
TBR=thakis

BUG=619993

Committed: https://crrev.com/29ec06ac0501b79dc08de753ee9c52b5db7369d1
Cr-Commit-Position: refs/heads/master@{#403449}
==========

[commit-bot: I haz the power, 2016-07-01 14:13:22]

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