1500066 - Add a pref for telemetry IPC accumulation timeout

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

Over in bug 1484255 I was struggling with testing event telemetry that was triggered in the content process, because the IPC sync timeout would mean that results from previous tests randomly appeared in my results in the beginning of the test.

It would be nice if folks testing event telemetry in content processes could be able to turn the pref to 0 or some other low number temporarily.

https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/toolkit/components/telemetry/core/ipc/TelemetryIPCAccumulator.cpp#42

Having a pref for this also sounds like a logical step for your team if you want to experiment with the e.g. perf impact of this setting in the future.

Comment 1

[Chris H-C :chutten]

Attachment: bug 1500066 - Control the Telemetry IPC batch timeout by pref r?Dexter

This allows tests to cut the time between accumulations being made in child
processes and them showing up in snapshots in the parent process.
For instance, setting it to 10ms instead of 2000ms takes test_ChildEvents.js
from 12s down to 10s on my local dev machine.

This patch also sets the default during telemetry xpcshell tests to 10ms.

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #1)
> This patch also sets the default during telemetry xpcshell tests to 10ms.

This might affect things more broadly, i recommend doing a broad try run on this.

Comment 3

[Chris H-C :chutten]

That was my thought as well when I scheduled this run[1] last night. I just neglected to mention it :S

Speaking of that run, there's a guaranteed crash on Win64 with this change, which seems really odd. I wouldn't have expected there to be platform differences here. debug/opt differences, maybe... but not linux/win 

Looking into it.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=207629886&revision=1fea76006d1f071012ae87b2600f1d72f7d776db

Comment 4

[Chris H-C :chutten]

The failures are from GPU process init. Apparently the Preferences service isn't ready at the time of Telemetry init (putting in a `if (Preferences::IsServiceReady())` around it fixes the crash) so that necessitates a change in approach.

I think I'll revert the InitializeGlobalState part and put the AddUintVarCache inside DoArmIPCTimer on the main thread. Initial thread arming might use the default time, but eventually we'd get the correct value.

Comment 5

[Chris H-C :chutten]

The new approach works well on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b525c0834bbe328d24be125670e0fe9952080715

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/839d553dbafb
Control the Telemetry IPC batch timeout by pref r=Dexter

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/839d553dbafb