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

RESOLVED FIXED in Firefox 40

Status

()

Toolkit
Telemetry
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Updated

3 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 1

3 years ago
Created attachment 8597235 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
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?
Attachment #8597235 - Flags: feedback?(gfritzsche)
(Reporter)

Comment 3

3 years ago
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.
Attachment #8597235 - Flags: feedback?(gfritzsche)
(Assignee)

Comment 4

3 years ago
Created attachment 8597684 [details] [diff] [review]
part 1 - Change Telemetry to allow base histogram recording
Attachment #8597235 - Attachment is obsolete: true
Attachment #8597684 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

3 years ago
Created attachment 8597685 [details] [diff] [review]
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.
Attachment #8597685 - Flags: review?(gfritzsche)
(Assignee)

Comment 6

3 years ago
(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.
(Reporter)

Comment 7

3 years ago
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.
Attachment #8597685 - Flags: review?(gfritzsche) → review+
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8597986 [details] [diff] [review]
part 2 - Add test coverage.  v2

Thanks for your review, Georg.
Attachment #8597685 - Attachment is obsolete: true
Attachment #8597986 - Flags: review+
(Reporter)

Comment 9

3 years ago
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.
Attachment #8597684 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8598042 [details] [diff] [review]
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
Attachment #8597684 - Attachment is obsolete: true
Attachment #8598042 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b43aa4b5b5ed
https://hg.mozilla.org/mozilla-central/rev/1fda5226fef5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.