Closed Bug 1252066 Opened 5 years ago Closed 4 years ago

Make UIMeasurements explicitly Android-only

Categories

(Toolkit :: Telemetry, defect, P4)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox47 --- affected
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: katejimme, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 file, 1 obsolete file)

59 bytes, text/x-review-board-request
gfritzsche
: review+
Details
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
Blocks: 1201837
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
Mentor: gfritzsche, alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Priority: P3 → P4
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
(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.
Hi! I would like to fix this.
Thanks Kate, i'm assigning this to you.
Assignee: nobody → katejimme
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 ...`_).
Attachment #8844157 - Flags: review?(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.
Attachment #8845078 - Flags: review?(gfritzsche)
Attachment #8845078 - Attachment is obsolete: true
Comment on attachment 8844157 [details]
Bug 1252066 - Make UIMeasurements explicitly Android-only;

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

Thanks, this looks good!
Attachment #8844157 - Flags: review?(gfritzsche) → review+
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/autoland/rev/ac12d71adb03
Make UIMeasurements explicitly Android-only; r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/ac12d71adb03
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.