Plumb IterationInfo from telemetry to a diagnostic

Plumb IterationInfo from telemetry to a diagnostic

For now, IterationInfo contains only those fields that are absolutely necessary
for results2.html: storyName, label, timestamp, storySetRepeatCounter,
storyRepeatCounter.

Values can become separated from their ValueSets, and ValueSets can contain
Values from different iterations, and the current IterationInfo is only a
hundred bytes or so, so for now, this diagnostic will be duplicated on every
Value.

This diagnostic will eventually gain more fields and also be displayed on the
dashboard and unified with the existing value metadata system.
For now, only the storyName is replicated between IterationInfo and the existing
value metadata.

If and when this diagnostic grows too large to be replicated on every Value, then
we can choose how to combat the bloat using strategies that are being worked out
in the design doc:

https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrpH8/edit#

BUG=catapult:#2405

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a5348f556ee6ed0e9dbde67440465a21490bb676

[benjhayden, 2016-06-09 22:41:57]

benjhayden@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com

[benjhayden, 2016-06-09 23:01:26]

Description was changed from

==========
Plumb IterationInfo from telemetry to a diagnostic

BUG=catapult:#2405
==========

to

==========
Plumb IterationInfo from telemetry to a diagnostic

See also Chrome-side: https://codereview.chromium.org/2055063002

BUG=catapult:#2405
==========

[benjhayden, 2016-06-09 23:07:32]

PTAL :-)

[nednguyen, 2016-06-10 00:53:51]

https://codereview.chromium.org/2055043002/diff/40001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2055043002/diff/40001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:238: opts.SetIterationInfo('benchmarkName',
self.Name())
Why is this logic done in CreatePageTest? Can't you just move it to
GetExtraIterationInfo?

https://codereview.chromium.org/2055043002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2055043002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:218:
test.WillRunIteration(pageset_repeat_counter,
Where is this test.WillRunIteration come from? I would prefer not adding another
method to test, especially no when you need to special case
TimelineBasedMeasurement for this. Can you make make this work by passing the
counter to results.WillRunPage(story, ..) below?

[benjhayden, 2016-06-16 20:17:11]

https://codereview.chromium.org/2055043002/diff/60001/telemetry/telemetry/and...
File telemetry/telemetry/android/shared_android_state.py (right):

https://codereview.chromium.org/2055043002/diff/60001/telemetry/telemetry/and...
telemetry/telemetry/android/shared_android_state.py:53:
pageset_repeat_counter=0,  # pylint: disable=unused-argument
page -> story

[benjhayden, 2016-06-16 21:46:36]

PTAL :-)

[benjhayden, 2016-06-16 21:51:22]

Description was changed from

==========
Plumb IterationInfo from telemetry to a diagnostic

See also Chrome-side: https://codereview.chromium.org/2055063002

BUG=catapult:#2405
==========

to

==========
Plumb IterationInfo from telemetry to a diagnostic

This will be used by both results2.html and the dashboard.
Values can become separated from their ValueSets, and ValueSets can contain
Values
from different iterations, so this diagnostic will be duplicated on every Value.

Some of this data is currently stored in the dashboard's db alongside the Values
by recipes, but that system is incompatible with results2.html, which needs some
of this information regardless of what recipes do.

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

See also Chrome-side: https://codereview.chromium.org/2055063002

BUG=catapult:#2405
==========

[benjhayden, 2016-06-17 18:02:06]

Description was changed from

==========
Plumb IterationInfo from telemetry to a diagnostic

This will be used by both results2.html and the dashboard.
Values can become separated from their ValueSets, and ValueSets can contain
Values
from different iterations, so this diagnostic will be duplicated on every Value.

Some of this data is currently stored in the dashboard's db alongside the Values
by recipes, but that system is incompatible with results2.html, which needs some
of this information regardless of what recipes do.

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

See also Chrome-side: https://codereview.chromium.org/2055063002

BUG=catapult:#2405
==========

to

==========
Plumb IterationInfo from telemetry to a diagnostic

For now, IterationInfo contains only those fields that are absolutely necessary
for results2.html: storyName, label, timestamp, storySetRepeatCounter,
storyRepeatCounter.

Values can become separated from their ValueSets, and ValueSets can contain
Values from different iterations, and the current IterationInfo is only a
hundred bytes or so, so for now, this diagnostic will be duplicated on every
Value.

This diagnostic will eventually gain more fields and also be displayed on the
dashboard and unified with the existing value metadata system.
For now, only the storyName is replicated between IterationInfo and the existing
value metadata.

If and when this diagnostic grows too large to be replicated on every Value,
then
we can choose how to combat the bloat using strategies that are being worked out
in the design doc:

https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

[benjhayden, 2016-06-17 22:23:23]

Ok, this is turning out to be a lot more complicated than we thought. :-)
It seems like most of the complexity is coming from the dashboard use case.
Can we descope and say that this CL is only for results2.html for now?

[nednguyen, 2016-06-17 22:26:53]

Mostly lg2me but the IterationInfo should not be part of
TimelinebasedMeasurement.Options

https://codereview.chromium.org/2055043002/diff/100001/telemetry/telemetry/we...
File telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/2055043002/diff/100001/telemetry/telemetry/we...
telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def
SetIterationInfo(self, name, value):
No. These should be parts of the page_test_results instead, not
TimelineBasedMeasurement.Options

[eakuefner, 2016-06-17 22:28:15]

lgtm from my perspective. we've talked about how to extend this to the dashboard
use case, and all of the ideas are backwards-compatible with what's being done
here, so i'd be happy if this lands so that we can unblock the results2.html
MVP.

[eakuefner, 2016-06-20 18:57:11]

few more comments, still lgtm

we discussed offline how we will implement a displayLabel feature, that, much
like the display name for Story, uses the label if set, and a combination of the
bm name and timestamp otherwise.

Ned will have the SH benchmark script set a custom label so that the results of
each individual benchmark can appear in the same column.

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:215: for story in group.stories:
Also put storyGroupingKeys as story.grouping_keys if it's non-empty here.

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:216:
results.SetIterationInfo('storyUrl', story.url)
You can replace both of these with storyDisplayName.

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:316:
results.SetIterationInfo('label', finder_options.results_label)
don't serialize label if it's not set.

[eakuefner, 2016-06-20 19:09:03]

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/page_test_results.py (right):

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:72:
self._iteration_info = {'timestamp': time.time()}
Comment that we use time() because we'll pass it to the JS Date constructor for
display.

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2055043002/diff/160001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:316:
results.SetIterationInfo('label', finder_options.results_label)
On 2016/06/20 at 18:57:11, eakuefner wrote:
> don't serialize label if it's not set.

Also, put benchmarkName. And move ts here because this is where all the globals
should go.

Finally, put all of these globals in CreateResults.

[nednguyen, 2016-06-20 19:10:12]

lgtm

[benjhayden, 2016-06-20 21:03:08]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-06-20 21:03:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2055043002/#ps180001
(title: "storyGroupingKeys, storyDisplayName, benchmarkName,
s/timestamp/benchmarkStartMs/g")

[benjhayden, 2016-06-20 21:03:17]

Thanks!

[commit-bot: I haz the power, 2016-06-20 21:03:18]

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

[nednguyen, 2016-06-20 21:09:00]

https://codereview.chromium.org/2055043002/diff/180001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/page_test_results.py (right):

https://codereview.chromium.org/2055043002/diff/180001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:73: def
SetIterationInfo(self, name, value):
as a follow up, can we add assert to blow thing up when benchmarkStartMs,
benchmarkName, label is set twice?

[commit-bot: I haz the power, 2016-06-20 21:14:48]

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

[commit-bot: I haz the power, 2016-06-20 21:14:49]

Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[benjhayden, 2016-06-20 23:26:56]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-06-20 23:26:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2055043002/#ps200001
(title: "IterationInfo python class")

[commit-bot: I haz the power, 2016-06-20 23:26:59]

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

[commit-bot: I haz the power, 2016-06-21 00:49:40]

Description was changed from

==========
Plumb IterationInfo from telemetry to a diagnostic

For now, IterationInfo contains only those fields that are absolutely necessary
for results2.html: storyName, label, timestamp, storySetRepeatCounter,
storyRepeatCounter.

Values can become separated from their ValueSets, and ValueSets can contain
Values from different iterations, and the current IterationInfo is only a
hundred bytes or so, so for now, this diagnostic will be duplicated on every
Value.

This diagnostic will eventually gain more fields and also be displayed on the
dashboard and unified with the existing value metadata system.
For now, only the storyName is replicated between IterationInfo and the existing
value metadata.

If and when this diagnostic grows too large to be replicated on every Value,
then
we can choose how to combat the bloat using strategies that are being worked out
in the design doc:

https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

to

==========
Plumb IterationInfo from telemetry to a diagnostic

For now, IterationInfo contains only those fields that are absolutely necessary
for results2.html: storyName, label, timestamp, storySetRepeatCounter,
storyRepeatCounter.

Values can become separated from their ValueSets, and ValueSets can contain
Values from different iterations, and the current IterationInfo is only a
hundred bytes or so, so for now, this diagnostic will be duplicated on every
Value.

This diagnostic will eventually gain more fields and also be displayed on the
dashboard and unified with the existing value metadata system.
For now, only the storyName is replicated between IterationInfo and the existing
value metadata.

If and when this diagnostic grows too large to be replicated on every Value,
then
we can choose how to combat the bloat using strategies that are being worked out
in the design doc:

https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405

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

[commit-bot: I haz the power, 2016-06-21 00:49:41]

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