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

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Telemetry
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: graememcc, Assigned: graememcc)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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]
(Assignee)

Comment 1

5 years ago
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]
(Assignee)

Comment 2

5 years ago
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.
Attachment #616989 - Flags: review?(taras.mozilla)
(Assignee)

Comment 3

5 years ago
Comment on attachment 616989 [details] [diff] [review]
Wallpaper

Ah, FlagHistogram didn't come from upstream. Real patch coming up.
Attachment #616989 - Flags: review?(taras.mozilla)
(Assignee)

Comment 4

5 years ago
Created attachment 617316 [details] [diff] [review]
v1
Attachment #616989 - Attachment is obsolete: true
Attachment #617316 - Flags: review?(taras.mozilla)

Comment 5

5 years ago
Comment on attachment 617316 [details] [diff] [review]
v1

Nathan is our cloning person
Attachment #617316 - Flags: review?(taras.mozilla) → review?(nfroyd)
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. ;)
Attachment #617316 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 7

5 years ago
Agreed on it being the "least bad" choice.

I edited the comment slightly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/05422247ed55
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → mozilla15
http://hg.mozilla.org/mozilla-central/rev/05422247ed55
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.