Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make UIMeasurements explicitly Android-only

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P4
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: Dexter, Assigned: Kate Ustiuzhanina, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
1

Firefox Tracking Flags

(firefox47 affected, firefox55 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
gfritzsche
: review+
Details | Review
(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
Blocks: 1201837
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
Mentor: gfritzsche@mozilla.com, alessio.placitelli@gmail.com
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
(Reporter)

Updated

5 months ago
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.
(Assignee)

Comment 3

5 months ago
Hi! I would like to fix this.
Thanks Kate, i'm assigning this to you.
Assignee: nobody → katejimme
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
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+

Comment 11

4 months ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/autoland/rev/ac12d71adb03
Make UIMeasurements explicitly Android-only; r=gfritzsche

Comment 12

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac12d71adb03
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.