Generalize delta statistics in Histogram.

Generalize delta statistics in Histogram.

Currently, all delta statistics are handled specially everywhere.
There is no way to compute delta min, max, sum, count, or percentiles.

This CL generalizes support for delta statistics versions of all possible
statistics in Histogram.

BUG=catapult:#3319

Review-Url: https://codereview.chromium.org/2742163002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/05828b4341b99a4a54d24a5efa800e6aa9791a1d

[benjhayden, 2017-03-11 21:11:25]

Description was changed from

==========
Compute inter-percentile range statistics in Histogram.

BUG=catapult:#3319
==========

to

==========
Compute inter-percentile range statistics in Histogram.

Currently, the only statistic that measures noise is std, but this is not
strictly applicable to non-normal distributions, such as most of our metrics.

Inter-quartile range is another measure of noise that is more broadly
applicable.
This CL generalizes IQR to inter-percentile ranges:
histogram.customizeSummaryOptions({iprs: [
tr.b.Range.fromExplicitRange(0.25, 0.75), tr.b.Range.fromExplicitRange(0.1,
0.9),
]});

In order to support delta IPR statistics, this CL also generalizes delta
statistics
generally.

BUG=catapult:#3319
==========

[benjhayden, 2017-03-11 22:06:58]

Patchset #1 (id:1) has been deleted

[benjhayden, 2017-03-11 22:07:03]

Patchset #1 (id:20001) has been deleted

[benjhayden, 2017-03-11 22:07:07]

Patchset #1 (id:40001) has been deleted

[benjhayden, 2017-03-11 22:07:13]

Patchset #1 (id:60001) has been deleted

[benjhayden, 2017-03-11 22:07:18]

Patchset #1 (id:80001) has been deleted

[benjhayden, 2017-03-11 22:11:56]

benjhayden@chromium.org changed reviewers:
+ eakuefner@chromium.org, tdresser@chromium.org

[benjhayden, 2017-03-11 22:11:56]

PTAL :-)

[tdresser, 2017-03-13 21:26:00]

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
File tracing/tracing/value/csv_builder.html (right):

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/csv_builder.html:73: for (const name of
hist.statisticsNames) {
Could this just be:
this.statisticsNames_ = new Set(hist.statisticsNames);

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:32: * @param {number} percent The percent
must be between 0.0 and 1.0.
Add new @param

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:521: *                                     
              (0,1)
Is this the right amount of indentation? I thought it would just be 4 extra
spaces.

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:534: * @throws {Error} when statName is not
recognized, such as delta statistics
when -> When

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:535: * when
!this.canCompare(opt_referenceHistograms).
Should this be indented?

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:627: * @return {!Array.<string>}
I'd move the description to be part of the @return, personally. Not sure if that
lines up with our general usage?

[benjhayden, 2017-03-13 21:56:54]

Thanks!

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
File tracing/tracing/value/csv_builder.html (right):

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/csv_builder.html:73: for (const name of
hist.statisticsNames) {
On 2017/03/13 at 21:26:00, tdresser wrote:
> Could this just be:
> this.statisticsNames_ = new Set(hist.statisticsNames);

No, this is inside a loop over all histograms.

[tdresser, 2017-03-14 14:25:55]

LGTM with nits (but obviously wait for Ethan's feedback).

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
File tracing/tracing/value/csv_builder.html (right):

https://codereview.chromium.org/2742163002/diff/100001/tracing/tracing/value/...
tracing/tracing/value/csv_builder.html:73: for (const name of
hist.statisticsNames) {
On 2017/03/13 21:56:54, benjhayden wrote:
> On 2017/03/13 at 21:26:00, tdresser wrote:
> > Could this just be:
> > this.statisticsNames_ = new Set(hist.statisticsNames);
> 
> No, this is inside a loop over all histograms.

D'oh.

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:33: * @param {boolean=} opt_force3 Whether
to force the result to be 3 chars long
I'm not sure if there's a standard here, but should we indicate what the default
is somewhere? Right now, you need to find the usage of this to determine what
the default is.

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:522: *     (0,1)
Nittiest nit: missing "." at the end.

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:564: if (statName.substr(0, 4) === 'pct_')
{
Here and below, could we split on '_', instead of substringing? It's a bit
slower, but I suspect it would be a lot more readable.

I'm not sure what the naming should be, but something like:
[prefix, lower, upper] = statName.split('_');

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/ui/histogram_set_table_test.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/ui/histogram_set_table_test.html:884: for (const hist of
histograms) {
Doesn't customizeSummaryOptions mutate?

[benjhayden, 2017-03-14 16:04:50]

Thanks!

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:33: * @param {boolean=} opt_force3 Whether
to force the result to be 3 chars long
On 2017/03/14 at 14:25:55, tdresser wrote:
> I'm not sure if there's a standard here, but should we indicate what the
default is somewhere? Right now, you need to find the usage of this to determine
what the default is.

Between all of the comments about this function, it looks like you have
correctly deduced that opt_force3 is a new wart on top of an existing wart.
The original wart is how the string format is not consistent: it might return
'xxx' or 'xxx_y'. opt_force3 prevents 'xxx_y' in order to support the
'ipr_lll_uuu' string handling.
I probably should eliminate the possiblity for 'xxx_y' altogether, but that
didn't seem like a job for this CL since it could theoretically impact metrics
and break dashboard monitoring. I don't think there are any '_pct_xxx_y'
timeseries that are monitored, and I don't think that there are any metrics that
rely on 'xxx_y', but fixing the original wart didn't seem like a job for this
CL.
opt_force3 will not hinder fixing the original wart.

I'm open to argument as always. :-)

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/ui/histogram_set_table_test.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/ui/histogram_set_table_test.html:884: for (const hist of
histograms) {
On 2017/03/14 at 14:25:55, tdresser wrote:
> Doesn't customizeSummaryOptions mutate?

JS const !== C++ const.
JS const-declared objects are mutable, but the name-value binding is not.
Making a JS object immutable requires using Object.freeze(), which is
discouraged due to performance concerns.

[tdresser, 2017-03-14 16:56:32]

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/histogram.html:33: * @param {boolean=} opt_force3 Whether
to force the result to be 3 chars long
On 2017/03/14 16:04:50, benjhayden wrote:
> On 2017/03/14 at 14:25:55, tdresser wrote:
> > I'm not sure if there's a standard here, but should we indicate what the
> default is somewhere? Right now, you need to find the usage of this to
determine
> what the default is.
> 
> Between all of the comments about this function, it looks like you have
> correctly deduced that opt_force3 is a new wart on top of an existing wart.
> The original wart is how the string format is not consistent: it might return
> 'xxx' or 'xxx_y'. opt_force3 prevents 'xxx_y' in order to support the
> 'ipr_lll_uuu' string handling.
> I probably should eliminate the possiblity for 'xxx_y' altogether, but that
> didn't seem like a job for this CL since it could theoretically impact metrics
> and break dashboard monitoring. I don't think there are any '_pct_xxx_y'
> timeseries that are monitored, and I don't think that there are any metrics
that
> rely on 'xxx_y', but fixing the original wart didn't seem like a job for this
> CL.
> opt_force3 will not hinder fixing the original wart.
> 
> I'm open to argument as always. :-)

Ha, gotcha.
SGTM

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
File tracing/tracing/value/ui/histogram_set_table_test.html (right):

https://codereview.chromium.org/2742163002/diff/120001/tracing/tracing/value/...
tracing/tracing/value/ui/histogram_set_table_test.html:884: for (const hist of
histograms) {
On 2017/03/14 16:04:50, benjhayden wrote:
> On 2017/03/14 at 14:25:55, tdresser wrote:
> > Doesn't customizeSummaryOptions mutate?
> 
> JS const !== C++ const.
> JS const-declared objects are mutable, but the name-value binding is not.
> Making a JS object immutable requires using Object.freeze(), which is
> discouraged due to performance concerns.

Ah, thanks for clarifying.
SGTM.

[benjhayden, 2017-03-14 18:18:23]

I'll split adding IPRs to another CL, and keep this one just for generalizing
delta statistics.

[benjhayden, 2017-03-14 20:15:19]

Description was changed from

==========
Compute inter-percentile range statistics in Histogram.

Currently, the only statistic that measures noise is std, but this is not
strictly applicable to non-normal distributions, such as most of our metrics.

Inter-quartile range is another measure of noise that is more broadly
applicable.
This CL generalizes IQR to inter-percentile ranges:
histogram.customizeSummaryOptions({iprs: [
tr.b.Range.fromExplicitRange(0.25, 0.75), tr.b.Range.fromExplicitRange(0.1,
0.9),
]});

In order to support delta IPR statistics, this CL also generalizes delta
statistics
generally.

BUG=catapult:#3319
==========

to

==========
Generalize delta statistics in Histogram.

Currently, all delta statistics are handled specially everywhere.
There is no way to compute delta min, max, sum, count, or percentiles.

This CL generalizes support for delta statistics versions of all possible
statistics in Histogram.

BUG=catapult:#3319
==========

[benjhayden, 2017-03-14 20:16:48]

Patchset #3 (id:140001) has been deleted

[benjhayden, 2017-03-14 20:16:51]

Patchset #3 (id:160001) has been deleted

[benjhayden, 2017-03-14 20:17:30]

Patchset #3 (id:180001) has been deleted

[benjhayden, 2017-03-14 20:20:11]

Patchset #3 (id:200001) has been deleted

[benjhayden, 2017-03-14 20:22:24]

Patchset #3 (id:220001) has been deleted

[benjhayden, 2017-03-14 20:34:16]

PTAL

IPRs are split out to https://codereview.chromium.org/2746363003

[eakuefner, 2017-03-17 20:09:30]

lgtm

[benjhayden, 2017-03-17 20:10:09]

https://codereview.chromium.org/2742163002/diff/240001/tracing/tracing/value/...
File tracing/tracing/value/ui/histogram_set_table_test.html (right):

https://codereview.chromium.org/2742163002/diff/240001/tracing/tracing/value/...
tracing/tracing/value/ui/histogram_set_table_test.html:452:
table.displayStatistic = '%' + tr.v.DELTA + 'avg';
format strings

[benjhayden, 2017-03-18 05:54:42]

Format strings are done.

I noticed that 21 statistics is a lot: 6 statistics enabled by default times 3
(non-delta, abs delta, %delta) plus 3 (z-score, U, p-value).

I hacked a blacklist in histogram-span to hide all of the new general delta
statistics so this CL doesn't make the existing firehose any worse.
I have some ideas to allow users to control which are hidden, and allow me to
make sane defaults: https://github.com/catapult-project/catapult/issues/3380

Committing this for now.
Thanks!

[benjhayden, 2017-03-18 05:55:03]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2017-03-18 05:55:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, eakuefner@chromium.org
Link to the patchset: https://codereview.chromium.org/2742163002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2017-03-18 05:55:12]

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-03-18 06:17:37]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489816503602090,
"parent_rev": "73f2994d743b2ba418ecba8493a2152ce93d09ee", "commit_rev":
"05828b4341b99a4a54d24a5efa800e6aa9791a1d"}

[commit-bot: I haz the power, 2017-03-18 06:17:41]

Description was changed from

==========
Generalize delta statistics in Histogram.

Currently, all delta statistics are handled specially everywhere.
There is no way to compute delta min, max, sum, count, or percentiles.

This CL generalizes support for delta statistics versions of all possible
statistics in Histogram.

BUG=catapult:#3319
==========

to

==========
Generalize delta statistics in Histogram.

Currently, all delta statistics are handled specially everywhere.
There is no way to compute delta min, max, sum, count, or percentiles.

This CL generalizes support for delta statistics versions of all possible
statistics in Histogram.

BUG=catapult:#3319

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

[commit-bot: I haz the power, 2017-03-18 06:17:42]

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