Support traces containing all parts/agents returned via DevTools streams

Support traces containing all parts/agents returned via DevTools streams

This prepares telemetry for a change in the format of data returned via
DevTools streams. The new format is consistent with trace files and
includes traces from all sources formatted as a JSON object.

BUG=chromium:599932
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ffc12951e69773de438534abd9055e2bd57a35da

[caseq, 2016-07-19 00:19:12]

caseq@chromium.org changed reviewers:
+ nednguyen@google.com

[nednguyen, 2016-07-19 10:08:04]

nednguyen@google.com changed reviewers:
+ charliea@chromium.org, zhenw@chromium.org

[nednguyen, 2016-07-19 10:08:05]

I defer reviewing this to Zhen & Charlie

[Zhen Wang, 2016-07-19 17:45:48]

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
File
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py
(right):

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:175:
'IO.read', {'data': '"systemTraceEvents": [{}]'}, 3)
Can you change this to test "metadata"? Telemetry controls system tracing by
itself and never enables systrace in Chrome. So Chrome will never send
systemTraceEvents back to Telemetry. Using this in the test may confuse people.

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:184:
system_trace_events = data.GetTraceFor(trace_data.ATRACE_PART)
I don't see trace_data.METADATA_PART. Is that in a separate CL?

[caseq, 2016-07-19 18:06:00]

On 2016/07/19 17:45:48, Zhen Wang wrote:
>
https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
> File
>
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py
> (right):
> 
>
https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
>
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:175:
> 'IO.read', {'data': '"systemTraceEvents": [{}]'}, 3)
> Can you change this to test "metadata"? Telemetry controls system tracing by
> itself and never enables systrace in Chrome. So Chrome will never send
> systemTraceEvents back to Telemetry. Using this in the test may confuse
people.
> 
>
https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
>
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:184:
> system_trace_events = data.GetTraceFor(trace_data.ATRACE_PART)
> I don't see trace_data.METADATA_PART.

That's the reason the test uses systrace. I was not even sure if we want
metadata to be handled generically as a trace part -- though this is certainly
an option.

> Is that in a separate CL?

Metadata are supposed to be covered by a separate CL. I would add those at once,
but I have little idea on where and how metadata will be used in the telemetry,
so rather than adding a stub that would remain the dead code till usages are
added, I'm doing a minimal CL that would lets us land the back-end change
(https://codereview.chromium.org/2161583004/).

[Zhen Wang, 2016-07-19 18:30:34]

> Metadata are supposed to be covered by a separate CL. I would add those at
once,
> but I have little idea on where and how metadata will be used in the
telemetry,
> so rather than adding a stub that would remain the dead code till usages are
> added, I'm doing a minimal CL that would lets us land the back-end change
> (https://codereview.chromium.org/2161583004/).

I am referring to metadata part on the Telemetry side:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

You can add METADATA_PART there.

Ned and Charlie, I have one question for the metadata part in Telemetry. Current
CL adds all fields from Chrome trace to top level fields in trace_data_builder.
Mostly, it should be fine. But for metadata, do we have any problem of collided
names? That is: does Telemetry produce metadata and should that be top level or
under Telemetry part? I see current hack is to do it under Telemetry part, but
that feels strange to me:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:270:
if type(trace) == dict and 'traceEvents' in trace:
s/'traceEvents'/trace_data_module.CHROME_TRACE_PART

[charliea (OOO until 10-5), 2016-07-19 20:38:46]

On 2016/07/19 at 18:30:34, zhenw wrote:
> > Metadata are supposed to be covered by a separate CL. I would add those at
once,
> > but I have little idea on where and how metadata will be used in the
telemetry,
> > so rather than adding a stub that would remain the dead code till usages are
> > added, I'm doing a minimal CL that would lets us land the back-end change
> > (https://codereview.chromium.org/2161583004/).
> 
> I am referring to metadata part on the Telemetry side:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> You can add METADATA_PART there.
> 
> Ned and Charlie, I have one question for the metadata part in Telemetry.
Current CL adds all fields from Chrome trace to top level fields in
trace_data_builder. Mostly, it should be fine. But for metadata, do we have any
problem of collided names? That is: does Telemetry produce metadata and should
that be top level or under Telemetry part? I see current hack is to do it under
Telemetry part, but that feels strange to me:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

Nope: putting the metadata under the Telemetry part is intentional and not
considered a hack. The idea is that each subtrace might have corresponding
metadata, like a Chrome version to accompany a Chrome trace or a clock domain
for each subtrace. Having per-trace metadata rather than global metadata allows
us to do this. I think there's a distinct possibility that we'll need some sort
of global metadata in the future, but we'd like to punt on it for as long as
possible because it makes composing subtraces to make a single trace a lot
harder.

[nednguyen, 2016-07-19 20:44:06]

On 2016/07/19 20:38:46, charliea wrote:
> On 2016/07/19 at 18:30:34, zhenw wrote:
> > > Metadata are supposed to be covered by a separate CL. I would add those at
> once,
> > > but I have little idea on where and how metadata will be used in the
> telemetry,
> > > so rather than adding a stub that would remain the dead code till usages
are
> > > added, I'm doing a minimal CL that would lets us land the back-end change
> > > (https://codereview.chromium.org/2161583004/).
> > 
> > I am referring to metadata part on the Telemetry side:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> > 
> > You can add METADATA_PART there.
> > 
> > Ned and Charlie, I have one question for the metadata part in Telemetry.
> Current CL adds all fields from Chrome trace to top level fields in
> trace_data_builder. Mostly, it should be fine. But for metadata, do we have
any
> problem of collided names? That is: does Telemetry produce metadata and should
> that be top level or under Telemetry part? I see current hack is to do it
under
> Telemetry part, but that feels strange to me:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> Nope: putting the metadata under the Telemetry part is intentional and not
> considered a hack. The idea is that each subtrace might have corresponding
> metadata, like a Chrome version to accompany a Chrome trace or a clock domain
> for each subtrace. Having per-trace metadata rather than global metadata
allows
> us to do this. I think there's a distinct possibility that we'll need some
sort
> of global metadata in the future, but we'd like to punt on it for as long as
> possible because it makes composing subtraces to make a single trace a lot
> harder.

What Charlie says is the current design I have in mind as well. Metadata is a
per-trace thing.

[Zhen Wang, 2016-07-19 21:09:25]

> > Nope: putting the metadata under the Telemetry part is intentional and not
> > considered a hack. The idea is that each subtrace might have corresponding
> > metadata, like a Chrome version to accompany a Chrome trace or a clock
domain
> > for each subtrace. Having per-trace metadata rather than global metadata
> allows
> > us to do this. I think there's a distinct possibility that we'll need some
> sort
> > of global metadata in the future, but we'd like to punt on it for as long as
> > possible because it makes composing subtraces to make a single trace a lot
> > harder.
> 
> What Charlie says is the current design I have in mind as well. Metadata is a
> per-trace thing.

Currently we have:

CHROME_TRACE_PART = TraceDataPart('traceEvents')

If we want to make metadata be part of Chrome trace, does it mean we need to do:

CHROME_TRACE_PART = TraceDataPart('chrome')

and then this data part contains 'traceEvents', 'metadata', etc? Does trace
viewer support this?

[caseq, 2016-07-19 21:48:50]

On 2016/07/19 20:38:46, charliea wrote:

> Nope: putting the metadata under the Telemetry part is intentional and not
> considered a hack. The idea is that each subtrace might have corresponding
> metadata, like a Chrome version to accompany a Chrome trace or a clock domain
> for each subtrace. Having per-trace metadata rather than global metadata
allows
> us to do this. I think there's a distinct possibility that we'll need some
sort
> of global metadata in the future, but we'd like to punt on it for as long as
> possible because it makes composing subtraces to make a single trace a lot
> harder.

Note that this does not match the design on the back-end -- specifically, the
metadata that we support in TraceDataSink and TraceDataEndpoint is on the same
level as all the traces, including chrome one (i.e. traceEvents). We also do not
have any provision for metadata within the traceEvents trace, sine this is an
array of events.

[caseq, 2016-07-20 00:50:04]

Description was changed from

==========
Support traces containing all parts/agents returned via DevTools streams

This prepares telemetry for a change in the format of data returned via
DevTools streams. The new format is consistent with trace files and
includes traces from all sources formatted as a JSON object.

BUG=crbug.com/599932
==========

to

==========
Support traces containing all parts/agents returned via DevTools streams

This prepares telemetry for a change in the format of data returned via
DevTools streams. The new format is consistent with trace files and
includes traces from all sources formatted as a JSON object.

BUG=chromium:599932
==========

[caseq, 2016-07-20 01:21:42]

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:270:
if type(trace) == dict and 'traceEvents' in trace:
On 2016/07/19 18:30:34, Zhen Wang wrote:
> s/'traceEvents'/trace_data_module.CHROME_TRACE_PART

Removed the second clause altogether, I don't think we need it.

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
File
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py
(right):

https://codereview.chromium.org/2160793003/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:175:
'IO.read', {'data': '"systemTraceEvents": [{}]'}, 3)
On 2016/07/19 17:45:48, Zhen Wang wrote:
> Can you change this to test "metadata"? Telemetry controls system tracing by
> itself and never enables systrace in Chrome. So Chrome will never send
> systemTraceEvents back to Telemetry. Using this in the test may confuse
people.

Replaced this with battor one to reduce confusion. I really don't want to turn
this into "support metadata in telemetry" CL, I'd like it to be minimal CL
required to support the back-end change, however trivial the former might be.
Actual existence of metadata in the returned stream is already tested by an
inspector-protocol test.

[Zhen Wang, 2016-07-20 17:21:49]

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:271:
for part in trace_data_module.ALL_TRACE_PARTS:
Ned and Charlie, can you take a look at this? This makes sense to me. But I
think it will contradict with your idea of having metadata being part of Chrome
trace part. We'd better have a consensus on how to handle metadata here.

I see another option is to make chrome trace part being 'chrome' instead of
'traceEvent' in Telemetry. Then all chrome generated traces will be put into
'chrome' and 'traceEvent', 'metadata' and other things can be nested under it.
But I think this will require some trace viewer change.

[nednguyen, 2016-07-20 17:26:11]

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:271:
for part in trace_data_module.ALL_TRACE_PARTS:
On 2016/07/20 17:21:49, Zhen Wang wrote:
> Ned and Charlie, can you take a look at this? This makes sense to me. But I
> think it will contradict with your idea of having metadata being part of
Chrome
> trace part. We'd better have a consensus on how to handle metadata here.
> 
> I see another option is to make chrome trace part being 'chrome' instead of
> 'traceEvent' in Telemetry. Then all chrome generated traces will be put into
> 'chrome' and 'traceEvent', 'metadata' and other things can be nested under it.
> But I think this will require some trace viewer change.

I think 'traceEvent' is reserved for chrome trace only in
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

Can you elaborate on why this part contradict with the idea of Chrome's metadata
stays in Chrome trace part?

[caseq, 2016-07-20 17:27:27]

On 2016/07/20 17:21:49, Zhen Wang wrote:
>
https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
> File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
> (right):
> 
>
https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
> telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:271:
> for part in trace_data_module.ALL_TRACE_PARTS:
> Ned and Charlie, can you take a look at this? This makes sense to me. But I
> think it will contradict with your idea of having metadata being part of
Chrome
> trace part. We'd better have a consensus on how to handle metadata here.
> 
> I see another option is to make chrome trace part being 'chrome' instead of
> 'traceEvent' in Telemetry. Then all chrome generated traces will be put into
> 'chrome' and 'traceEvent', 'metadata' and other things can be nested under it.
> But I think this will require some trace viewer change.

If we change format, we better do it once -- so +1 to Zhen, if we really want
metadata to be per trace source, let's have something like 
{ 
    chrome: {metadata: {}, traceEvents: [...]}
}
But I wouldn't like to have a different format just for Telemetry -- so let's
change this for all clients, i.e. background tracing and trace viewer as well?

[Zhen Wang, 2016-07-20 17:30:14]

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:271:
for part in trace_data_module.ALL_TRACE_PARTS:
On 2016/07/20 17:26:11, nednguyen wrote:
> On 2016/07/20 17:21:49, Zhen Wang wrote:
> > Ned and Charlie, can you take a look at this? This makes sense to me. But I
> > think it will contradict with your idea of having metadata being part of
> Chrome
> > trace part. We'd better have a consensus on how to handle metadata here.
> > 
> > I see another option is to make chrome trace part being 'chrome' instead of
> > 'traceEvent' in Telemetry. Then all chrome generated traces will be put into
> > 'chrome' and 'traceEvent', 'metadata' and other things can be nested under
it.
> > But I think this will require some trace viewer change.
> 
> I think 'traceEvent' is reserved for chrome trace only in
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> Can you elaborate on why this part contradict with the idea of Chrome's
metadata
> stays in Chrome trace part?

So now Chrome can give you a dict like this:

{
  "traceEvents": [...],
  "displayTimeUnit": "ns",
  "systemTraceEvents": "SystemTraceData",
  "otherData": {
    "version": "My Application v1.0"
  },
  "stackFrames": {...}
  "samples": [...],
  "metadata": {...}
}

You see metadata is at the same level with traceEvents. By looping all trace
parts, since we do not have metadata part now, metadata will be dropped. If we
add metadata part, it becomes at the same level with traceEvent, which is
global.

[caseq, 2016-07-20 17:30:37]

> 
> I think 'traceEvent' is reserved for chrome trace only in
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> Can you elaborate on why this part contradict with the idea of Chrome's
metadata
> stays in Chrome trace part?

We could easily pretend this is so in Telemetry, but in actual JSON trace
container format (i.e. the one presently used by the trace viewer and background
tracing) the consumers expect traceEvents to be an array of events and the
metadata is global.

[nednguyen, 2016-07-20 17:41:27]

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:271:
for part in trace_data_module.ALL_TRACE_PARTS:
On 2016/07/20 17:30:13, Zhen Wang wrote:
> On 2016/07/20 17:26:11, nednguyen wrote:
> > On 2016/07/20 17:21:49, Zhen Wang wrote:
> > > Ned and Charlie, can you take a look at this? This makes sense to me. But
I
> > > think it will contradict with your idea of having metadata being part of
> > Chrome
> > > trace part. We'd better have a consensus on how to handle metadata here.
> > > 
> > > I see another option is to make chrome trace part being 'chrome' instead
of
> > > 'traceEvent' in Telemetry. Then all chrome generated traces will be put
into
> > > 'chrome' and 'traceEvent', 'metadata' and other things can be nested under
> it.
> > > But I think this will require some trace viewer change.
> > 
> > I think 'traceEvent' is reserved for chrome trace only in
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > 
> > Can you elaborate on why this part contradict with the idea of Chrome's
> metadata
> > stays in Chrome trace part?
> 
> So now Chrome can give you a dict like this:
> 
> {
>   "traceEvents": [...],
>   "displayTimeUnit": "ns",
>   "systemTraceEvents": "SystemTraceData",
>   "otherData": {
>     "version": "My Application v1.0"
>   },
>   "stackFrames": {...}
>   "samples": [...],
>   "metadata": {...}
> }
> 
> You see metadata is at the same level with traceEvents. By looping all trace
> parts, since we do not have metadata part now, metadata will be dropped. If we
> add metadata part, it becomes at the same level with traceEvent, which is
> global.

Oh I see. I think we can just use trace_data_builder.SetTraceFor(chrome_part,
"<the whole chrome trace as string>")

There is no need to un-nest the sub traces in traceEvents to the top level, if I
recall correctly about the spec of trace container?

If I remember this wrong, we can just handle the "metadata" differently from
others field so that it stays in Chrome part?

[charliea (OOO until 10-5), 2016-07-20 17:50:21]

Gah, sorry: I thought I replied to this code review yesterday, but I must never
have hit submit.

The trace format is when a trace comes in as a javascript object that can
contain up to two fields:

{
  traceEvents: [  /* An array of trace events */ ],
  metadata: { /* A dictionary from metadata field to value */ }
}

Multiple subtraces can make up a single trace by being zipped (now) or TARed
(to-be-implemented

> You see metadata is at the same level with traceEvents. By looping all trace
parts, 
> since we do not have metadata part now, metadata will be dropped. If we add 
> metadata part, it becomes at the same level with traceEvent, which is global.

I don't think that traceEvent is global: it only contains events for a single
trace (Chrome). I think that, in the long run, we want to get rid of the JSON
trace container format altogether and just .zip or .tar up multiple traces
together.

That would mean that, if we had a Telemetry trace, a Chrome trace, and a BattOr
trace that, together, make up a single trace, that looks like:

trace.[zip,tar]

  --- chrome.ctrace.json 
      
      {
        traceEvents: [ /* Chrome trace events go here */ ],
        metadata: { /* Chrome metadata dictionary goes here */ }
      }

  --- telemetry.ctrace.json

      {
        traceEvents: [ /* Telemetry trace events go here */ ],
        metadata: { /* Telmetry metadata dictionary goes here */ }
      }       
 
  --- battor.battor

      # BattOr
      # voltage_range [0.0, 6144.0] mV
      # current_range [0.0, 2275.5] mA
      # sample_rate 2000 Hz, gain 5.0x
      0.000000 0.000000 4000.000000
      0.500000 0.000000 4000.000000
      ...

A couple of important notes: 
 
  - The type of the trace should be encoded in the filename. ctrace.json is just
an example of what the file extension might be for what's currently called the
"trace event format". The name for this format is widely thought to be
confusing, though, so it might be changed to something like "json trace format"
or "chrome trace format" or "ctrace" or something else.

  - Each trace type is responsible for coming up with its own way of storing
metadata, which the importers will understand.

[charliea (OOO until 10-5), 2016-07-20 17:53:39]

One important mistake that I made in the last post:

> That would mean that, if we had a Telemetry trace, a Chrome trace, and a 
> BattOr trace that, together, make up a single trace, that looks like:

should have said

> That would mean that, if we had a Telemetry subtrace, a Chrome subtrace, and a

> BattOr subtrace that, together, make up a single trace, that looks like:

To make things clearer, "subtrace" should be used to refer to a complete set of
one tracing agent's events (with metadata). A "trace" is one or more subtraces
that combine to form a more complete view of what was happening when the trace
was recorded.

[caseq, 2016-07-20 18:15:35]

On 2016/07/20 17:50:21, charliea wrote:
> Gah, sorry: I thought I replied to this code review yesterday, but I must
never
> have hit submit.
> 
> The trace format is when a trace comes in as a javascript object that can
> contain up to two fields:
> 
> {
>   traceEvents: [  /* An array of trace events */ ],
>   metadata: { /* A dictionary from metadata field to value */ }
> }
> 
> Multiple subtraces can make up a single trace by being zipped (now) or TARed
> (to-be-implemented

This is not how tracing controller currently packages traces. This CL has added
support for multiple trace "agents" in tracing controller:

https://chromium.googlesource.com/chromium/src/+/b0b2a7f0fd7f6ec4a43f735e643a...

and as I you can see, these come at the same level as traceEvents and metadata.
Moreover, we used to have more than two values there even before this CL. FWIW,
I've recently refactored a lot of this code, but the format remains the same.

I reckon the only real consumers that expects anything other than traceEvent and
metadata is likely trace viewer, as we haven't been returning this via the
protocol and the background tracing is perhaps not expecting this, so it's not
too late to change -- but this is the way it is now.

[nednguyen, 2016-07-22 14:34:52]

On 2016/07/20 18:15:35, caseq wrote:
> On 2016/07/20 17:50:21, charliea wrote:
> > Gah, sorry: I thought I replied to this code review yesterday, but I must
> never
> > have hit submit.
> > 
> > The trace format is when a trace comes in as a javascript object that can
> > contain up to two fields:
> > 
> > {
> >   traceEvents: [  /* An array of trace events */ ],
> >   metadata: { /* A dictionary from metadata field to value */ }
> > }
> > 
> > Multiple subtraces can make up a single trace by being zipped (now) or TARed
> > (to-be-implemented
> 
> This is not how tracing controller currently packages traces. This CL has
added
> support for multiple trace "agents" in tracing controller:
> 
>
https://chromium.googlesource.com/chromium/src/+/b0b2a7f0fd7f6ec4a43f735e643a...
> 
> and as I you can see, these come at the same level as traceEvents and
metadata.
> Moreover, we used to have more than two values there even before this CL.
FWIW,
> I've recently refactored a lot of this code, but the format remains the same.
> 
> I reckon the only real consumers that expects anything other than traceEvent
and
> metadata is likely trace viewer, as we haven't been returning this via the
> protocol and the background tracing is perhaps not expecting this, so it's not
> too late to change -- but this is the way it is now.

Yeah, I think this is a known diversion between how chrome vs telemetry package
trace. According to the doc, I believe the right trace container format is just
an archive file, with each trace being a file. It probably took a while for
Chrome to update to follow the format, but for telemetry, we can make sure that
each trace is in its own part.

The code that packages telemetry traces is in
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

Can we for now just make sure trace_data basically a dictionary:
{
 CHROME_TRACE_PART: {all the data telemetry got from chrome, including chrome
trace's metadata}
 BATTOR_TRACE_PART: ...
}

For the case of Chrome trace contains other subtrace like battor or atrace, we
don't do that for telemetry, hence you can just throw exception or ignore it.

[caseq, 2016-07-22 16:48:18]

On 2016/07/22 14:34:52, nednguyen wrote:

> Yeah, I think this is a known diversion between how chrome vs telemetry
package
> trace. According to the doc, I believe the right trace container format is
just
> an archive file, with each trace being a file. It probably took a while for
> Chrome to update to follow the format, but for telemetry, we can make sure
that
> each trace is in its own part.
> 
> The code that packages telemetry traces is in
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> Can we for now just make sure trace_data basically a dictionary:
> {
>  CHROME_TRACE_PART: {all the data telemetry got from chrome, including chrome
> trace's metadata}
>  BATTOR_TRACE_PART: ...
> }
> 
> For the case of Chrome trace contains other subtrace like battor or atrace, we
> don't do that for telemetry, hence you can just throw exception or ignore it.

So when we write a trace in telemetry, who would the consumers be?

[nednguyen, 2016-07-22 16:52:04]

On 2016/07/22 16:48:18, caseq wrote:
> On 2016/07/22 14:34:52, nednguyen wrote:
> 
> > Yeah, I think this is a known diversion between how chrome vs telemetry
> package
> > trace. According to the doc, I believe the right trace container format is
> just
> > an archive file, with each trace being a file. It probably took a while for
> > Chrome to update to follow the format, but for telemetry, we can make sure
> that
> > each trace is in its own part.
> > 
> > The code that packages telemetry traces is in
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > 
> > Can we for now just make sure trace_data basically a dictionary:
> > {
> >  CHROME_TRACE_PART: {all the data telemetry got from chrome, including
chrome
> > trace's metadata}
> >  BATTOR_TRACE_PART: ...
> > }
> > 
> > For the case of Chrome trace contains other subtrace like battor or atrace,
we
> > don't do that for telemetry, hence you can just throw exception or ignore
it.
> 
> So when we write a trace in telemetry, who would the consumers be?

trace_viewer should be the only consumer. There is also the legacy trace parser
written in python in
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
but you can mostly ignore it.

[caseq, 2016-07-22 17:59:32]

On 2016/07/22 16:52:04, nednguyen wrote:

> > So when we write a trace in telemetry, who would the consumers be?
> 
> trace_viewer should be the only consumer. There is also the legacy trace
parser
> written in python in
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> but you can mostly ignore it.

So if we change the format written by telemetry, we'll break the trace viewer,
right?

[nednguyen, 2016-07-22 18:16:07]

On 2016/07/22 17:59:32, caseq wrote:
> On 2016/07/22 16:52:04, nednguyen wrote:
> 
> > > So when we write a trace in telemetry, who would the consumers be?
> > 
> > trace_viewer should be the only consumer. There is also the legacy trace
> parser
> > written in python in
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > but you can mostly ignore it.
> 
> So if we change the format written by telemetry, we'll break the trace viewer,
> right?

I believe trace viewer is already able to handle this case. To make sure, you
can try running ./telemetry/bin/run_tests --browser=<...>
timeline_based_page_test which run the whole thing end-to-end

[caseq, 2016-07-26 22:43:28]

ptal -- I've made CHROME_TRACE_PART to be an object that contains traceEvents
and metadata fields. This unfortunately required a somewhat hacky support on
part of TraceDataBuilder due to the fact that we re-use the builder to
implicitly concatenate traces from multiple recordings when flushing the trace.

[charliea (OOO until 10-5), 2016-07-27 19:52:23]

lgtm, but definitely wait for Ned and Zhen to take a deeper look (they have a
lot more knowledge about Telemetry code than I do)

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:168: #
After Tracing.end, chrome browser will send asynchronous notifications
nit: is this line too long?

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:224:
trace_data_builder: An instance of TraceDataBuilder to put results to.
s/results to/results into

[caseq, 2016-07-28 01:09:00]

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:168: #
After Tracing.end, chrome browser will send asynchronous notifications
On 2016/07/27 19:52:22, charliea wrote:
> nit: is this line too long?

Why, 76 chars. But thanks for pointint out, I actually didn't mean to add this
comment, my guess is that it accidentally crept in during merge. Dropped it
altogether.

https://codereview.chromium.org/2160793003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:224:
trace_data_builder: An instance of TraceDataBuilder to put results to.
On 2016/07/27 19:52:22, charliea wrote:
> s/results to/results into

Done.

[caseq, 2016-07-28 01:09:16]

Patchset #4 (id:60001) has been deleted

[nednguyen, 2016-07-28 16:39:23]

I defer reviewing this to Zhen

[Zhen Wang, 2016-07-28 17:17:11]

https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:100:
self._had_data_collected = False
Can you move _had_data_collected to be after _has_received_all_tracing_data.

By the way, those two are a little confusing. Also see comments below in
_NotificationHandler.

https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:269:
'Got both dataCollected events and a stream from server')
So is Tracing.tracingComplete for streaming version and Tracing.dataCollected
for non-streaming version?

[caseq, 2016-07-28 22:59:37]

On 2016/07/28 17:17:11, Zhen Wang wrote:
>
https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
> File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
> (right):
> 
>
https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
> telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:100:
> self._had_data_collected = False
> Can you move _had_data_collected to be after _has_received_all_tracing_data.

Done.

> By the way, those two are a little confusing. Also see comments below in
> _NotificationHandler.

Actually, I can drop _had_data_collected altogether if you like, it's there just
to preserve a consistency check that was otherwise done with the help of
_trace_events field that I dropped. It's not that we really depend on this
check.

>
https://codereview.chromium.org/2160793003/diff/80001/telemetry/telemetry/int...
> telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:269:
> 'Got both dataCollected events and a stream from server')
> So is Tracing.tracingComplete for streaming version and Tracing.dataCollected
> for non-streaming version?

Tracing.tracingComplete is there either way, but in case of legacy (aka
non-streaming) mode it's meaning is that all Tracing.dataCollected events have
been emitted and the client has the entire trace at this point. In case of
streaming mode, it means "tracing is over and the data are available in the
stream with given id".

[Zhen Wang, 2016-07-28 23:23:46]

> Tracing.tracingComplete is there either way, but in case of legacy (aka
> non-streaming) mode it's meaning is that all Tracing.dataCollected events have
> been emitted and the client has the entire trace at this point. In case of
> streaming mode, it means "tracing is over and the data are available in the
> stream with given id".


Since when the streaming mode is supported? Can we remove the old way
altogether? (That can be in a different patch though)

[caseq, 2016-07-28 23:38:06]

On 2016/07/28 23:23:46, Zhen Wang wrote:
> > Tracing.tracingComplete is there either way, but in case of legacy (aka
> > non-streaming) mode it's meaning is that all Tracing.dataCollected events
have
> > been emitted and the client has the entire trace at this point. In case of
> > streaming mode, it means "tracing is over and the data are available in the
> > stream with given id".
> 
> 
> Since when the streaming mode is supported? Can we remove the old way
> altogether? (That can be in a different patch though)

Since m48. I thought we can remove it, but turns out not, as we were returning
traces in the old format for tracing initiated via startup tracing before the
following two CLs landed yesterday & today:

https://codereview.chromium.org/2180423004/
https://codereview.chromium.org/2189823002/

[Zhen Wang, 2016-07-29 14:45:43]

> Since m48. I thought we can remove it, but turns out not, as we were returning
> traces in the old format for tracing initiated via startup tracing before the
> following two CLs landed yesterday & today:

Acknowledged.

https://codereview.chromium.org/2160793003/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:267:
if self._had_data_collected:
So this will never happen? If so, I would suggest to remove _had_data_collected
to reduce the complexity here.

[caseq, 2016-07-29 19:23:36]

On 2016/07/29 14:45:43, Zhen Wang wrote:
> > Since m48. I thought we can remove it, but turns out not, as we were
returning
> > traces in the old format for tracing initiated via startup tracing before
the
> > following two CLs landed yesterday & today:
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2160793003/diff/100001/telemetry/telemetry/in...
> File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
> (right):
> 
>
https://codereview.chromium.org/2160793003/diff/100001/telemetry/telemetry/in...
> telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:267:
> if self._had_data_collected:
> So this will never happen? If so, I would suggest to remove
_had_data_collected
> to reduce the complexity here.

Done.

[caseq, 2016-07-29 19:46:47]

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

[commit-bot: I haz the power, 2016-07-29 19:46:55]

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-29 20:50:03]

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

[commit-bot: I haz the power, 2016-07-29 20:50:04]

Dry run: This issue passed the CQ dry run.

[Zhen Wang, 2016-07-29 21:41:50]

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:89:
def __init__(self, inspector_socket, is_tracing_running=False,
When TracingBackend is created, the is_tracing_running parameter tells if it is
startup tracing. So I don't think you need self._start_issued.

Also see:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

[caseq, 2016-07-29 22:07:39]

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:89:
def __init__(self, inspector_socket, is_tracing_running=False,
On 2016/07/29 21:41:50, Zhen Wang wrote:
> When TracingBackend is created, the is_tracing_running parameter tells if it
is
> startup tracing. So I don't think you need self._start_issued.
> 
> Also see:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

Why are we discussing it here? This is not something that this patch changes.
Also, what is your proposal?

[Zhen Wang, 2016-07-29 22:27:07]

lgtm

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/2160793003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend.py:89:
def __init__(self, inspector_socket, is_tracing_running=False,
On 2016/07/29 22:07:39, caseq wrote:
> On 2016/07/29 21:41:50, Zhen Wang wrote:
> > When TracingBackend is created, the is_tracing_running parameter tells if it
> is
> > startup tracing. So I don't think you need self._start_issued.
> > 
> > Also see:
> >
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> Why are we discussing it here? This is not something that this patch changes.
> Also, what is your proposal?

Sorry, I thought it was introduced by this CL. I guess it came from rebasing
from ToT.

[caseq, 2016-07-29 22:30:29]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-07-29 22:30:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2160793003/#ps120001
(title: "dropped _had_data_collected")

[commit-bot: I haz the power, 2016-07-29 22:30:36]

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-29 22:32:00]

Description was changed from

==========
Support traces containing all parts/agents returned via DevTools streams

This prepares telemetry for a change in the format of data returned via
DevTools streams. The new format is consistent with trace files and
includes traces from all sources formatted as a JSON object.

BUG=chromium:599932
==========

to

==========
Support traces containing all parts/agents returned via DevTools streams

This prepares telemetry for a change in the format of data returned via
DevTools streams. The new format is consistent with trace files and
includes traces from all sources formatted as a JSON object.

BUG=chromium:599932

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-07-29 22:32:02]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...