[Telemetry] Fix bug in markers for cache temperature

[Telemetry] Fix bug in markers for cache temperature

A previous CL (https://codereview.chromium.org/2693923005) introduced
a bug in the markers injected in the trace to control the cache
"temperature" on pages.

This caused lots of spurious regressions on all of the "warm"
measurements for page_cycler_v2.

BUG=chromium:692929
Review-Url: https://codereview.chromium.org/2704493002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/19a742985d7b0ca4192614fc7942e57dbb257abc

[perezju, 2017-02-16 13:57:50]

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

[nednguyen, 2017-02-16 13:59:10]

lgtm

[nednguyen, 2017-02-16 13:59:56]

On 2017/02/16 13:59:10, nednguyen wrote:
> lgtm

Can we add test for this?

[perezju, 2017-02-16 14:11:28]

On 2017/02/16 13:59:56, nednguyen wrote:
> On 2017/02/16 13:59:10, nednguyen wrote:
> > lgtm
> 
> Can we add test for this?

I thought about that, the tests in:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

do not seem to do much more than "run the code" a hope it does't fail/crash (no
asserts!).

Is there a easy way to inspect the trace there and check that both "start" and
"end" markers were included?

[nednguyen, 2017-02-16 14:15:53]

On 2017/02/16 14:11:28, perezju wrote:
> On 2017/02/16 13:59:56, nednguyen wrote:
> > On 2017/02/16 13:59:10, nednguyen wrote:
> > > lgtm
> > 
> > Can we add test for this?
> 
> I thought about that, the tests in:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> do not seem to do much more than "run the code" a hope it does't fail/crash
(no
> asserts!).
> 
> Is there a easy way to inspect the trace there and check that both "start" and
> "end" markers were included?

I think the easiest way to do this is just to get the trace as a simple dict
(not the model since we don't want to rely on it) in python & assert you can
find 'start' & 'end' marker events

[perezju, 2017-02-16 14:19:52]

On 2017/02/16 14:15:53, nednguyen wrote:
> On 2017/02/16 14:11:28, perezju wrote:
> > On 2017/02/16 13:59:56, nednguyen wrote:
> > > On 2017/02/16 13:59:10, nednguyen wrote:
> > > > lgtm
> > > 
> > > Can we add test for this?
> > 
> > I thought about that, the tests in:
> >
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> > 
> > do not seem to do much more than "run the code" a hope it does't fail/crash
> (no
> > asserts!).
> > 
> > Is there a easy way to inspect the trace there and check that both "start"
and
> > "end" markers were included?
> 
> I think the easiest way to do this is just to get the trace as a simple dict
> (not the model since we don't want to rely on it) in python & assert you can
> find 'start' & 'end' marker events

Is there an example where we do something like that? (the reading the trace as
a dict thing)

[nednguyen, 2017-02-16 14:28:31]

On 2017/02/16 14:19:52, perezju wrote:
> On 2017/02/16 14:15:53, nednguyen wrote:
> > On 2017/02/16 14:11:28, perezju wrote:
> > > On 2017/02/16 13:59:56, nednguyen wrote:
> > > > On 2017/02/16 13:59:10, nednguyen wrote:
> > > > > lgtm
> > > > 
> > > > Can we add test for this?
> > > 
> > > I thought about that, the tests in:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> > > 
> > > do not seem to do much more than "run the code" a hope it does't
fail/crash
> > (no
> > > asserts!).
> > > 
> > > Is there a easy way to inspect the trace there and check that both "start"
> and
> > > "end" markers were included?
> > 
> > I think the easiest way to do this is just to get the trace as a simple dict
> > (not the model since we don't want to rely on it) in python & assert you can
> > find 'start' & 'end' marker events
> 
> Is there an example where we do something like that? (the reading the trace as
> a dict thing)

https://cs.chromium.org/chromium/src/tools/perf/measurements/clock_domain_tes...

[perezju, 2017-02-16 15:12:41]

PTAL.

Confirmed that tests didn't pass, but they do now.

[nednguyen, 2017-02-16 15:16:47]

On 2017/02/16 15:12:41, perezju wrote:
> PTAL.
> 
> Confirmed that tests didn't pass, but they do now.

lgtm

Tests look great!

[perezju, 2017-02-16 15:18:43]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2017-02-16 15:18:55]

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, 2017-02-16 15:41:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487258323924910,
"parent_rev": "88e9135e3e0a5cfe60aa54b9248aac917ca865f0", "commit_rev":
"19a742985d7b0ca4192614fc7942e57dbb257abc"}

[commit-bot: I haz the power, 2017-02-16 15:42:01]

Description was changed from

==========
[Telemetry] Fix bug in markers for cache temperature

A previous CL (https://codereview.chromium.org/2693923005) introduced
a bug in the markers injected in the trace to control the cache
"temperature" on pages.

This caused lots of spurious regressions on all of the "warm"
measurements for page_cycler_v2.

BUG=chromium:692929
==========

to

==========
[Telemetry] Fix bug in markers for cache temperature

A previous CL (https://codereview.chromium.org/2693923005) introduced
a bug in the markers injected in the trace to control the cache
"temperature" on pages.

This caused lots of spurious regressions on all of the "warm"
measurements for page_cycler_v2.

BUG=chromium:692929

Review-Url: https://codereview.chromium.org/2704493002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-02-16 15:42:02]

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