Last Comment Bug 711195 - Perform the range checks for non-boolean telemetry pings at compile time
: Perform the range checks for non-boolean telemetry pings at compile time
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on:
Blocks: 708123
  Show dependency treegraph
 
Reported: 2011-12-15 11:29 PST by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2011-12-28 13:09 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
Patch (v1) (4.03 KB, patch)
2011-12-15 11:32 PST, :Ehsan Akhgari (busy, don't ask for review please)
taras.mozilla: review+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2011-12-15 11:29:27 PST
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.
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-15 11:32:47 PST
Created attachment 582047 [details] [diff] [review]
Patch (v1)
Comment 2 (dormant account) 2011-12-15 15:09:12 PST
Comment on attachment 582047 [details] [diff] [review]
Patch (v1)

Whoa.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-16 11:31:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f93eaff43d0
Comment 4 Matt Brubeck (:mbrubeck) 2011-12-17 09:16:44 PST
https://hg.mozilla.org/mozilla-central/rev/9f93eaff43d0
Comment 5 Andrew McCreight [:mccr8] 2011-12-19 15:45:34 PST
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 (dormant account) 2011-12-19 17:26:05 PST
(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 Andrew McCreight [:mccr8] 2011-12-19 17:30:03 PST
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 (dormant account) 2011-12-19 17:34:36 PST
(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 Andrew McCreight [:mccr8] 2011-12-20 10:10:44 PST
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).
Comment 10 christian 2011-12-21 16:07:40 PST
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" ;-)
Comment 11 Andrew McCreight [:mccr8] 2011-12-24 06:30:28 PST
I removed the fix for UPDATE_STATUS which is not in beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/1e13590c5969
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:09:46 PST
Marking qa- as I don't think this is anything QA needs to verify. Please mark qa+ if this is not the case.

Note You need to log in before you can comment on or make changes to this bug.