Closed Bug 1007095 Opened 5 years ago Closed 5 years ago

Add UI telemetry for Reader actions

Categories

(Firefox for Android :: Reader View, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: mfinkle, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #8418711 - Flags: review?(lucasr.at.mozilla)
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)?
Attachment #8418711 - Flags: review?(lucasr.at.mozilla) → feedback+
(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.
Note that we also have a "reader.1" Session active when these "action" Events are fired, so we have more context about them.
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.
Attachment #8418711 - Flags: feedback+ → review+
https://hg.mozilla.org/mozilla-central/rev/e0e28f569c29
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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
Attachment #8418711 - Flags: approval-mozilla-aurora?
Attachment #8418711 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.