The default bug view has changed. See this FAQ.

make it even harder to get enumerated histograms wrong

RESOLVED FIXED in mozilla18

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 781200
(Assignee)

Updated

5 years ago
Depends on: 785959
(Assignee)

Comment 5

5 years ago
Created attachment 656089 [details] [diff] [review]
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?
Attachment #656089 - Flags: review?(taras.mozilla)

Comment 6

5 years ago
Comment on attachment 656089 [details] [diff] [review]
patch

excellent
Attachment #656089 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/30c7ffa7bd97
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/30c7ffa7bd97
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.