Last Comment Bug 747379 - Cloning a flag histogram with Telemetry::HistogramFrom breaks the "only one count" invariant
: Cloning a flag histogram with Telemetry::HistogramFrom breaks the "only one c...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla15
Assigned To: Graeme McCutcheon [:graememcc]
:
: Georg Fritzsche [:gfritzsche]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-20 07:22 PDT by Graeme McCutcheon [:graememcc]
Modified: 2012-04-29 14:00 PDT (History)
4 users (show)
graeme.mccutcheon: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Wallpaper (1.86 KB, patch)
2012-04-20 09:09 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
v1 (4.00 KB, patch)
2012-04-22 07:25 PDT, Graeme McCutcheon [:graememcc]
nfroyd: review+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2012-04-20 07:22:33 PDT
In a build with the fix for bug 747163...

graememcc@Vitalstatistix:~/moz/trunk/debug_mozwork/src/obj/dist/bin$ ./run-mozilla.sh ./xpcshell 
js> const Cc = Components.classes    
js> const Ci = Components.interfaces
js> const Cu = Components.utils
js> const Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry)
js> var h = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG")
js> var s = Telemetry.snapshot()
js> var s = h.snapshot()
js> uneval(s.counts)
[1, 0, 0]
js> var h2 = Telemetry.histogramFrom("BROKEN_FLAG", "TELEMETRY_TEST_FLAG")
js> var s2 = Telemetry.snapshot()
js> var s2 = h2.snapshot()
js> uneval(s2.counts)
[2, 0, 0]
Comment 1 Graeme McCutcheon [:graememcc] 2012-04-20 07:43:11 PDT
And of course...

js> var h = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG")
js> h.add(1)
js> var s = h.snapshot()
js> uneval(s.counts)
[0, 1, 0]
js> var h2 = Telemetry.histogramFrom("REALLY_BROKEN", "TELEMETRY_TEST_FLAG")
js> var s2 = h2.snapshot()
js> uneval(s2.counts)
[1, 1, 0]
Comment 2 Graeme McCutcheon [:graememcc] 2012-04-20 09:09:29 PDT
Created attachment 616989 [details] [diff] [review]
Wallpaper

It's a bug in the Chromium histogram code, Histogram::AddSampleSet will blindly add the sample, regardless of the histogram type, and so needs fixed upstream.

In the meantime, attached is a wallpaper patch (applies on top of 747163) to fix our use of it in Telemetry.
Comment 3 Graeme McCutcheon [:graememcc] 2012-04-20 09:28:54 PDT
Comment on attachment 616989 [details] [diff] [review]
Wallpaper

Ah, FlagHistogram didn't come from upstream. Real patch coming up.
Comment 4 Graeme McCutcheon [:graememcc] 2012-04-22 07:25:13 PDT
Created attachment 617316 [details] [diff] [review]
v1
Comment 5 (dormant account) 2012-04-26 10:49:53 PDT
Comment on attachment 617316 [details] [diff] [review]
v1

Nathan is our cloning person
Comment 6 Nathan Froyd [:froydnj] 2012-04-26 13:10:29 PDT
Comment on attachment 617316 [details] [diff] [review]
v1

Review of attachment 617316 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not keen about the path this patch takes, but I think all the other paths are worse (special-casing in Telemetry itself, using Accumulate in AddSampleSet, etc.).  So r=me with nits below fixed.

Please fix your patch's checkin comment to match reality: you're not not using Histogram::AddSampleSet, you're modifying it.

::: ipc/chromium/src/base/histogram.cc
@@ +970,5 @@
> +  //  - If our flag has already been set do nothing
> +  //  - Set our flag if the following hold
> +  //      - The sum of the counts in the provided SampleSet is 1
> +  //      - That single value is in the same bucket as we would place our flag
> +  //  - Otherwise, take no action

Please add periods after all the sentences in this comment.  Don't forget the ones that aren't shown in this bugzilla comment. ;)
Comment 7 Graeme McCutcheon [:graememcc] 2012-04-27 03:22:35 PDT
Agreed on it being the "least bad" choice.

I edited the comment slightly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/05422247ed55
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-04-29 14:00:08 PDT
http://hg.mozilla.org/mozilla-central/rev/05422247ed55

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