Closed Bug 1158251 Opened 5 years ago Closed 5 years ago

Sub-session histograms should be cloned before adding to them

Categories

(Toolkit :: Telemetry, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox39 --- affected
firefox40 --- fixed

People

(Reporter: vladan, Assigned: Dexter)

References

Details

Attachments

(1 file, 5 obsolete files)

STR:

1. Launch Firefox (Nightly 40 & Aurora 39 are both affected), don't do anything that would trigger a new Telemetry subsession
2. Open about:telemetry
3. Compare the values of any subsession and full-session histograms, e.g. GC_MS

The subsession histogram will have 1 more value than the full-session histogram
Assignee: nobody → alessio.placitelli
Attached patch bug1158251 (obsolete) — Splinter Review
From my debug log:

**** DEBUG Adding to GC_MS (1)
**** DEBUG Cloning GC_MS (2)
**** DEBUG Adding to sub#GC_MS (3)

We're adding to the existing histogram, then cloning it to create the subsession histogram, then adding to the subsession histogram. Step (1) should happen after creating the subsession histogram.

Keyed histograms already behave like that.
Attachment #8597950 - Flags: review?(gfritzsche)
Attachment #8597950 - Attachment is patch: true
Comment on attachment 8597950 [details] [diff] [review]
bug1158251

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

Can we have test-coverage for this?

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1085,5 @@
>    if (Histogram* subsession = GetSubsessionHistogram(histogram)) {
>      subsession->Add(value);
>    }
>  #endif
> +  // It is safe to add to the histogram now: the subsession histogram was already

Nit: Line-break before this.
Attachment #8597950 - Flags: review?(gfritzsche) → review+
Attached patch bug1158251 - v2 (obsolete) — Splinter Review
Adds test coverage.
Attachment #8597950 - Attachment is obsolete: true
Attachment #8597978 - Flags: review?(gfritzsche)
Status: NEW → ASSIGNED
Attachment #8597978 - Attachment is patch: true
Comment on attachment 8597978 [details] [diff] [review]
bug1158251 - v2

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

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +613,5 @@
>    let h = Telemetry.getHistogramById(ID);
>    let flag = Telemetry.getHistogramById(FLAG);
>  
> +  // Instantiate the subsession histogram through |add| and make they match.
> +  h.add(1);

This only instantiates the subsession histogram on first use (not after being cleared).
I don't think we have a useful way to assert this scenario, so we should either:
* make it very clear in a comment that this should be the first use of TELEMETRY_TEST_COUNT in this file
* or put in a separate test file

Also, are keyed histograms prone to the same issue?
Attachment #8597978 - Flags: review?(gfritzsche)
Attached patch bug1158251.patch - v3 (obsolete) — Splinter Review
Attachment #8597978 - Attachment is obsolete: true
Attachment #8598011 - Flags: review?(gfritzsche)
Comment on attachment 8598011 [details] [diff] [review]
bug1158251.patch - v3

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

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +614,5 @@
>    let flag = Telemetry.getHistogramById(FLAG);
>  
> +  // Instantiate the subsession histogram through |add| and make sure they match.
> +  // This MUST be the first use of "TELEMETRY_TEST_COUNT" in this file, otherwise
> +  // |add| will not instantiate the histogram.

Ok, last request:
Lets move this check into say test_instantiate() and call this as the very first test function from run_test().
This should make it much less likely to be a footgun.
Attachment #8598011 - Flags: review?(gfritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
>
> Also, are keyed histograms prone to the same issue?

No, they behaved correctly already.
Attached patch bug1158251.patch - v5 (obsolete) — Splinter Review
Fixes the broken Android tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6fd09cea65f
Attachment #8598026 - Attachment is obsolete: true
Attachment #8598684 - Flags: review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebased
Attachment #8598684 - Attachment is obsolete: true
Attachment #8599118 - Flags: review+
Keywords: checkin-needed
Summary: Sub-session histograms are double-counting measurements? → Sub-session histograms should be cloned before adding to them
https://hg.mozilla.org/mozilla-central/rev/20f5e306d6b6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.