Regression: # of BHR jank events doesn't match # of BHR jank stacks

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
Telemetry
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: vladan, Assigned: avih)

Tracking

46 Branch
mozilla47
Points:
---

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

User Story

STR:
1. Cause some jank events > 127ms
2. In about:telemetry -> "Thread Hangs", for the Gecko thread, compare # of jank events in the BHR event histogram vs sum of events reported in each BHR stack

These numbers should match

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
In IRC, :jchen mentioned that when accessing histograms from JS, there's an additional bucket prepended to the time histograms.

That means that the ranges we were using in the histograms are incorrect. For example, bucket 7 in JS actually represents 64-127ms, rather than 128-255. That means that the way we're interpreting it puts it in the next bucket, which increases the reported hang duration range.

The fix would be to add 1 to the index used in the range. I am currently implementing and testing this in statuser, but about:telemetry also needs to be updated.
(Reporter)

Updated

a year ago
Duplicate of this bug: 1241296
(Reporter)

Comment 3

a year ago
The BHR histograms are stored differently from other Telemetry histograms, so the issue was just a matter of interpretation. 

Anthony fixed the extension in:
https://github.com/chutten/statuser/commit/8811418c4e57f3b285a20fe071998b258b7ac842

Fix for about:telemetry coming soon
Assignee: nobody → avihpit
Status: NEW → ASSIGNED
(Assignee)

Comment 4

a year ago
Created attachment 8711395 [details] [diff] [review]
fix-js-TimeHistogram--careful.patch

The issue here is that TimeHistogram (which is only used for hangs stats) have a different javascript representation than the rest of the histograms, as follows:

1. It includes a dummy (empty) bucket with label "0".

2. For the other buckets, it uses the upper value of the bucket as label (while the other histograms use the lower value as label).

This patch fixes the JS representation of TimeHistogram to match that of the rest of the histograms, and as a result makes the display at about:telemetry behave as expected.

The issue with this patch is that any code which uses the JS representation of TimeHistogram would be affected by this, e.g. the statuser addon, possibly the ping payloads, etc.

So while it's probably the correct approach here, it's too risky without deep investigation of where are these histograms used, followed by modifying all this code, and make sure it remains backward compatible with the "old" (current) JS representation (especially for server-side code, e.g. maybe the telemetry dashboards).

So this patch is only here for reference, and the approach I've actually taken (at the next comment/attachment) is much more conservative: keep the JS representation of Timehistogram as is, and just fix the display of those histograms at about:telemetry - which is all and only the histograms of the "Thread hangs" section.
(Assignee)

Comment 5

a year ago
Created attachment 8711398 [details] [diff] [review]
about-telemetry-bhr-hists.v2.patch

Fix the display of TimeHistograms at about:telemetry (all and only the histograms of the "Thread hangs" section).

The JS representation of those histograms remains unaffected, and as such there's no need to modify any other code which relies on this awkward representation.
Attachment #8711395 - Attachment is obsolete: true
Attachment #8711398 - Flags: review?(vladan.bugzilla)
(Reporter)

Updated

a year ago
Attachment #8711398 - Flags: review?(vladan.bugzilla) → review?(chutten)

Comment 6

a year ago
Comment on attachment 8711398 [details] [diff] [review]
about-telemetry-bhr-hists.v2.patch

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

One nit

::: toolkit/content/aboutTelemetry.js
@@ +1126,5 @@
>     * @param aParent Parent element
>     * @param aName Histogram name
>     * @param aHgram Histogram information
>     * @param aOptions Object with render options
>     *                 * exponential: bars follow logarithmic scale

Need to update the doc here to describe the new param
Attachment #8711398 - Flags: review?(chutten) → review+
(Assignee)

Comment 7

a year ago
Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f708fbffede

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f708fbffede
(Assignee)

Comment 9

a year ago
Created attachment 8711849 [details] [diff] [review]
bug1241362.v2.patch

Just updating the patch to the actual one which landed (addresses the review comment), carrying r+.

Will use this patch for uplift request to 45 once it lands on m-c.
Attachment #8711398 - Attachment is obsolete: true
Attachment #8711849 - Flags: review+

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f708fbffede
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Target Milestone: mozilla46 → mozilla47
(Assignee)

Comment 11

a year ago
Comment on attachment 8711849 [details] [diff] [review]
bug1241362.v2.patch

Approval Request Comment
[Feature/regressing bug #]:
1241362

[User impact if declined]:
Potential incorrect evaluation (interpretation) of hang histograms at about:telemetry - important for the beta 45 e10s experiment.

[Describe test coverage new/current, TreeHerder]:
None

[Risks and why]:
Very low risk - only fixes the display of hang histograms at about:telemetry.

[String/UUID change made/needed]:
None
Attachment #8711849 - Flags: approval-mozilla-beta?
Attachment #8711849 - Flags: approval-mozilla-aurora?
status-firefox45: --- → affected
Comment on attachment 8711849 [details] [diff] [review]
bug1241362.v2.patch

Taking it to improve the experiment results.
Attachment #8711849 - Flags: approval-mozilla-beta?
Attachment #8711849 - Flags: approval-mozilla-beta+
Attachment #8711849 - Flags: approval-mozilla-aurora?
Attachment #8711849 - Flags: approval-mozilla-aurora+

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5267af465053
status-firefox46: affected → fixed

Comment 14

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/885381395b74
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.