[Telemetry] Ensure that story display names are unique

[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/42560ff37c155a0b8c319f79511154f58c465b34
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a3d2a9721a0bc501733c7e03722e902a7d7c0077
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1b820a28c383840a84933543dd4630daa0f25fc0
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/471f516504c46017af98ab87f1a02bd589f0e5b2

[eakuefner, 2016-07-14 18:52:00]

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

[eakuefner, 2016-07-14 18:52:01]

Please, take a look.

[nednguyen, 2016-07-14 19:01:18]

lgtm
Thanks!

[eakuefner, 2016-07-14 23:25:13]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2016-07-14 23:25:18]

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-14 23:40:12]

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

[commit-bot: I haz the power, 2016-07-14 23:40:13]

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

[nednguyen, 2016-07-15 16:34:12]

lgtm but as we talked offline, what's unique here is story name + grouping key

[eakuefner, 2016-07-15 19:34:58]

On 2016/07/15 at 16:34:12, nednguyen wrote:
> lgtm but as we talked offline, what's unique here is story name + grouping key

Done, and added tests for both new cases: same story name + different grouping
keys is ok; same story name + same grouping keys raises.

[eakuefner, 2016-07-15 19:35:06]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-07-15 19:35:06]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2153513002/#ps40001
(title: "Address Ned's comment")

[commit-bot: I haz the power, 2016-07-15 19:35:16]

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

[eakuefner, 2016-07-15 19:35:22]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2016-07-15 19:35:43]

Actually, I will land this on Monday because I would like to watch the Catapult
roll.

[nednguyen, 2016-07-15 19:38:38]

On 2016/07/15 19:35:43, eakuefner wrote:
> Actually, I will land this on Monday because I would like to watch the
Catapult
> roll.

+1

[eakuefner, 2016-07-18 17:27:44]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2016-07-18 17:27:48]

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-18 17:52:12]

Description was changed from

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429
==========

to

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

[commit-bot: I haz the power, 2016-07-18 17:52:13]

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

[commit-bot: I haz the power, 2016-07-18 17:52:15]

CQ bit was unchecked.

[eakuefner, 2016-07-19 06:28:05]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2163583002/ by eakuefner@chromium.org.

The reason for reverting is: Broke telemetry_perf_unittests:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....

[eakuefner, 2016-08-05 08:53:15]

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

[commit-bot: I haz the power, 2016-08-05 08:53:29]

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-08-05 09:19:02]

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

[commit-bot: I haz the power, 2016-08-05 09:19:03]

Dry run: This issue passed the CQ dry run.

[eakuefner, 2016-10-28 19:00:33]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-10-28 19:00:33]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2153513002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-28 19:00:35]

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-10-28 19:30:27]

Description was changed from

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

to

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

[commit-bot: I haz the power, 2016-10-28 19:30:27]

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

[eakuefner, 2016-10-29 04:25:06]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2457303003/ by eakuefner@chromium.org.

The reason for reverting is: Causing telemetry_perf_unittests to time out on
Windows???.

[eakuefner, 2016-12-01 00:13:27]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2016-12-01 00:13:39]

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-12-01 01:02:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480551207625910,
"parent_rev": "582ccd4ab71bd01044002b4798dc7c5fc5ad7fde", "commit_rev":
"1b820a28c383840a84933543dd4630daa0f25fc0"}

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

Description was changed from

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

to

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

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

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

[eakuefner, 2016-12-01 04:00:24]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2545573003/ by eakuefner@chromium.org.

The reason for reverting is: Still breaks Chromium CQ..

[nednguyen, 2016-12-01 13:08:33]

On 2016/12/01 04:00:24, eakuefner wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2545573003/ by mailto:eakuefner@chromium.org.
> 
> The reason for reverting is: Still breaks Chromium CQ..

Have you tried to run this telemetry_perf_unittest locally on your Windows to
see why it stuck? This seems to be failing consistently, so shouldn't be that
hard to reproduce.

[eakuefner, 2016-12-01 15:57:53]

On 2016/12/01 at 13:08:33, nednguyen wrote:
> On 2016/12/01 04:00:24, eakuefner wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/2545573003/ by
mailto:eakuefner@chromium.org.
> > 
> > The reason for reverting is: Still breaks Chromium CQ..
> 
> Have you tried to run this telemetry_perf_unittest locally on your Windows to
see why it stuck? This seems to be failing consistently, so shouldn't be that
hard to reproduce.

I have; I can seemingly repro but not reliably, and the problem is that when you
ctrl+c under typ, you don't get a stack trace for any of the child processes. I
also get a lot of 10 second lock timeouts when I try to run
telemetry_perf_unittests on my Windows laptop.

I might see if I can modify the typ runner to propagate the signal when I ctrl+c
to its child processes and thereby get stack traces for them.

[nednguyen, 2016-12-01 16:00:52]

On 2016/12/01 15:57:53, eakuefner wrote:
> On 2016/12/01 at 13:08:33, nednguyen wrote:
> > On 2016/12/01 04:00:24, eakuefner wrote:
> > > A revert of this CL (patchset #4 id:60001) has been created in
> > > https://codereview.chromium.org/2545573003/ by
> mailto:eakuefner@chromium.org.
> > > 
> > > The reason for reverting is: Still breaks Chromium CQ..
> > 
> > Have you tried to run this telemetry_perf_unittest locally on your Windows
to
> see why it stuck? This seems to be failing consistently, so shouldn't be that
> hard to reproduce.
> 
> I have; I can seemingly repro but not reliably, and the problem is that when
you
> ctrl+c under typ, you don't get a stack trace for any of the child processes.
I
> also get a lot of 10 second lock timeouts when I try to run
> telemetry_perf_unittests on my Windows laptop.
> 
> I might see if I can modify the typ runner to propagate the signal when I
ctrl+c
> to its child processes and thereby get stack traces for them.

I am suspecting test in scripts_smoke_unittest causing the hang. Can you try
running ./tools/perf/run_testst .. scripts_smoke_unittest a few time to see if
it hangs?

It probably worths adding logging to see why it hangs & run the test with
--passthrough flag. Dirk@ is the person to help with debugging typ

[eakuefner, 2016-12-01 17:18:42]

On 2016/12/01 at 16:00:52, nednguyen wrote:
> On 2016/12/01 15:57:53, eakuefner wrote:
> > On 2016/12/01 at 13:08:33, nednguyen wrote:
> > > On 2016/12/01 04:00:24, eakuefner wrote:
> > > > A revert of this CL (patchset #4 id:60001) has been created in
> > > > https://codereview.chromium.org/2545573003/ by
> > mailto:eakuefner@chromium.org.
> > > > 
> > > > The reason for reverting is: Still breaks Chromium CQ..
> > > 
> > > Have you tried to run this telemetry_perf_unittest locally on your Windows
to
> > see why it stuck? This seems to be failing consistently, so shouldn't be
that
> > hard to reproduce.
> > 
> > I have; I can seemingly repro but not reliably, and the problem is that when
you
> > ctrl+c under typ, you don't get a stack trace for any of the child
processes. I
> > also get a lot of 10 second lock timeouts when I try to run
> > telemetry_perf_unittests on my Windows laptop.
> > 
> > I might see if I can modify the typ runner to propagate the signal when I
ctrl+c
> > to its child processes and thereby get stack traces for them.
> 
> I am suspecting test in scripts_smoke_unittest causing the hang. Can you try
running ./tools/perf/run_testst .. scripts_smoke_unittest a few time to see if
it hangs?
> 
> It probably worths adding logging to see why it hangs & run the test with
--passthrough flag. Dirk@ is the person to help with debugging typ

Running scripts_smoke_unittest by itself doesn't seem to cause any hang.

[nednguyen, 2016-12-02 12:15:14]

On 2016/12/01 17:18:42, eakuefner wrote:
> On 2016/12/01 at 16:00:52, nednguyen wrote:
> > On 2016/12/01 15:57:53, eakuefner wrote:
> > > On 2016/12/01 at 13:08:33, nednguyen wrote:
> > > > On 2016/12/01 04:00:24, eakuefner wrote:
> > > > > A revert of this CL (patchset #4 id:60001) has been created in
> > > > > https://codereview.chromium.org/2545573003/ by
> > > mailto:eakuefner@chromium.org.
> > > > > 
> > > > > The reason for reverting is: Still breaks Chromium CQ..
> > > > 
> > > > Have you tried to run this telemetry_perf_unittest locally on your
Windows
> to
> > > see why it stuck? This seems to be failing consistently, so shouldn't be
> that
> > > hard to reproduce.
> > > 
> > > I have; I can seemingly repro but not reliably, and the problem is that
when
> you
> > > ctrl+c under typ, you don't get a stack trace for any of the child
> processes. I
> > > also get a lot of 10 second lock timeouts when I try to run
> > > telemetry_perf_unittests on my Windows laptop.
> > > 
> > > I might see if I can modify the typ runner to propagate the signal when I
> ctrl+c
> > > to its child processes and thereby get stack traces for them.
> > 
> > I am suspecting test in scripts_smoke_unittest causing the hang. Can you try
> running ./tools/perf/run_testst .. scripts_smoke_unittest a few time to see if
> it hangs?
> > 
> > It probably worths adding logging to see why it hangs & run the test with
> --passthrough flag. Dirk@ is the person to help with debugging typ
> 
> Running scripts_smoke_unittest by itself doesn't seem to cause any hang.

Hmhh, I would recommend you look at the failed log in catapult to figure out
which tests were not executed when they hang.  That would give some clues about
what hang.

[eakuefner, 2016-12-02 19:48:37]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-12-02 19:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2153513002/#ps80001
(title: "Use set-based check")

[commit-bot: I haz the power, 2016-12-02 19:48:45]

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

[nednguyen, 2016-12-02 19:59:04]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2016-12-02 19:59:58]

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/story_set.py:35: self.story_names_and_grouping_keys =
set()
Can you make this private? Things that are not supposed to be touched/modified
by external should be private.

[nednguyen, 2016-12-02 20:00:57]

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/story_set.py:118: self.stories.remove(story)
Don't you need to remove the corresponding data from
display_name_and_grouping_key_tuple? Please also add a unittest that would catch
this bug

[eakuefner, 2016-12-02 20:28:58]

Thanks for your comments. Please take another look.

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/story_set.py:35: self.story_names_and_grouping_keys =
set()
On 2016/12/02 at 19:59:58, nednguyen wrote:
> Can you make this private? Things that are not supposed to be touched/modified
by external should be private.

Done. Also made stories private, since now it's critical that stories and
story_names_and_grouping_keys should remain in sync.

https://codereview.chromium.org/2153513002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/story_set.py:118: self.stories.remove(story)
On 2016/12/02 at 20:00:57, nednguyen wrote:
> Don't you need to remove the corresponding data from
display_name_and_grouping_key_tuple? Please also add a unittest that would catch
this bug

Done.

[eakuefner, 2016-12-02 20:57:29]

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

[commit-bot: I haz the power, 2016-12-02 20:57:37]

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

[nednguyen, 2016-12-02 20:59:38]

lgtm

[commit-bot: I haz the power, 2016-12-02 21:09:28]

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

[commit-bot: I haz the power, 2016-12-02 21:09:30]

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

[eakuefner, 2016-12-02 21:13:18]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-12-02 21:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2153513002/#ps120001
(title: "fix typo")

[commit-bot: I haz the power, 2016-12-02 21:13:30]

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-12-02 22:14:24]

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

[commit-bot: I haz the power, 2016-12-02 22:14:26]

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

[eakuefner, 2016-12-02 22:24:17]

Kari, it looks like there are some issues on Linux to do with "can't find
browser" -- could this have to do with the recent stable roll?

[nednguyen, 2016-12-02 22:33:22]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2016-12-02 22:33:29]

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

[aiolos (Not reviewing), 2016-12-02 22:50:40]

On 2016/12/02 22:24:17, eakuefner wrote:
> Kari, it looks like there are some issues on Linux to do with "can't find
> browser" -- could this have to do with the recent stable roll?

Hmm, possibly. I would assume it would fail to land in the first case if it
caused those errors though.

[aiolos (Not reviewing), 2016-12-02 23:00:47]

On 2016/12/02 22:50:40, aiolos wrote:
> On 2016/12/02 22:24:17, eakuefner wrote:
> > Kari, it looks like there are some issues on Linux to do with "can't find
> > browser" -- could this have to do with the recent stable roll?
> 
> Hmm, possibly. I would assume it would fail to land in the first case if it
> caused those errors though.

I did a speculative revert of https://codereview.chromium.org/2415683002/ to see
if that fixes the issue. 

I didn't see the "can't find browser" log that you mentioned.

Messages with the following are expected since we're running outside of chrome
checkout:
"Chrome build location for linux_x86_64 not found."

[commit-bot: I haz the power, 2016-12-02 23:25:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480718002498830,
"parent_rev": "2c4509f202dee320e26bdef10d7a414eb5f30a72", "commit_rev":
"471f516504c46017af98ab87f1a02bd589f0e5b2"}

[commit-bot: I haz the power, 2016-12-02 23:25:33]

Description was changed from

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

to

==========
[Telemetry] Ensure that story display names are unique

Telemetry operates on a de-facto assumption that story display names are
unique; this CL makes it de-jure.

BUG=catapult:#2429

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

[commit-bot: I haz the power, 2016-12-02 23:25:34]

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