Closed Bug 764585 Opened 8 years ago Closed 7 years ago

make it even harder to get enumerated histograms wrong

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

Bug 763359, comment 3:

Because multiple teams got tripped up by this, I highly recommend that we change the API so that one **cannot** create a linear histogram from (1-n) with n buckets, by adding a MOZ_STATIC_ASSERT for that condition. For the case where this is a false positive, the person who defines the telemetry can just change the bucket count to (n+1). For the case where this check would prevent a problem, it will avoid collecting bad data.
r+
there's at least one sample of this for the macros.
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> there's at least one sample of this for the macros.

That is, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#196.

Turns out that bugzilla comments typed in on a phone aren't that useful!
This blocks a FF 14 tracked bug.
Depends on: 781200
Depends on: 785959
Attached patch patchSplinter Review
This patch adds a check for high > n_buckets for both linear and exponential histograms, in an attempt to eliminate the common mistake of using a linear histogram with high == n_buckets == N to record values from 1 to N.  It's still possible to make errors, of course, but I think this is the best we can do short of rewriting how the histogram code at its core works.  WDYT?
Attachment #656089 - Flags: review?(taras.mozilla)
Comment on attachment 656089 [details] [diff] [review]
patch

excellent
Attachment #656089 - Flags: review?(taras.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/30c7ffa7bd97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.