Define DiagnosticMap and a basic Diagnostic hierarchy

Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(Generic, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAhok/edit

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180

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

[benjhayden, 2016-05-24 23:41:27]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics, with nice
APIs like DiagnosticMap and RelatedValueSet.

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics, with nice
APIs like DiagnosticMap and RelatedValueSet.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

[benjhayden, 2016-05-24 23:59:07]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics, with nice
APIs like DiagnosticMap and RelatedValueSet.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

[benjhayden, 2016-05-25 00:00:10]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

[benjhayden, 2016-05-25 00:02:05]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved by ValueSet after all Values are
imported.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

BUG=catapult:#2180
==========

[eakuefner, 2016-05-25 21:30:28]

eakuefner@chromium.org changed reviewers:
+ eakuefner@chromium.org

[eakuefner, 2016-05-25 21:30:29]

canonicalUrl is a dumb; let's get rid of it.

We will probably get rid of options (which was basically only used for
description) by solving the RandomCrapDiagnostic problem in another CL.

We will probably get rid of groupingKeys by making it strongly typed and
pertaining directly to the bots (IterationInfo extends Diagnostic?)

https://codereview.chromium.org/2000063006/diff/20001/tracing/tracing/value/d...
File tracing/tracing/value/diagnostics/composition.html (right):

https://codereview.chromium.org/2000063006/diff/20001/tracing/tracing/value/d...
tracing/tracing/value/diagnostics/composition.html:24: type:
this.constructor.name,
Move type into Diagnostic.asDict and then put _asDictInto.

https://codereview.chromium.org/2000063006/diff/20001/tracing/tracing/value/d...
File tracing/tracing/value/diagnostics/string.html (right):

https://codereview.chromium.org/2000063006/diff/20001/tracing/tracing/value/d...
tracing/tracing/value/diagnostics/string.html:15: function String(v) {
This seems like the RandomCrapDiagnostic that we had talked about -- should be
anything that can be JSON.stringified and then we can just make the viz a GOV.

[benjhayden, 2016-05-26 00:42:13]

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

[benjhayden, 2016-05-26 00:42:13]

Thanks! PTAL.

[benjhayden, 2016-05-26 00:42:18]

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

[commit-bot: I haz the power, 2016-05-26 00:42:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000063006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000063006/80001

[benjhayden, 2016-05-26 00:42:47]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

The only diagnostic that was actually being consistently recorded before this
patch is 'canonical_url' because it was required by the Value API.
The new DiagnosticMap API allows this common diagnostic to be handled by the
ValueSet transparently to metrics, so metrics can focus on naming and computing
Values.
In a followup CL, telemetry will pass other common diagnostics through
metricMapFunction such as botName and storyName.

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

BUG=catapult:#2180
==========

[commit-bot: I haz the power, 2016-05-26 01:05:22]

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

[commit-bot: I haz the power, 2016-05-26 01:05:23]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 01:05:31]

Dry run: None

[nednguyen, 2016-05-27 00:28:52]

lgtm

Looks like we can also remove the important field since we have the graphical
model

[nednguyen, 2016-05-27 00:29:12]

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

[eakuefner, 2016-05-27 00:43:21]

lgtm; +1 to killing important. You can just set important=False in the Python if
need be.

[petrcermak, 2016-05-27 08:58:46]

I'm happy to see this change remove unused parts of the codebase. However,
please document it in the patch description. I couldn't find any mention of
"removing grouping keys" (which seems to be a crucial part of the patch) in
there. Also, you seem to remove some value reported by the hazard metric, which
I think should again be mentioned in the patch description.

Apart from the above, LGTM with some inline comments.

Thanks,
Petr

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (left):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:146:
values.addValue(new tr.v.NumericValue(
Did you really want to remove this? How is that related to "Defining
DiagnosticMap and a basic Diagnostic hierarchy"?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/value_set.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/value_set.html:39: addValueDicts: function(dicts) {
Could you maybe add a comment explaining what this method is for? Perhaps it
should be called addValuesFromDicts?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/value_set.html:45:
this.values_[guid].diagnostics.forEach(function(name, diagnostic) {
If I understand correctly, this will re-resolve the diagnostics of all existing
values (that were added previously). Firstly, is this safe (is resolution
idempotent)? Secondly, it's inefficient (O(n^2) in the worst case in the number
of values).

Why not resolve the values right after line 42:

  ...
  this.addValue(value);
  value.diagnostics.forEach(function(name, diagnostic) {
    if (diagnostic instanceof tr.v.d.RelatedValueSet)
      diagnostic.resolve(this);
  }, this);

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/value_set_test.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/value_set_test.html:74: assert.isTrue(rvs2 instanceof
tr.v.d.RelatedValueSet);
assert.instanceOf(rvs2, tr.v.d.RelatedValueSet)

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/value_set_test.html:75: assert.equal(1,
rvs2.values.length);
strictEqual

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/ui/ana...
File tracing/tracing/ui/analysis/single_user_expectation_sub_view.html (left):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/ui/ana...
tracing/tracing/ui/analysis/single_user_expectation_sub_view.html:70: var values
= new tr.metrics.ValueSet();
How is this related to this patch?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/composition.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/composition.html:8: <link rel="import"
href="/tracing/base/base.html">
no need for importing base when you import something else

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/composition.html:15: function
Composition(opt_values) {
nit: add /** @constructor */ above

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/diagnostic.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic.html:14: function Diagnostic() {
nit: add /** @constructor */ above

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic.html:38: throw new
Error('Diagnostics must define fromDict(d)');
nit: please use braces in if statements with multi-line conditions.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic.html:40: // When subclasses set
their prototype to an entirely new object and omit
BTW (unrelated to this patch): This is something that I think we should address
at the project level. I think we should provide a tr.b.extend API so that
everyone extends classes correctly.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/diagnostic_map.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:8: <link rel="import"
href="/tracing/base/base.html">
ditto: no need

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:17: function
DiagnosticMap() {
nit: add /** @constructor */ above

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:40: for (var name in
this.diagnosticsByName_)
not sure if important, but we usually do `opt_this = opt_this || this;` at the
beginning of methods/functions like this (see iteration_helpers.html).

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:46: for (var name in
this.diagnosticsByName_)
I suggest you use iteration_helpers.html for this:

  asDict: function() {
    return tr.b.mapItems(
        this.diagnosticsByName_,
        (name, diagnostic) => diagnostic.asDict());
  }

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:55: diagnostics.add(name,
tr.v.d.Diagnostic.fromDict(d));
Don't you mean tr.v.d.Diagnostic.fromDict(d[name])? If so, this is indicating a
missing test. You can use iteration_helpers.html here as well:

tr.b.iterItems(d, (name, dict) => diagnostics.add(name,
tr.v.d.Diagnostic.fromDict(dict)));

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/generic.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/generic.html:20: * @param {Object} value
This should be {*} or even {!*} if you don't allow `null`. See
https://developers.google.com/closure/compiler/docs/js-for-compiler#types.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/related_value_set.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/related_value_set.html:15: function
ValueRef(guid) {
nit: add /** @constructor */ above

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/related_value_set.html:19: function
RelatedValueSet(opt_values) {
nit: add /** @constructor */ above

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/related_value_set.html:50: * @param
{tr.metrics.ValueSet} valueSet
nit: Unless you allow null, this should be {!tr.metrics.ValueSet}
(https://developers.google.com/closure/compiler/docs/js-for-compiler#types).

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/related_value_set.html:56: if (value
instanceof tr.v.Value) {
nit: no need for braces.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/value.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/value.html:40: case 'numeric': value =
NumericValue.fromDict(d); break;
I'm personally not a big fan of putting multiple statements on a single line,
but feel free to ignore.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/value.html:45: if (value === undefined)
If fromDict is guaranteed to return an object in each case, then I suggest you
put this as a "default:" case instead (in which case you won't need to
initialize |value| to `undefined` on line 38).

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/value.html:49: for (var name in d.diagnostics) {
Alternatively, you could use tr.b.iterItems

[benjhayden, 2016-05-27 16:53:38]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The only diagnostic that had been created was canonical_url, which is unused, so
this CL removes it, which changes the Value API.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

BUG=catapult:#2180
==========

[benjhayden, 2016-05-27 16:56:50]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The only diagnostic that had been created was canonical_url, which is unused, so
this CL removes it, which changes the Value API.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The only diagnostic that had been created was canonical_url, which is unused, so
this CL removes it, which changes the Value API.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

[benjhayden, 2016-05-27 16:57:30]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The only diagnostic that had been created was canonical_url, which is unused, so
this CL removes it, which changes the Value API.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

[benjhayden, 2016-05-27 16:57:54]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already needed to significantly change the Value
API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

[benjhayden, 2016-05-27 18:59:03]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(String, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(Generic, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

[benjhayden, 2016-05-27 18:59:16]

Thanks! PTAL?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (left):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:146:
values.addValue(new tr.v.NumericValue(
On 2016/05/27 at 08:58:44, petrcermak wrote:
> Did you really want to remove this? How is that related to "Defining
DiagnosticMap and a basic Diagnostic hierarchy"?

Yes.
This CL could refactor hazardMetric's diagnostics to use the new
RelatedValueSet, but this metric is being refactored anyway:
https://codereview.chromium.org/1992303003

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/value_set.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/value_set.html:45:
this.values_[guid].diagnostics.forEach(function(name, diagnostic) {
On 2016/05/27 at 08:58:44, petrcermak wrote:
> If I understand correctly, this will re-resolve the diagnostics of all
existing values (that were added previously). Firstly, is this safe (is
resolution idempotent)? Secondly, it's inefficient (O(n^2) in the worst case in
the number of values).
> 
> Why not resolve the values right after line 42:
> 
>   ...
>   this.addValue(value);
>   value.diagnostics.forEach(function(name, diagnostic) {
>     if (diagnostic instanceof tr.v.d.RelatedValueSet)
>       diagnostic.resolve(this);
>   }, this);

Commented. PTAL?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/ui/ana...
File tracing/tracing/ui/analysis/single_user_expectation_sub_view.html (left):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/ui/ana...
tracing/tracing/ui/analysis/single_user_expectation_sub_view.html:70: var values
= new tr.metrics.ValueSet();
On 2016/05/27 at 08:58:45, petrcermak wrote:
> How is this related to this patch?

This patch removes grouping_keys, which this code relied on.

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/diagnostic.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic.html:40: // When subclasses set
their prototype to an entirely new object and omit
On 2016/05/27 at 08:58:45, petrcermak wrote:
> BTW (unrelated to this patch): This is something that I think we should
address at the project level. I think we should provide a tr.b.extend API so
that everyone extends classes correctly.

+100
Or ES6 classes?

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
File tracing/tracing/value/diagnostics/diagnostic_map.html (right):

https://codereview.chromium.org/2000063006/diff/140001/tracing/tracing/value/...
tracing/tracing/value/diagnostics/diagnostic_map.html:55: diagnostics.add(name,
tr.v.d.Diagnostic.fromDict(d));
On 2016/05/27 at 08:58:45, petrcermak wrote:
> Don't you mean tr.v.d.Diagnostic.fromDict(d[name])? If so, this is indicating
a missing test. You can use iteration_helpers.html here as well:
> 
> tr.b.iterItems(d, (name, dict) => diagnostics.add(name,
tr.v.d.Diagnostic.fromDict(dict)));

Refactored, and value_test.genericDiagnostic catches this.

[benjhayden, 2016-05-27 20:45:34]

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

[commit-bot: I haz the power, 2016-05-27 20:45:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000063006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000063006/160001

[commit-bot: I haz the power, 2016-05-27 21:08:14]

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

[commit-bot: I haz the power, 2016-05-27 21:08:15]

Dry run: This issue passed the CQ dry run.

[benjhayden, 2016-05-27 22:39:48]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-05-27 22:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/2000063006/#ps160001
(title: "comments")

[commit-bot: I haz the power, 2016-05-27 22:39:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000063006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000063006/160001

[commit-bot: I haz the power, 2016-05-27 23:49:27]

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

[commit-bot: I haz the power, 2016-05-27 23:49:28]

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

[benjhayden, 2016-05-31 17:54:21]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-05-31 17:54:21]

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

[commit-bot: I haz the power, 2016-05-31 17:54:27]

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

[commit-bot: I haz the power, 2016-05-31 18:16:55]

Description was changed from

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(Generic, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180
==========

to

==========
Define DiagnosticMap and a basic Diagnostic hierarchy.

Currently, Value.diagnostics is a free-form dictionary.
Metrics can add anything they want to it!
The problem is that metrics don't know what to add to it.

This CL defines a simple hierarchy of strongly-typed Diagnostics
(Generic, RelatedValueSet, Composition),
with nice APIs like DiagnosticMap and RelatedValueSet.

Each subtype of Diagnostic is defined and registered in its own file,
so anybody can define and register a new kind of Diagnostic.

A followup CL will define a diagnostic-map-view and a registry of diagnostic
views
for the different types of Diagnostics.
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

In order to support the new RelatedValueSet and Composition diagnostics,
placeholder ValueRef objects are resolved into Values using ValueSet
after all Values are imported from dicts,
so that individual Values can be imported without their entire ValueSet.

The canonical_url diagnostic is unused, so this CL removes it.

Grouping keys are removed from Values as a first step towards refactoring them
into diagnostics, since this CL already significantly changes the Value API.

A followup CL will refactor Value's opt_options and remove 'important'.

The only metric that made extensive use of diagnostics was hazardMetric, which
is
being refactored to not use diagnostics anyway:
https://codereview.chromium.org/1992303003


BUG=catapult:#2180

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

[commit-bot: I haz the power, 2016-05-31 18:16:56]

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