Closed
Bug 711195
Opened 13 years ago
Closed 13 years ago
Perform the range checks for non-boolean telemetry pings at compile time
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla10
Tracking | Status | |
---|---|---|
firefox9 | --- | unaffected |
firefox10 | --- | fixed |
firefox11 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file)
4.03 KB,
patch
|
taras.mozilla
:
review+
christian
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
I was bitten by not reading the docs correctly, and use 0 as the minimum value for the UPDATE_STATUS histogram. I'm adding a compile-time check to catch these types of mistakes at compile time, to reduce the pain for the next person to hit this bug. I have a patch for doing that. I'm also fixing the UPDATE_STATUS and GC_REASON probes which were affected by this bug.
Assignee | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
Comment on attachment 582047 [details] [diff] [review] Patch (v1) Whoa.
Attachment #582047 -
Flags: review?(taras.mozilla) → review+
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 3•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f93eaff43d0
Comment 4•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f93eaff43d0
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 5•13 years ago
|
||
This has been landed for a few days. Looking at my about:telemetry, I see that GC_REASON is showing up now, where it wasn't before. But also I notice that more than half of the values are 0, which is less than the minimum bucket size (this telemetry reports an enum). Is it okay to report 0 values when the minimum bucket is 0? That seems weird to me.
Comment 6•13 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #5) > This has been landed for a few days. Looking at my about:telemetry, I see > that GC_REASON is showing up now, where it wasn't before. But also I notice > that more than half of the values are 0, which is less than the minimum > bucket size (this telemetry reports an enum). Is it okay to report 0 values > when the minimum bucket is 0? That seems weird to me. minimum bucket is the first non-0 bucket. It's a little confusing, there is always a 0 bucket.
Comment 7•13 years ago
|
||
Ah, I see. Thanks. Do you want to try to get this into 10? I guess it will be rolling over into beta tomorrow, but it seems like a fairly low risk change, and having GC_REASON working might give some information about GC scheduling regressions. Just a thought.
Comment 8•13 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #7) > Ah, I see. Thanks. Do you want to try to get this into 10? I guess it > will be rolling over into beta tomorrow, but it seems like a fairly low risk > change, and having GC_REASON working might give some information about GC > scheduling regressions. Just a thought. up to you. There is 0 risk in this patch.
Comment 9•13 years ago
|
||
Comment on attachment 582047 [details] [diff] [review] Patch (v1) Nominating for Firefox 10 because it is low risk and may let us get more information about garbage collector performance in Firefox 10 via telemetry (GC_REASON was broken prior to this patch).
Attachment #582047 -
Flags: approval-mozilla-beta?
Comment 10•13 years ago
|
||
Comment on attachment 582047 [details] [diff] [review] Patch (v1) [triage comment] Approving for beta. Looks low risk and the additional data may uncover other issues. Please land as soon as possible. Also note that there is never "0 risk" ;-)
Attachment #582047 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•13 years ago
|
||
I removed the fix for UPDATE_STATUS which is not in beta. https://hg.mozilla.org/releases/mozilla-beta/rev/1e13590c5969
Target Milestone: mozilla11 → mozilla10
Updated•13 years ago
|
Comment 12•13 years ago
|
||
Marking qa- as I don't think this is anything QA needs to verify. Please mark qa+ if this is not the case.
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•