Closed
Bug 1157282
Opened 9 years ago
Closed 9 years ago
Telemetry histograms for base set are not recorded when canRecordExtended is false
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: Dexter)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → alessio.placitelli
Assignee | ||
Comment 1•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Attachment #8597235 -
Attachment is obsolete: true
Attachment #8597684 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 5•9 years ago
|
||
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•9 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•9 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•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•9 years ago
|
||
Thanks for your review, Georg.
Attachment #8597685 -
Attachment is obsolete: true
Attachment #8597986 -
Flags: review+
Reporter | ||
Comment 9•9 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•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b43aa4b5b5ed https://hg.mozilla.org/integration/fx-team/rev/1fda5226fef5
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b43aa4b5b5ed https://hg.mozilla.org/mozilla-central/rev/1fda5226fef5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
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.
Description
•