1157282 - Telemetry histograms for base set are not recorded when canRecordExtended is false

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

See e.g. here:
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l1213

This should record when either:
* `canRecordExtended==true` or
* `canRecordBase==true && histogram.dataset == DATASET_RELEASE_CHANNEL_OPTOUT`

This was introduced by bug 1134279. We are also apparently missing test-coverage for this behavior.

This affects:
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l1213
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l3625
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l3637
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l3650
https://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/toolkit/components/telemetry/Telemetry.cpp#l4165

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1157282.patch

This patch allows Telemetry histogram recording to deal with the following cases:

- If |canRecordExtended| is true, record the histogram data.
- If |canRecordBase| is true and the histogram belongs to the base set, record the histogram data.
- If |canRecordBase| is true, |canRecordExtended| is false, and the histogram was added at runtime, don't record the histogram data.

Appropriate testing was added to ns_ITelemetry.js.

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8597235 [details] [diff] [review]
bug1157282.patch

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ -3621,5 @@
>  
>  void
>  Accumulate(ID aHistogram, uint32_t aSample)
>  {
> -  if (!TelemetryImpl::CanRecordExtended()) {

Instead of completely removing this, how about checking for CanRecordBase and bailing out early if it's disabled? We could simplify HistogramAdd a bit and avoid the useless histogram lookup (which shouldn't be a huge deal).

What do you think?

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +581,5 @@
> +  const TEST_KEY = "record_foo";
> +  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTOUT");
> +  let orig = h.snapshot();
> +  h.add(TEST_KEY, 1);
> +  Assert.equal(uneval(orig), uneval(h.snapshot()));

Is this the right way to check keyed histograms?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8597235 [details] [diff] [review]
bug1157282.patch

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

The test mostly looks good.

For the Telemetry.cpp changes i think we should try to avoid additional lookups where it is reasonable:
* for plain histograms, make HistogramAdd take a dataset argument (so we can pass it on from where it is easily available)
* for keyed histograms, make KeyedHistogram store a dataset member on construction

To make things cleaner i think we should:
* add TelemetryImpl::CanRecordDataset() to consolidate the logic
* consolidate this with IsInDataset() if reasonable?

::: toolkit/components/telemetry/Telemetry.cpp
@@ -3621,5 @@
>  
>  void
>  Accumulate(ID aHistogram, uint32_t aSample)
>  {
> -  if (!TelemetryImpl::CanRecordExtended()) {

Yes, i think we should do that.
I think we need to check `!CanRecordBase() && !CanRecordExtended()` (or add a TelemetryImpl::CanRecord() that does that).

@@ +4175,5 @@
>  {
> +  // Add to the keyed histogram only if:
> +  // - we are recording the extended telemetry data;
> +  // - we are recording the base telemetry data and the histogram is part of it;
> +  if (!TelemetryImpl::CanRecordExtended()){

Lets move the logic to a helper KeyedHistogram::CanRecord()?

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +368,5 @@
> +
> +  let h = Telemetry.getHistogramById("TELEMETRY_TEST_RELEASE_OPTOUT");
> +  let orig = h.snapshot();
> +  h.add(1);
> +  Assert.equal(uneval(orig), uneval(h.snapshot()));

Just use snapshot.sum() for comparison.

@@ +390,5 @@
> +  h.add(1);
> +  Assert.equal(uneval(orig), uneval(h.snapshot()),
> +               "Histograms should be equal after recording.");
> +
> +  // Check that extended histograms are recorded when required.

... and that base histograms are still recorded.

@@ +581,5 @@
> +  const TEST_KEY = "record_foo";
> +  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTOUT");
> +  let orig = h.snapshot();
> +  h.add(TEST_KEY, 1);
> +  Assert.equal(uneval(orig), uneval(h.snapshot()));

You can always use Assert.deepEqual instead of equal + uneval (and avoid property ordering issues in the process).

A simpler check to use here and below is:
equal(h.snapshot(TEST_KEY).sum, expectedValue)

@@ +603,5 @@
> +  h.add(TEST_KEY, 1);
> +  Assert.equal(uneval(orig), uneval(h.snapshot()),
> +               "Keyed histograms should be equal after recording.");
> +
> +  // Check that extended histograms are recorded when required.

... and that base histograms are still recorded as well.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change Telemetry to allow base histogram recording

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage.

Since the patch grew a bit, I've split it in two. This one deals with test coverage: I've addressed your concerns from the previous round.

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> ::: toolkit/components/telemetry/Telemetry.cpp
> @@ -3621,5 @@
> >  
> >  void
> >  Accumulate(ID aHistogram, uint32_t aSample)
> >  {
> > -  if (!TelemetryImpl::CanRecordExtended()) {
> 
> Yes, i think we should do that.
> I think we need to check `!CanRecordBase() && !CanRecordExtended()` (or add
> a TelemetryImpl::CanRecord() that does that).
>

Do you think we should really check for both conditions? We only really want to bail out if |CanRecordBase()| is false: we can't record extended telemetry without base telemetry, but we might want to record base histograms.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8597685 [details] [diff] [review]
part 2 - Add test coverage.

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

Looks good with the below fixed.

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +374,5 @@
> +  // Check that only base histograms are recorded.
> +  Telemetry.canRecordBase = true;
> +  h.add(1);
> +  Assert.notEqual(orig.sum, h.snapshot().sum,
> +                  "Histograms should be different after recording.");

You can just do the stricter assert here: check that the value was incremented by 1.

@@ +395,5 @@
> +  Telemetry.canRecordExtended = true;
> +
> +  h.add(1);
> +  Assert.notEqual(orig.sum, h.snapshot().sum,
> +                  "Runtime histograms should be different after recording.");

Dito.

@@ +401,5 @@
> +  h = Telemetry.getHistogramById("TELEMETRY_TEST_RELEASE_OPTIN");
> +  orig = h.snapshot();
> +  h.add(1);
> +  Assert.notEqual(orig.sum, h.snapshot().sum,
> +               "Histograms should be different after recording.");

Dito.

@@ +409,5 @@
> +  h.clear();
> +  orig = h.snapshot();
> +  h.add(1);
> +  Assert.notEqual(orig.sum, h.snapshot().sum,
> +                  "Histograms should be different after recording.");

Dito.

@@ +588,5 @@
> +
> +  const TEST_KEY = "record_foo";
> +  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTOUT");
> +  h.add(TEST_KEY, 1);
> +  Assert.equal(h.snapshot(TEST_KEY).sum, 0);

When comparing histograms (plain or keyed) to an explicit value, you'll want to .clear() them at the start of the test.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage. v2

Thanks for your review, Georg.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8597684 [details] [diff] [review]
part 1 - Change Telemetry to allow base histogram recording

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +895,5 @@
> +CanRecordDataset(uint32_t dataset)
> +{
> +  // Add to the histogram only if:
> +  // - we are recording the extended telemetry data;
> +  // - we are recording the base telemetry data and the histogram is part of it;

This comment looks like a left-over from moving code.
It needs to be changed for the context of this function.

@@ +1958,5 @@
>      return rv;
>    }
>  
> +  KeyedHistogram* keyed = new KeyedHistogram(name, expiration, histogramType, min, max, bucketCount,
> +                                             nsITelemetry::DATASET_RELEASE_CHANNEL_OPTIN);

Nit: keep |min, max, bucketCount| on new line.

@@ +3673,5 @@
>    }
>    Histogram *h;
>    nsresult rv = GetHistogramByEnumId(aHistogram, &h);
>    if (NS_SUCCEEDED(rv))
> +    HistogramAdd(*h, aSample, gHistograms[aHistogram].dataset);

Let's wrap this with {} while we're here.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change Telemetry to allow base histogram recording. v2

Thank you for your time, Georg.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f351cd8b151

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b43aa4b5b5ed
https://hg.mozilla.org/integration/fx-team/rev/1fda5226fef5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b43aa4b5b5ed
https://hg.mozilla.org/mozilla-central/rev/1fda5226fef5