1343729 - Ignore non-main-thread IPCs in the IPC_SYNC_LATENCY_MS probe

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(no longer active)]

Having things like the GMP IPC messages show up is confusing.  If we want to track the performance of OMT code we can add specialized probes for them later.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Only collect IPC_SYNC_LATENCY_MS on the main thread

This super short patch should make us stop collecting this telemetry off of the main thread.

I'm not sure if there is any other work I should be avoiding. The latencyMs seems to be needed for a log anyways so I didn't make computing it conditional on being on the main thread.

MozReview-Commit-ID: GtsujcVJNtW

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

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

::: build/clang-plugin/.#Utils.h
@@ +1,1 @@
> +mlayzell@skyrocket.2554:1488295635
\ No newline at end of file

Oops.

::: ipc/glue/MessageChannel.cpp
@@ +1284,1 @@
>        Telemetry::Accumulate(Telemetry::IPC_SYNC_LATENCY_MS,

We should rename this probe.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bill McCloskey (:billm) from comment #2)
> ::: ipc/glue/MessageChannel.cpp
> @@ +1284,1 @@
> >        Telemetry::Accumulate(Telemetry::IPC_SYNC_LATENCY_MS,
> 
> We should rename this probe.

If we're going to rename the probe we should do bug 1337073 at the same time. I'll hold off on landing this until we figure out what we want the new parameters to be.

Comment 4

[(no longer active)]

Yeah let's rename this once!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbcd60ba4e7
Only collect IPC_SYNC_LATENCY_MS on the main thread, r=billm

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Less useful sync IPC telemetry on beta/aurora
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: bug 1333489 must be uplifted to beta. Also depends on bug 1337073.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Renames and updates parameters for a telemetry probe.
[String changes made/needed]: None

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

Improvement for sync telemetry, let's take this for aurora + beta 2. 
On beta, we need the patch from bug 1333489 first.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0dc9b3b87501

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5091aed32a48

Comment 10

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 6).

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bbcd60ba4e7