Ownership into GenericSets

Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetBugComponents' and `GetOwners' in Benchmark for the
  information coming from the decorators (instead of GetOwnership).

* Adds unindexed `name' field to the Sparse Diagnostic model

* Adds method to query Sparse Diagnostics by name, and moves the 
  method that queries by type to the Sparse Diagnostic model 
  (instead of in find_anomalies).

* Changes diagnostic instantiation to use GenericSet in the
  story runner to use two separate GenericSet diagnostics instead of just 
  the one for ownership.

* Change sample diagnostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics (bug #3718).

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507

Review-Url: https://codereview.chromium.org/2977283002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e94ec7bdc9789b3b7afec8d0ef6dd724c1a6d56f

[phsilva, 2017-07-17 21:01:48]

Description was changed from

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of \"owners\") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component (\"bug components\").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

Despite these changes, there still is a need for the Sparse
Diagnostics to be created by querying the name field.
Currently, it references the type field, which is how it
could previously create Ownership diagnostics. This work
will also be necessary for the transitions of other
sparse diagnostics.

In regards to the length of the change list of this CL, other
than independent changes such as documentation, it was necessary
to submit all of the above changes together so that none of these
parts be broken.

BUG=catapult:
==========

to

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of \"owners\") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component (\"bug components\").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

Despite these changes, there still is a need for the Sparse
Diagnostics to be created by querying the name field.
Currently, it references the type field, which is how it
could previously create Ownership diagnostics. This work
will also be necessary for the transitions of other
sparse diagnostics.

BUG=catapult:#3715,catapult:#3507
==========

[phsilva, 2017-07-17 21:04:46]

Description was changed from

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of \"owners\") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component (\"bug components\").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

Despite these changes, there still is a need for the Sparse
Diagnostics to be created by querying the name field.
Currently, it references the type field, which is how it
could previously create Ownership diagnostics. This work
will also be necessary for the transitions of other
sparse diagnostics.

BUG=catapult:#3715,catapult:#3507
==========

to

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of \"owners\") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component (\"bug components\").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics.

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

[phsilva, 2017-07-17 21:05:52]

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

[phsilva, 2017-07-17 21:08:12]

Hey,

This is the CL addressing moving Ownership to GenericSet, along with a couple of
other things necessary to make the move. I acknowledge this is a CL that touches
a lot of parts, so let me know if you think any of the things this CL is doing
should be broken out into a different CL.

Let me know what you think.

Thanks!

https://codereview.chromium.org/2977283002/diff/20001/docs/histogram-set-json...
File docs/histogram-set-json-format.md (left):

https://codereview.chromium.org/2977283002/diff/20001/docs/histogram-set-json...
docs/histogram-set-json-format.md:207: ### OwnershipInfo
Ben, do we want to replace this with anything else in the docs?

[phsilva, 2017-07-17 21:08:24]

Description was changed from

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of \"owners\") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component (\"bug components\").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics.

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

to

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics.

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

[benjhayden, 2017-07-17 22:08:06]

https://codereview.chromium.org/2977283002/diff/20001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms.py (right):

https://codereview.chromium.org/2977283002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms.py:23: SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES =
set(
Did you want to go ahead and use this in this CL to plumb the new diagnostics
into SparseDiagnostic? Or is there a bug for Ethan to do that?

https://codereview.chromium.org/2977283002/diff/20001/docs/histogram-set-json...
File docs/histogram-set-json-format.md (left):

https://codereview.chromium.org/2977283002/diff/20001/docs/histogram-set-json...
docs/histogram-set-json-format.md:207: ### OwnershipInfo
On 2017/07/17 at 21:08:12, phsilva wrote:
> Ben, do we want to replace this with anything else in the docs?

Not in this CL. I'll add a section about reserved names. Thanks for reminding
me!

https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark_unittest.py (right):

https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark_unittest.py:198:
self.assertEqual(fooComponentDiagnostic.AsDict()['values'], [])
You can get at the values more directly by casting the GenericSet to a list:
self.assertEqual(list(fooComponentDiagnostic), [])

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

https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:409:
results.histograms.AddSharedDiagnostic(
Why not elide these diagnostics if they are empty?

https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
File tracing/tracing/value/diagnostics/reserved_names.html (left):

https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
tracing/tracing/value/diagnostics/reserved_names.html:36: MEMORY_AMOUNTS: {name:
'memory amounts', type: tr.v.d.GenericSet},
Why did this change?

https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
tracing/tracing/value/diagnostics/reserved_names.html:39: OS_NAMES: {name: 'os
names', type: tr.v.d.GenericSet},
Why did this change?

https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
tracing/tracing/value/diagnostics/reserved_names.html:48: TRACE_START: {name:
'trace start', type: tr.v.d.DateRange},
Why did this change?

[phsilva, 2017-07-17 22:55:56]

On 2017/07/17 at 22:08:06, benjhayden wrote:
>
https://codereview.chromium.org/2977283002/diff/20001/dashboard/dashboard/add...
> File dashboard/dashboard/add_histograms.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/20001/dashboard/dashboard/add...
> dashboard/dashboard/add_histograms.py:23: SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES
= set(
> Did you want to go ahead and use this in this CL to plumb the new diagnostics
into SparseDiagnostic? Or is there a bug for Ethan to do that?

This (plumbing) isn't meant to be in this CL. I'll report a new bug and add a
TODO to it.

>
https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/ben...
> File telemetry/telemetry/benchmark_unittest.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/ben...
> telemetry/telemetry/benchmark_unittest.py:198:
self.assertEqual(fooComponentDiagnostic.AsDict()['values'], [])
> You can get at the values more directly by casting the GenericSet to a list:
> self.assertEqual(list(fooComponentDiagnostic), [])
> 

Thanks! Done now.

>
https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/int...
> File telemetry/telemetry/internal/story_runner.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/20001/telemetry/telemetry/int...
> telemetry/telemetry/internal/story_runner.py:409:
results.histograms.AddSharedDiagnostic(
> Why not elide these diagnostics if they are empty?
> 

I'll add a check and not add them if not necessary now.

>
https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
> File tracing/tracing/value/diagnostics/reserved_names.html (left):
> 
>
https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
> tracing/tracing/value/diagnostics/reserved_names.html:36: MEMORY_AMOUNTS:
{name: 'memory amounts', type: tr.v.d.GenericSet},
> Why did this change?
> 
>
https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
> tracing/tracing/value/diagnostics/reserved_names.html:39: OS_NAMES: {name: 'os
names', type: tr.v.d.GenericSet},
> Why did this change?
> 
>
https://codereview.chromium.org/2977283002/diff/20001/tracing/tracing/value/d...
> tracing/tracing/value/diagnostics/reserved_names.html:48: TRACE_START: {name:
'trace start', type: tr.v.d.DateRange},
> Why did this change?

Sorry, these must have slipped through when I was re-basing to master from a
patch of the previous CL. I'll revert these.
Thanks for pointing it out.

[shatch, 2017-07-18 16:24:15]

https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:392: def
GetMostRecentDiagnosticValueByName(test_key, diagnostic_name):
Maybe make this return a dict of {name: diagnostic} instead of querying multiple
times.

[benjhayden, 2017-07-18 16:43:07]

https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:408: diagnostics_named = [d for d in
diagnostics if d.name == diagnostic_name]
Can you merge this for loop with the next one?

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

https://codereview.chromium.org/2977283002/diff/60001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:224: def GetComponent(self):
Can we refer to this as BugComponents everywhere for consistency?
We can bikeshed on the name if you want, but it should be consistent throughout
catapult.

[phsilva, 2017-07-18 23:58:14]

On 2017/07/18 at 16:24:15, simonhatch wrote:
>
https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:392: def
GetMostRecentDiagnosticValueByName(test_key, diagnostic_name):
> Maybe make this return a dict of {name: diagnostic} instead of querying
multiple times.

I thought of doing this originally but it felt a bit like premature optimization
since it's only used twice here. I've changed it to use only 1 query, let me
know what you think now.

[phsilva, 2017-07-19 00:00:02]

On 2017/07/18 at 16:43:07, benjhayden wrote:
>
https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/60001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:408: diagnostics_named = [d for d in
diagnostics if d.name == diagnostic_name]
> Can you merge this for loop with the next one?
> 

Done now, thanks for pointing it out!

>
https://codereview.chromium.org/2977283002/diff/60001/telemetry/telemetry/ben...
> File telemetry/telemetry/benchmark.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/60001/telemetry/telemetry/ben...
> telemetry/telemetry/benchmark.py:224: def GetComponent(self):
> Can we refer to this as BugComponents everywhere for consistency?
> We can bikeshed on the name if you want, but it should be consistent
throughout catapult.

I think the name makes sense. The original 'GetComponent' I got from the
decorator which use a method by the same name. I think it makes sense for us to
just call it "Component" while referring to the benchmark ownership decorator
and then "bug components" when talking about the GenericSet diagnostic. I've
updated the CL to reflect the new name.

[shatch, 2017-07-19 14:52:51]

lg2m % q's

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms.py (right):

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms.py:24: SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES =
set(
Where is this used in this cl?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:415: diagnostic_map.get(diagnostic.name)
is None):
I'm not sure I understand this extra check (and
diagnostic_map.get(diagnostic.name) is None), could you explain why it's needed,
or why you'd have 2 diagnostics back from the query with the same name?

[phsilva, 2017-07-19 17:05:45]

On 2017/07/19 at 14:52:51, simonhatch wrote:
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/add...
> File dashboard/dashboard/add_histograms.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/add...
> dashboard/dashboard/add_histograms.py:24: SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES
= set(
> Where is this used in this cl?
> 

This is not used in this CL, I put it there so it can be used for when the
Sparse Diagnostic picked up diagnostics by name. I figured I'd add it now so
this is not forgotten to be done when that change happens. But I see how it's
out of the scope of this CL so I'll remove it.

>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:415: diagnostic_map.get(diagnostic.name)
is None):
> I'm not sure I understand this extra check (and
diagnostic_map.get(diagnostic.name) is None), could you explain why it's needed,
or why you'd have 2 diagnostics back from the query with the same name?

I added this check in order to cover all cases, even if right now it shouldn't
happen

[benjhayden, 2017-07-19 19:55:01]

Did you mean to re-upload after removing SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:394: def
GetMostRecentDiagnosticValueByNames(test_key, diagnostic_names):
Would it make sense to make this a classmethod on SparseDiagnostic so that other
endpoints can call it without importing find_anomalies?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:415: diagnostic_map.get(diagnostic.name)
is None):
On 2017/07/19 at 14:52:51, shatch wrote:
> I'm not sure I understand this extra check (and
diagnostic_map.get(diagnostic.name) is None), could you explain why it's needed,
or why you'd have 2 diagnostics back from the query with the same name?

Should this be an assert?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies_test.py (right):

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies_test.py:515: end_revision=sys.maxint,
id='sampleOwners', name='owners')
reserved_infos.OWNERS.name

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies_test.py:520: end_revision=sys.maxint,
id='sampleComponent', name='bug components')
reserved_infos.BUG_COMPONENTS.name

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies_test.py:539: owners_name =
reserved_infos.OWNERS.name
Why alias these instead of referring to them inline?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies_test.py:557: end_revision=sys.maxint,
id='sampleOwners', name=owners_name)
Can you set the ids to the data samples' guids to make these tests more
realistic?

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies_test.py:606: end_revision=sys.maxint,
id='foobar')
Can you set name to reserved_infos.DEVICE.name to be more realistic?

[shatch, 2017-07-19 21:48:59]

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:394: def
GetMostRecentDiagnosticValueByNames(test_key, diagnostic_names):
On 2017/07/19 19:55:00, benjhayden wrote:
> Would it make sense to make this a classmethod on SparseDiagnostic so that
other
> endpoints can call it without importing find_anomalies?

+1

https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
dashboard/dashboard/find_anomalies.py:415: diagnostic_map.get(diagnostic.name)
is None):
On 2017/07/19 19:55:01, benjhayden wrote:
> On 2017/07/19 at 14:52:51, shatch wrote:
> > I'm not sure I understand this extra check (and
> diagnostic_map.get(diagnostic.name) is None), could you explain why it's
needed,
> or why you'd have 2 diagnostics back from the query with the same name?
> 
> Should this be an assert?

Yeah, I'd rather this shows up in error monitoring since it's not supposed to
happen.

[phsilva, 2017-07-20 22:15:12]

Thanks for the comments Ben.

On 2017/07/19 at 19:55:01, benjhayden wrote:
> Did you mean to re-upload after removing SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES?
> 

I did. Should I have kept it instead?

>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:394: def
GetMostRecentDiagnosticValueByNames(test_key, diagnostic_names):
> Would it make sense to make this a classmethod on SparseDiagnostic so that
other endpoints can call it without importing find_anomalies?
> 

I've moved it there, along with "GetMostRecentDiagnosticData" (which does the
same except selecting by type).

>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies_test.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies_test.py:539: owners_name =
reserved_infos.OWNERS.name
> Why alias these instead of referring to them inline?
> 

Orignaly, to avoid repetition. I've removed them now.

>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies_test.py:606: end_revision=sys.maxint,
id='foobar')
> Can you set name to reserved_infos.DEVICE.name to be more realistic?

I left the name field intentionally blank here since we expect there to be
sparse diagnostics without the name field defined. This test was meant to make
sure the function still worked.

[phsilva, 2017-07-20 22:16:14]

On 2017/07/19 at 21:48:59, simonhatch wrote:
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> File dashboard/dashboard/find_anomalies.py (right):
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:394: def
GetMostRecentDiagnosticValueByNames(test_key, diagnostic_names):
> +1
> 
>
https://codereview.chromium.org/2977283002/diff/80001/dashboard/dashboard/fin...
> dashboard/dashboard/find_anomalies.py:415: diagnostic_map.get(diagnostic.name)
is None):
>
> Yeah, I'd rather this shows up in error monitoring since it's not supposed to
happen.

Thanks, Simon. I've moved the method and added the assert, along with a test for
it in the latest patch

[phsilva, 2017-07-21 20:17:25]

Description was changed from

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetComponent' and `GetOwners' in Benchmark for the
  information coming from the decorators.

* Adds unindexed `name' field to the Sparse Diagnostic model

* Changes query to be done by the name field (not type) when
  making the Anomaly entities.

* Changes diagnostic instantiation to use GenericSet in the
  story runner.

* Change sample daignostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics.

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

to

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetBugComponents' and `GetOwners' in Benchmark for the
  information coming from the decorators (instead of GetOwnership).

* Adds unindexed `name' field to the Sparse Diagnostic model

* Adds method to query Sparse Diagnostics by name, and moves the 
  method that queries by type to the Sparse Diagnostic model 
  (instead of in find_anomalies).

* Changes diagnostic instantiation to use GenericSet in the
  story runner to use two separate GenericSet diagnostics instead of just 
  the one for ownership.

* Change sample diagnostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics (bug #3718).

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

[shatch, 2017-07-24 16:35:38]

lgtm % some comments

https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
File dashboard/dashboard/models/histogram.py (right):

https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram.py:38: def
GetMostRecentDiagnosticValueByNames(test_key, diagnostic_names):
Since this is part of SparseDiagnostic, you can shorten the name.

https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram.py:65: def
GetMostRecentDiagnosticData(test_key, diagnostic_type):
Does this get called anywhere? As far as I can tell, the
GetMostRecentDiagnosticValueByNames replaced the need for
GetMostRecentDiagnosticData in the original version, so this seems like it could
just be removed. Also, since most types will just be GenericSet, this function
doesn't seem like it'd return the right thing (the first one from the query, not
ordered in any way?). I'd just ditch this.

https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
File dashboard/dashboard/models/histogram_test.py (right):

https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram_test.py:169: def
testGetMostRecentDiagnosticData_WithoutComponent(self):
I'm not clear the purpose of this test, since GetMostRecentDiagnosticData is
only ever supposed to return a single diagnostic.

[benjhayden, 2017-07-24 17:22:57]

lgtm

[phsilva, 2017-07-24 17:30:50]

Thanks for the comments, Simon. I've addressed them in the new patch.

On 2017/07/24 at 16:35:38, simonhatch wrote:
>
https://codereview.chromium.org/2977283002/diff/120001/dashboard/dashboard/mo...
> dashboard/dashboard/models/histogram.py:65: def
GetMostRecentDiagnosticData(test_key, diagnostic_type):
> Does this get called anywhere? As far as I can tell, the
GetMostRecentDiagnosticValueByNames replaced the need for
GetMostRecentDiagnosticData in the original version, so this seems like it could
just be removed. Also, since most types will just be GenericSet, this function
doesn't seem like it'd return the right thing (the first one from the query, not
ordered in any way?). I'd just ditch this.
> 

I had left this in case anyone was developing using this method. I originally
made it abstract (non-type specific) because that possibility was pointed out to
me. I don't see it being used right now, so I've removed it and its
corresponding tests.

[shatch, 2017-07-24 17:33:49]

lgtm

[eakuefner, 2017-07-24 17:59:57]

lgtm % a bunch of nits.

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/fi...
File dashboard/dashboard/find_anomalies.py (right):

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/fi...
dashboard/dashboard/find_anomalies.py:327: queried_diagnostics = \
nit: please use parentheses instead of the backslash line continuation.

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/mo...
File dashboard/dashboard/models/histogram.py (right):

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram.py:48: corresponding diagnostic's values.
nit: diagnostics'

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/mo...
File dashboard/dashboard/models/histogram_test.py (right):

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram_test.py:13: 
nit: additional blank line here

https://codereview.chromium.org/2977283002/diff/140001/dashboard/dashboard/mo...
dashboard/dashboard/models/histogram_test.py:131: if __name__ == '__main__':
nit: no need for this main bit.

https://codereview.chromium.org/2977283002/diff/140001/telemetry/telemetry/be...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2977283002/diff/140001/telemetry/telemetry/be...
telemetry/telemetry/benchmark.py:225: """Returns a Generic Diagnostic containing
the benchmark's ownership
nit: GenericSet, also s/ownership/Monorail/

https://codereview.chromium.org/2977283002/diff/140001/telemetry/telemetry/be...
telemetry/telemetry/benchmark.py:229: GenericSet Diagnostic with the benchmark's
owners' component name
s/owners'/bug/

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

https://codereview.chromium.org/2977283002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:412: if list(benchmark_owners):
Can you file a bug and put a TODO here about adding an IsEmpty method to
GenericSet so that we don't have to write this?

[phsilva, 2017-07-24 18:41:45]

Thanks for the comments, Ethan. I've addressed all of them in the most recent
patch.

[phsilva, 2017-07-24 18:43:22]

The CQ bit was checked by phsilva@google.com

[phsilva, 2017-07-24 18:43:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, eakuefner@chromium.org, simonhatch@chromium.org
Link to the patchset: https://codereview.chromium.org/2977283002/#ps160001
(title: "Address Ethan's nits")

[commit-bot: I haz the power, 2017-07-24 18:43:31]

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-07-24 20:44:01]

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

[commit-bot: I haz the power, 2017-07-24 20:44:02]

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[phsilva, 2017-07-25 22:48:07]

The CQ bit was checked by phsilva@google.com

[commit-bot: I haz the power, 2017-07-25 22:48: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, 2017-07-25 22:50:49]

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

[commit-bot: I haz the power, 2017-07-25 22:50:50]

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...)
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[phsilva, 2017-07-25 22:59:15]

The CQ bit was checked by phsilva@google.com

[phsilva, 2017-07-25 22:59:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, benjhayden@chromium.org, simonhatch@chromium.org
Link to the patchset: https://codereview.chromium.org/2977283002/#ps180001
(title: "Fix tests after reabse")

[commit-bot: I haz the power, 2017-07-25 22:59: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, 2017-07-26 00:00:02]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1501023555578890,
"parent_rev": "088ee235440b16490c10ad38d22b0486445dd783", "commit_rev":
"e94ec7bdc9789b3b7afec8d0ef6dd724c1a6d56f"}

[commit-bot: I haz the power, 2017-07-26 00:00:08]

Description was changed from

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetBugComponents' and `GetOwners' in Benchmark for the
  information coming from the decorators (instead of GetOwnership).

* Adds unindexed `name' field to the Sparse Diagnostic model

* Adds method to query Sparse Diagnostics by name, and moves the 
  method that queries by type to the Sparse Diagnostic model 
  (instead of in find_anomalies).

* Changes diagnostic instantiation to use GenericSet in the
  story runner to use two separate GenericSet diagnostics instead of just 
  the one for ownership.

* Change sample diagnostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics (bug #3718).

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507
==========

to

==========
Ownership into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the Ownership diagnostic class into two Generic Sets:
one where the values are an array of the owners' emails (which goes
with the name of "owners") and one where the values is an array,
either empty or with 1 string, decribing the benchmark's ownership
component ("bug components").

Among other things, this CL:
* Removes the Ownership Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of Ownership.

* Removes Ownership from all_diagnostics.

* Uses `GetBugComponents' and `GetOwners' in Benchmark for the
  information coming from the decorators (instead of GetOwnership).

* Adds unindexed `name' field to the Sparse Diagnostic model

* Adds method to query Sparse Diagnostics by name, and moves the 
  method that queries by type to the Sparse Diagnostic model 
  (instead of in find_anomalies).

* Changes diagnostic instantiation to use GenericSet in the
  story runner to use two separate GenericSet diagnostics instead of just 
  the one for ownership.

* Change sample diagnostic in add_histogram_queue test to use
  Telemetry instead of Ownership.

* Updates documentation.

What has not been addressed:

* Despite these changes, there still is a need for the Sparse
  Diagnostics to be created by querying the name field.
  Currently, it references the type field, which is how it
  could previously create Ownership diagnostics. This work
  will also be necessary for the transitions of other
  sparse diagnostics (bug #3718).

* Include in the documentation that owners and bug components
  are GenericSet diagnostics that will be automatically
  attached from the benchmarks in Telemetry.

BUG=catapult:#3715,catapult:#3507

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

[commit-bot: I haz the power, 2017-07-26 00:00:10]

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