1252066 - Make UIMeasurements explicitly Android-only

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Alessio Placitelli [:Dexter]]

It looks like UIMeasurements is only filled with Reader Mode data [1][2], which is only really considered for Fennec.

We should consider making the UIMeasurements section of the main-ping Android-only until anyone actually uses it on Desktop.

[1] - https://dxr.mozilla.org/mozilla-central/search?q=UITelemetry.addEvent%28&redirect=false&case=true
[2] - https://dxr.mozilla.org/mozilla-central/rev/5e0140b6d11821e0c2a2de25bc5431783f03380a/mobile/android/chrome/content/Reader.js#212

Comment 1

[Georg Fritzsche [:gfritzsche]]

This bug requires:
- changing the ping construction (in TelemetrySession) to only submit this data on the "android" platform
- clarifying documentation where needed
- running the unit tests and fixing any fallout

Code search:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2F+UIMeasurements&redirect=false

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> - running the unit tests and fixing any fallout

Most importantly running & fixing them on Firefox Desktop builds. Android builds should continue to work the same after this.

Comment 3

[Kate Ustiuzhanina]

Hi! I would like to fix this.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks Kate, i'm assigning this to you.

Comment 5

[Kate Ustiuzhanina]

Attachment: Bug 1252066 - Make UIMeasurements explicitly Android-only;

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

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8844157 [details]
Bug 1252066 - Make UIMeasurements explicitly Android-only;

https://reviewboard.mozilla.org/r/117692/#review119992

Thanks, this looks good with the comments below fixed.
I pushed this to "try" to see that the tests run through fine.

::: toolkit/components/telemetry/TelemetrySession.jsm:1350
(Diff revision 1)
> -      payloadObj.UIMeasurements = protect(() => UITelemetry.getUIMeasurements(clearUIsession));
>  
> +      if (AppConstants.platform == "android") {
> +        payloadObj.UIMeasurements = protect(() => UITelemetry.getUIMeasurements(clearUIsession));
> +      }
> +      

There is trailing whitespace here, please remove it.

::: toolkit/components/telemetry/docs/data/main-ping.rst:73
(Diff revision 1)
>        gc: {...},
>        fileIOReports: {...},
>        lateWrites: {...},
>        addonDetails: {...},
>        addonHistograms: {...},
>        UIMeasurements: [...],

Lets add a comment behind this:
// Android only.

::: toolkit/components/telemetry/docs/data/main-ping.rst:674
(Diff revision 1)
>  --------------
>  This section contains the slow SQL statements gathered at startup (until the "sessionstore-windows-restored" event is fired). The structure of this section resembles the one for `slowSQL`_.
>  
>  UIMeasurements
>  --------------
> -This section contains UI specific telemetry measurements and events. This section is mainly populated with Android-specific data and events (`see here <https://dxr.mozilla.org/mozilla-central/search?q=regexp%3AUITelemetry.%28addEvent|startSession|stopSession%29&redirect=false&case=false>`_).
> +This section contains UI specific telemetry measurements and events. This section is mainly populated with Android-specific data and events (`see here <https://dxr.mozilla.org/mozilla-central/search?q=regexp%3AUITelemetry.%28addEvent|startSession|stopSession%29&redirect=false&case=false>`_) and does not apply to other platforms.

How about changing this to:
This section is Android-only and contains UI specific Telemetry measurements and events (`see here ...`_).

Comment 7

[Kate Ustiuzhanina]

Attachment: Bug 1252066 - Fix docs;

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8845078 [details]
Bug 1252066 - Fix docs;

https://reviewboard.mozilla.org/r/118294/#review120484

Thanks! This looks good for the changes, but ended up in two commits (sorry about misleading here).
Lets get this into one commit, probably a new MozReview request is easiest here.

Comment 9

[Kate Ustiuzhanina]

Comment on attachment 8844157 [details]
Bug 1252066 - Make UIMeasurements explicitly Android-only;

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

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8844157 [details]
Bug 1252066 - Make UIMeasurements explicitly Android-only;

https://reviewboard.mozilla.org/r/117692/#review120982

Thanks, this looks good!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac12d71adb03
Make UIMeasurements explicitly Android-only; r=gfritzsche

Comment 12

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

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