1007095 - Add UI telemetry for Reader actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

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

Attachment: uitelemetry-readeractions v0.1

Add some probes for the Reader toolbar actions.

I used "action.1" since this seemed very similar to the App mainmenu and Home contextmenu probes. The button IDs are passed as etras, just like the mainmenu R.id.xxx values are passed as extras.

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 8418711 [details] [diff] [review]
uitelemetry-readeractions v0.1

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

Giving f+ to get question answered before r+.

::: mobile/android/chrome/content/aboutReader.js
@@ +707,5 @@
> +        // Create a relative timestamp for telemetry
> +        let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;
> +        // Just pass the ID of the button as an extra and hope the ID doesn't change
> +        // unless the context changes
> +        UITelemetry.addEvent("action.1", "button", uptime, id);

Where is the id coming from? I don't see it being defined anywhere.

Maybe this uptime argument should simply default to (Date.now() - Services.startup.getStartupInfo().linkerInitialized)?

Comment 2

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

(In reply to Lucas Rocha (:lucasr) from comment #1)
> Comment on attachment 8418711 [details] [diff] [review]
> uitelemetry-readeractions v0.1
> 
> Review of attachment 8418711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Giving f+ to get question answered before r+.
> 
> ::: mobile/android/chrome/content/aboutReader.js
> @@ +707,5 @@
> > +        // Create a relative timestamp for telemetry
> > +        let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;
> > +        // Just pass the ID of the button as an extra and hope the ID doesn't change
> > +        // unless the context changes
> > +        UITelemetry.addEvent("action.1", "button", uptime, id);
> 
> Where is the id coming from? I don't see it being defined anywhere.

It's passed into the function:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#677

> Maybe this uptime argument should simply default to (Date.now() -
> Services.startup.getStartupInfo().linkerInitialized)?

I was going to file a bug to explore that. The code pattern is duplicated a lot and will only keep happening.

Comment 3

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

Note that we also have a "reader.1" Session active when these "action" Events are fired, so we have more context about them.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8418711 [details] [diff] [review]
uitelemetry-readeractions v0.1

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

(In reply to Mark Finkle (:mfinkle) from comment #2)
> (In reply to Lucas Rocha (:lucasr) from comment #1)
> It's passed into the function:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutReader.js#677

Ah, ok.

> > Maybe this uptime argument should simply default to (Date.now() -
> > Services.startup.getStartupInfo().linkerInitialized)?
> 
> I was going to file a bug to explore that. The code pattern is duplicated a
> lot and will only keep happening.

Please file a follow-up.

Comment 5

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

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

Filed bug 1007647 for the timestamp issue.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

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

Comment on attachment 8418711 [details] [diff] [review]
uitelemetry-readeractions v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We need more before and after telemetry data
Testing completed (on m-c, etc.): Working fine on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f6a7121a73c