1312778 - High volume spike in Fx52 for FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

The evolution for FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS went way up with the Fx 52 of bug 1218576:
https://mzl.la/2eIdu0E

Comment 1

[Chris H-C :chutten]

This may or may not be exactly bug 1304730. Investigating...

Comment 2

[Chris H-C :chutten]

Attachment: bug 1312778 - Ensure histogram recording is enabled before remote accumulation

There is a mechanism to selectively turn hgram recording off and on. This is
presently only used to throttle an otherwise-devastatingly-chatty hgram to only
contain accumulations from specific times (sync scroll).

This was only checked on the parent. This checks it on the child as well.

Review commit: https://reviewboard.mozilla.org/r/88662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88662/

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8805158 [details]
bug 1312778 - Ensure histogram recording is enabled before remote accumulation

https://reviewboard.mozilla.org/r/88662/#review88352

Good find.
We have test coverage for the parent side in test_nsITelemetry.js.
Can we add test coverage for the child too in test_ChildHistograms.js?

Comment 4

[Chris H-C :chutten]

Comment on attachment 8805158 [details]
bug 1312778 - Ensure histogram recording is enabled before remote accumulation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88662/diff/1-2/

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8805158 [details]
bug 1312778 - Ensure histogram recording is enabled before remote accumulation

https://reviewboard.mozilla.org/r/88662/#review88814

Thanks.

::: toolkit/components/telemetry/tests/unit/test_ChildHistograms.js:31
(Diff revision 2)
> +  Telemetry.setHistogramRecordingEnabled("TELEMETRY_TEST_KEYED_COUNT", false);
> +  countKeyed.add("a");
> +  countKeyed.add("b");
> +  Telemetry.setHistogramRecordingEnabled("TELEMETRY_TEST_KEYED_COUNT", true);

Keyed histograms have slightly different code paths than non-keyed ones.
Lets add the same pattern for a non-keyed histogram to have good coverage.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8805158 [details]
bug 1312778 - Ensure histogram recording is enabled before remote accumulation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88662/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee32f39572dd
Ensure histogram recording is enabled before remote accumulation r=gfritzsche

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/autoland/rev/d0f5bb427e0f2d7b0dbc68df53ccca5e151ff16d because it conflicted with bug 1304494, causing build bustage when that other bug got merged to autoland:

https://treeherder.mozilla.org/logviewer.html#?job_id=5917817&repo=autoland

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Via ryanvm on irc:
09:25:54 <RyanVM> oh I see
09:26:00 <RyanVM> dvander's patch changed the function signature
09:26:05 <RyanVM> chutten added a new call on autoland
09:26:49 <RyanVM> in the first hunk of https://hg.mozilla.org/integration/autoland/rev/ee32f39572dd76c9b6db98831d61b54c668147f5
09:27:04 <RyanVM> add a |, GeckoProcessType_Default| to that internal_GetHistogramByEnumId call

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/443c4ea33338
Ensure histogram recording is enabled before remote accumulation r=gfritzsche

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/443c4ea33338

Comment 13

[Chris H-C :chutten]

Submission volumes for the probe have returned to pre-bug 1218576 levels https://mzl.la/2fQpQb2