764585 - make it even harder to get enumerated histograms wrong

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[(dormant account)]

r+

Comment 2

[(no longer active)]

there's at least one sample of this for the macros.

Comment 3

[(no longer active)]

(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!

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

This blocks a FF 14 tracked bug.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: patch

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?

Comment 6

[(dormant account)]

Comment on attachment 656089 [details] [diff] [review]
patch

excellent

Comment 7

[Nathan Froyd [:froydnj]]

Green build: https://tbpl.mozilla.org/?tree=Try&rev=5d50758559d2

https://hg.mozilla.org/integration/mozilla-inbound/rev/30c7ffa7bd97

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30c7ffa7bd97