Set tracing categories explicitly in Clovis and Sandwich

Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
Committed: https://crrev.com/ca2594ea175abc0daf257cff17b46b9418dad29a
Cr-Commit-Position: refs/heads/master@{#398262}

[pasko, 2016-06-06 16:02:44]

Description was changed from

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
==========

to

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
==========

[pasko, 2016-06-06 16:02:45]

pasko@chromium.org changed reviewers:
+ blundell@chromium.org, gabadie@chromium.org, mattcary@chromium.org

[blundell, 2016-06-06 16:06:38]

lgtm

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
File tools/android/loading/queuing_lens.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
tools/android/loading/queuing_lens.py:20: QUEUING_CATEGORY =
clovis_constants.QUEUING_CATEGORY
nit: maybe this should still be defined in tracing.py since the name of the
category isn't itself specific to Clovis?

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
File tools/android/loading/tracing_unittest.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing_unittest.py:11: import clovis_constants
nit: this seems like a minor layering violation. any reason this test shouldn't
just define its own arbitrary set of initial categories?

[gabadie, 2016-06-06 16:17:30]

overall, looking good. Only tracing_unittest.py case is concerning me.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
File tools/android/loading/queuing_lens.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
tools/android/loading/queuing_lens.py:20: QUEUING_CATEGORY =
clovis_constants.QUEUING_CATEGORY
On 2016/06/06 16:06:38, blundell wrote:
> nit: maybe this should still be defined in tracing.py since the name of the
> category isn't itself specific to Clovis?

Curious: why would only this category be defined in tracing? Does it have any
particularity compared to the other ones?

Might not be consistent unless we defines all the other as well...

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:47: ]
We might probably be able to get rid of devtools.timeline, v8, toplevel and
java. Or have I missed something?

Can be deferred to a separate CL.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
File tools/android/loading/tracing_unittest.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing_unittest.py:47:
clovis_constants.DEFAULT_CATEGORIES + ['A', 'B', 'C', 'D'])
Why is it needed to add clovis_constants.DEFAULT_CATEGORIES? Looks like it can
be avoided. Unless there is an assertion here that I believe could also be
triggered in sandwich?

[pasko, 2016-06-06 17:30:52]

Addressed all comments, PTAL

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
File tools/android/loading/queuing_lens.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
tools/android/loading/queuing_lens.py:20: QUEUING_CATEGORY =
clovis_constants.QUEUING_CATEGORY
On 2016/06/06 16:17:30, gabadie wrote:
> On 2016/06/06 16:06:38, blundell wrote:
> > nit: maybe this should still be defined in tracing.py since the name of the
> > category isn't itself specific to Clovis?
> 
> Curious: why would only this category be defined in tracing? Does it have any
> particularity compared to the other ones?
> 
> Might not be consistent unless we defines all the other as well...

I am concerned with clovis_constants.py and tracing.py depending on each other.
Circular dependencies are harder to understand and manage.

We can make tracing_constants.py and have there:
QUEUING_CATEGORY
CLOVIS_DEFAULT_CATEGORIES

other suggestions are welcome, beyond circular dependencies I do not have clear
naming preferences yet.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:47: ]
On 2016/06/06 16:17:30, gabadie wrote:
> We might probably be able to get rid of devtools.timeline, v8, toplevel and
> java. Or have I missed something?
> 
> Can be deferred to a separate CL.

If we remove toplevel, we would not be able to reason about what happened by
looking at the traces in the traceviewer.

Same for v8: it tells us when the blink thread was busy executing javascript,
which is an important bit of data.

as for 'java' - I did not see any events in our traces in this category, which
means:
1. there is probably a bug somewhere
2. we can easily remove it

I just removed it in the next patchset.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
File tools/android/loading/tracing_unittest.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing_unittest.py:11: import clovis_constants
On 2016/06/06 16:06:38, blundell wrote:
> nit: this seems like a minor layering violation.

It does not look like a layering violation to me. If tracing.py depends on
clovis_constants.py, it should be fine to depend on clovis_constants in
tracing_unittest.

> any reason this test shouldn't just define its own arbitrary set of initial
categories?

Agreed, the initial categories are not needed here. I should have given it more
thought instead of mechanically mirroring what was done before.

Removed.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing_unittest.py:47:
clovis_constants.DEFAULT_CATEGORIES + ['A', 'B', 'C', 'D'])
On 2016/06/06 16:17:30, gabadie wrote:
> Why is it needed to add clovis_constants.DEFAULT_CATEGORIES? Looks like it can
> be avoided. Unless there is an assertion here that I believe could also be
> triggered in sandwich?

You are right, this is not needed. Also blundell@ raised the same concern, so I
removed the dependency on clovis_constants entirely from this file. Does not
affect how things are tested here.

[gabadie, 2016-06-06 17:37:10]

lgtm

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
File tools/android/loading/queuing_lens.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/queui...
tools/android/loading/queuing_lens.py:20: QUEUING_CATEGORY =
clovis_constants.QUEUING_CATEGORY
On 2016/06/06 17:30:52, pasko wrote:
> On 2016/06/06 16:17:30, gabadie wrote:
> > On 2016/06/06 16:06:38, blundell wrote:
> > > nit: maybe this should still be defined in tracing.py since the name of
the
> > > category isn't itself specific to Clovis?
> > 
> > Curious: why would only this category be defined in tracing? Does it have
any
> > particularity compared to the other ones?
> > 
> > Might not be consistent unless we defines all the other as well...
> 
> I am concerned with clovis_constants.py and tracing.py depending on each
other.
> Circular dependencies are harder to understand and manage.
> 
> We can make tracing_constants.py and have there:
> QUEUING_CATEGORY
> CLOVIS_DEFAULT_CATEGORIES
> 
> other suggestions are welcome, beyond circular dependencies I do not have
clear
> naming preferences yet.

To me it just looks fine as is.

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2047463002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:47: ]
On 2016/06/06 17:30:52, pasko wrote:
> On 2016/06/06 16:17:30, gabadie wrote:
> > We might probably be able to get rid of devtools.timeline, v8, toplevel and
> > java. Or have I missed something?
> > 
> > Can be deferred to a separate CL.
> 
> If we remove toplevel, we would not be able to reason about what happened by
> looking at the traces in the traceviewer.
> 
> Same for v8: it tells us when the blink thread was busy executing javascript,
> which is an important bit of data.
> 
> as for 'java' - I did not see any events in our traces in this category, which
> means:
> 1. there is probably a bug somewhere
> 2. we can easily remove it
> 
> I just removed it in the next patchset.

Nice.

[mattcary, 2016-06-07 08:42:31]

lgtm

[pasko, 2016-06-07 09:17:38]

The CQ bit was checked by pasko@chromium.org

[pasko, 2016-06-07 09:17:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2047463002/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-06-07 09:17:49]

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

[commit-bot: I haz the power, 2016-06-07 09:58:16]

Description was changed from

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
==========

to

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
==========

[commit-bot: I haz the power, 2016-06-07 09:58:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-07 10:00:22]

Description was changed from

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080
==========

to

==========
Set tracing categories explicitly in Clovis and Sandwich

Also restrict the categories in Sandwich to reduce the size of traces by 2x.

BUG=582080

Committed: https://crrev.com/ca2594ea175abc0daf257cff17b46b9418dad29a
Cr-Commit-Position: refs/heads/master@{#398262}
==========

[commit-bot: I haz the power, 2016-06-07 10:00:23]

Patchset 2 (id:??) landed as
https://crrev.com/ca2594ea175abc0daf257cff17b46b9418dad29a
Cr-Commit-Position: refs/heads/master@{#398262}