1145164 - Allow adding values >1 to "count" histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Roberto Agostino Vitillo (:rvitillo)]

Currently a count histogram can only be incremented by one with each add/accumulate call. Ingoring the passed value is confusing and makes it easy to use the interface in the wrong way (see https://bugzilla.mozilla.org/show_bug.cgi?id=1141915#c5). 

Either we remove the parameter or we use it to increment the count by its value. The latter option though would break the semantic of a histogram. Using a histogram to represent a count doesn't seem to me a good idea in the first place.

Comment 1

[Benjamin Smedberg]

This really shouldn't be a histogram, it's just a counter. We're using the histogram mechanism because it's a good way to list and record measurements. But I do think we should increment by the passed value.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: WIP patch

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1145164.patch

This patch allows non-unitary increments to count histograms, adds test coverage and (bonus) fixes some JS syntax errors in the test file.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8753734 [details] [diff] [review]
bug1145164.patch

Review of attachment 8753734 [details] [diff] [review]:
-----------------------------------------------------------------

Have you checked whether anyone already accumulates values > 1 for count histograms and might see regressions because of this patch?
A rough test run with an assertion on calls with values > 1 could be a simple sanity check.

::: ipc/chromium/src/base/histogram.cc
@@ +1075,5 @@
>  void
>  CountHistogram::Accumulate(Sample value, Count count, size_t index)
>  {
>    size_t zero_index = BucketIndex(0);
> +  LinearHistogram::Accumulate(value, 1, zero_index);

I think we should flip the order of those.
The sum increases correctly with this approach, but not the count of values in the zero bucket - see e.g. the visualization in about:telemetry when adding values > 1.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1003,4 @@
>    int32_t value = 1;
> +  bool shouldCheckArgs =
> +    (type != base::CountHistogram::COUNT_HISTOGRAM) ||
> +    ((type == base::CountHistogram::COUNT_HISTOGRAM) && args.length());

This looks redundant?
(!a || (a && b)) -> (!a || b)

@@ +1235,4 @@
>    int32_t value = 1;
> +  bool shouldCheckArgs =
> +    (type != base::CountHistogram::COUNT_HISTOGRAM) ||
> +    ((type == base::CountHistogram::COUNT_HISTOGRAM) && args.length() == 2);

Ditto on the condition.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Comment on attachment 8753734 [details] [diff] [review]
> bug1145164.patch
> 
> Review of attachment 8753734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have you checked whether anyone already accumulates values > 1 for count
> histograms and might see regressions because of this patch?
> A rough test run with an assertion on calls with values > 1 could be a
> simple sanity check.

Try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=192f80158a23

The only real "issue" seems to be the test_TelemetryStopwatch.js [1] test, which is using a count histogram in the test. This shouldn't be a problem, as we're only crashing because of my MOZ_CRASH. The test runs just fine without it.

> ::: ipc/chromium/src/base/histogram.cc
> @@ +1075,5 @@
> >  void
> >  CountHistogram::Accumulate(Sample value, Count count, size_t index)
> >  {
> >    size_t zero_index = BucketIndex(0);
> > +  LinearHistogram::Accumulate(value, 1, zero_index);
> 
> I think we should flip the order of those.
> The sum increases correctly with this approach, but not the count of values
> in the zero bucket - see e.g. the visualization in about:telemetry when
> adding values > 1.

With the current patch, bucket zero contains the number of time |hist.add()| was called (e.g. the number of samples), even for values > 1. Unfortunately, swapping the parameters doesn't seem to set bucket zero to the same value as "sum". It will, instead, show the value of the last submitted sample.

[1] - https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js#12

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > Comment on attachment 8753734 [details] [diff] [review]
> > bug1145164.patch
> > 
> > Review of attachment 8753734 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Have you checked whether anyone already accumulates values > 1 for count
> > histograms and might see regressions because of this patch?
> > A rough test run with an assertion on calls with values > 1 could be a
> > simple sanity check.
> 
> Try push for that:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=192f80158a23

Also worth mentioning that I ran the patch through some manual browsing/search/navigation tests. Nothing blew up.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug1145164.patch

This patch addresses the |shouldCheckArgs| conditions.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8754807 [details] [diff] [review]
bug1145164.patch

Review of attachment 8754807 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1003,5 @@
>    int32_t value = 1;
> +  bool shouldCheckArgs =
> +    (type != base::CountHistogram::COUNT_HISTOGRAM) || args.length();
> +
> +  if (shouldCheckArgs) {

I don't think we need the extra shouldCheckArgs here now, i'd rather have it directly in the condition.

@@ +1237,2 @@
>  
> +  if (shouldCheckArgs) {

Ditto on moving this in the branching condition.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: bug1145164.patch

Comment 10

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc4098b51ce

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/62a14696a5a7

Comment 12

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/62a14696a5a71de6ddea031c128f46c8662662d2
Bug 1145164 - Allow non-unitary increments to count histograms. r=gfritzsche

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62a14696a5a7