Last Comment Bug 732053 - make linear histograms harder to get wrong
: make linear histograms harder to get wrong
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-01 09:09 PST by Nathan Froyd [:froydnj]
Modified: 2012-03-16 06:31 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.29 KB, patch)
2012-03-08 10:17 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
patch (2.49 KB, patch)
2012-03-15 07:31 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-03-01 09:09:30 PST
Due to a quirk (?) in linear histograms, if you want a histogram with a single bucket for each value in 1..N, you need to declare it:

HISTOGRAM(NAME, 1, N, N+1, LINEAR, EXPLANATION)

and not:

HISTOGRAM(NAME, 1, N, N, LINEAR, EXPLANATION)

because you need to account for the implicit zero bucket at the bottom.

This has come up a couple times for new histograms, and at least GC_REASON seems to suffer from a wrong declaration.  SSL telemetry was/is going to use histograms like this, too.
Comment 1 Nathan Froyd [:froydnj] 2012-03-08 10:17:30 PST
Created attachment 604111 [details] [diff] [review]
patch

The dead simple patch.  Bikeshedding about the name welcome.
Comment 2 Nathan Froyd [:froydnj] 2012-03-08 10:25:08 PST
Histograms that could benefit from this macro:

GC_REASON
HTTP_*_DISPOSITION
CACHE_*_SEARCH (odd to have a linear histogram for ms histogram, especially one so finely bucketed)
CACHE_SERVICE_LOCK_WAIT* (same)
PLACES_EXPIRATION_STEPS_TO_CLEAN
DNS_LOOKUP_METHOD
UPDATE_STATUS

but most of those would involve a small tweak to the histogram (either in # of buckets or upper bound); doesn't seem worth the effort
Comment 3 (dormant account) 2012-03-09 13:03:37 PST
Comment on attachment 604111 [details] [diff] [review]
patch

HISTOGRAM_SINGLE_VALUE_BUCKET is the best you can come up with?  How about HISTOGRAM_CONTINUOUS?
Comment 4 Nathan Froyd [:froydnj] 2012-03-09 13:13:19 PST
HISTOGRAM_CONTINUOUS doesn't sound right.  HISTOGRAM_ENUMERATED_VALUES (since most people seem to be sticking enums in these anyway)?
Comment 5 (dormant account) 2012-03-09 14:50:15 PST
(In reply to Nathan Froyd (:froydnj) from comment #4)
> HISTOGRAM_CONTINUOUS doesn't sound right.  HISTOGRAM_ENUMERATED_VALUES
> (since most people seem to be sticking enums in these anyway)?

r+
Comment 6 Nathan Froyd [:froydnj] 2012-03-15 07:31:51 PDT
Created attachment 606203 [details] [diff] [review]
patch

Use HISTOGRAM_ENUMERATED_VALUES.  Adding r+ per Taras's earlier comment.
Comment 7 Mozilla RelEng Bot 2012-03-15 07:35:54 PDT
Autoland Patchset:
	Patches: 606203
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=a9acecdb826d
Try run started, revision a9acecdb826d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=a9acecdb826d
Comment 8 Mozilla RelEng Bot 2012-03-15 12:31:50 PDT
Try run for a9acecdb826d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a9acecdb826d
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-a9acecdb826d
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-03-15 17:13:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/da0a5a4b0519
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-16 06:31:17 PDT
https://hg.mozilla.org/mozilla-central/rev/da0a5a4b0519

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