747379 - Cloning a flag histogram with Telemetry::HistogramFrom breaks the "only one count" invariant

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Graeme McCutcheon [:graememcc]]

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]]

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]]

Attachment: 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]]

Comment on attachment 616989 [details] [diff] [review]
Wallpaper

Ah, FlagHistogram didn't come from upstream. Real patch coming up.

Comment 4

[Graeme McCutcheon [:graememcc]]

Attachment: v1

Comment 5

[(dormant account)]

Comment on attachment 617316 [details] [diff] [review]
v1

Nathan is our cloning person

Comment 6

[Nathan Froyd [:froydnj]]

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]]

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]]

http://hg.mozilla.org/mozilla-central/rev/05422247ed55