1246973 - Support clearing UI Telemetry when creating pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: clear-uitelemetry v0.1

Currently, UI Telemetry is never cleared when the Telemetry service creates a ping. That means we are posting duplicate events in subsequent pings.

This patch adds basic support for clearing measurements when retrieved for ping generation. We still need to enable unified telemetry mode to request the data be cleared. That is a different bug.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Note that this currently won't clear UIMeasurements on Android (Unified Telemetry is not enabled there and clearSubsession will never be true).
We can't always reset UIMeasurements either because about:telemetry uses this code path too and we don't want to reset in this case.

Comment 2

[Georg Fritzsche [:gfritzsche]]

A possible short-term solution:
TelemetrySesssionImpl.assemblePayloadWithMeasurements() gets |reason| as a parameter.
For about:telemetry collection, |reason| will be one of REASON_GATHER_PAYLOAD or REASON_GATHER_SUBSESSION_PAYLOAD.
So we can reset UIMeasurements on Android whenever |reason| is neither of those.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: clear-uitelemetry v0.2

This version of the patch adds a Android specific clearing flag. Review to Georg, but feedback to Margaret and Blake since this affects there platforms.

Well, I coded the patch to _not_ affect Blake's platform, but maybe he wants me to make it clear on Desktop too.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8717562 [details] [diff] [review]
clear-uitelemetry v0.2

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

This looks good for me.
I don't know who the users of the mobile UIMeasurements are, so just calling it out that this may affect others.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8717562 [details] [diff] [review]
clear-uitelemetry v0.2

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

LGTM. I don't know why desktop wouldn't want to clear these as well, but I can let bwinton answer that.

::: mobile/android/tests/browser/robocop/testUITelemetry.js
@@ +93,2 @@
>    let obs = getObserver();
> +  let measurements = removeNonTestMeasurements(obs.getUIMeasurements(clearMeasurements));

Wow, we have a robocop test for UI telemetry. Go us.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to :Margaret Leibovic from comment #5)
> LGTM. I don't know why desktop wouldn't want to clear these as well, but I
> can let bwinton answer that.

For desktop we currently send two types of pings:
* "saved-session" (whole-session measurements, powering t.m.o until that is migrated)
* "main" (subsession measurements)

If we already look at UITelemetry from "main" pings, we should clear it, otherwise leave it for now.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Ilana (or her intern, or maybe RRayborn) is the main consumer of the UITelemetry for Desktop, so let's check with them…

(The question is: For the UITelemetry Dashboard, do we look at "main" pings, or "saved-session" pings?)

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(I suspect we want to clear them for desktop as well, but would like to double-check.)

Comment 9

[Ilana]

https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/spark.py#L314 makes me think that saved_session pings are still the ones we plan on using, but rvitillo can verify.

Comment 10

[Benjamin Smedberg]

As far as I know our plan is to get rid of saved_session pings entirely, once porting to main pings is complete.

Georg, is this for desktop or mobile? I figured we'd either get rid of UITelemetry entirely on desktop, since nobody is using it currently, or move it into its own ping.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8717562 [details] [diff] [review]
clear-uitelemetry v0.2

Seems good, but we also want it for desktop.  More comments coming.

Comment 12

[Ilana]

Yes, we need this info in desktop (not important to me where it is, but we need it in the ping).

Comment 13

[Blake Winton (:bwinton) (:☕️)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> As far as I know our plan is to get rid of saved_session pings entirely,
> once porting to main pings is complete.

In that case, we'll want to switch to main pings, and we'll want them cleared on desktop so that we don't double-count things.

> Georg, is this for desktop or mobile? I figured we'd either get rid of
> UITelemetry entirely on desktop, since nobody is using it currently, or move
> it into its own ping.

That comes as a huge surprise (and reasonably large disappointment) to me.  We on the UX team are frequently trying to use it, but haven't had the resources to fix the dashboard until recently.  (We have also run several custom reports using the data recently, so I'm not sure where you got the idea that we're not using it…)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e705022687ad

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e705022687ad

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Pulsebot from comment #14)
> https://hg.mozilla.org/integration/fx-team/rev/e705022687ad

Hm, this landed as clearing it out on all platforms, not just Android, without comment 7 ff. being cleared up completely.

(In reply to Blake Winton (:bwinton) (:☕️) from comment #7)
> Ilana (or her intern, or maybe RRayborn) is the main consumer of the
> UITelemetry for Desktop, so let's check with them…
> 
> (The question is: For the UITelemetry Dashboard, do we look at "main" pings,
> or "saved-session" pings?)

What is the UITelemetry dashboard powered from now, presumably "saved-session"?
What aggregation code is it powered from and who knows about it?

Comment 17

[Georg Fritzsche [:gfritzsche]]

And are you looking at payload.simpleMeasurements.UITelemetry or payload.UIMeasurements?

Comment 18

[Blake Winton (:bwinton) (:☕️)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> (In reply to Pulsebot from comment #14)
> > https://hg.mozilla.org/integration/fx-team/rev/e705022687ad
> Hm, this landed as clearing it out on all platforms, not just Android,
> without comment 7 ff. being cleared up completely.

mfinkle pinged me about it on IRC before landing, and we agreed based on chats with ilana that clearing it out on all platforms was the right thing to do.

> What is the UITelemetry dashboard powered from now, presumably
> "saved-session"?

Saved session I believe, as per https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/spark.py#L332 (and comment 9).

> What aggregation code is it powered from and who knows about it?

https://github.com/mozilla/user-advocacy and rrayborn (and ilana and to a lesser extent me).

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> And are you looking at payload.simpleMeasurements.UITelemetry or
> payload.UIMeasurements?

payload.simpleMeasurements.UITelemetry although switching to payload.UIMeasurements sounds like it might be a good idea when we switch to main pings…

Comment 19

[Georg Fritzsche [:gfritzsche]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #18)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> > (In reply to Pulsebot from comment #14)
> > > https://hg.mozilla.org/integration/fx-team/rev/e705022687ad
> > Hm, this landed as clearing it out on all platforms, not just Android,
> > without comment 7 ff. being cleared up completely.
> 
> mfinkle pinged me about it on IRC before landing, and we agreed based on
> chats with ilana that clearing it out on all platforms was the right thing
> to do.

The problem here is that Fx Desktop behaves different from Android (which has a quirky ping collection right now).

> > What is the UITelemetry dashboard powered from now, presumably
> > "saved-session"?
> 
> Saved session I believe, as per
> https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/
> spark.py#L332 (and comment 9).

Right, i was asking again because i don't see any UITelemetry handling there.
But we can talk about this elsewhere :)

> (In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> > And are you looking at payload.simpleMeasurements.UITelemetry or
> > payload.UIMeasurements?
> 
> payload.simpleMeasurements.UITelemetry although switching to
> payload.UIMeasurements sounds like it might be a good idea when we switch to
> main pings…

Ah, great, then things keep working for you as before (as payload.simpleMeasurements.UITelemetry is not affected by this patch).
UIMeasurements will not behave properly for desktop Fx analysis until you switch to using "main" pings.
If you are moving to "main" pings and keep using UITelemetry, we would need to properly clear that info too.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Just following up here. I ran a script to look at the payload sizes of various parts of the ping. Here is the _median_ UIMeasurement section sizes and the number of pings, before and after this patch:

Before:

 ('payload/UIMeasurements', 1032.0, 12044),

After:

 ('payload/UIMeasurements', 200.0, 6103),

So we dropped from ~1KB to ~200bytes, which is nice. Sadly, it won't effect the overall size of the ping very much. The median ping size is ~98KB.

I don't think we really need to uplift this patch since it doesn't make a dramatic difference.