948139 - Add telemetry for switching from Metro to Desktop

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Marina Samuel [:emtwo]]

We want to see how often a user clicks the button in Metro to switch to Desktop. This is a follow up from bug 944114.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP: Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

Taking cues from BrowserUITelemetry.jsm, as far as I can tell this ought to do it. In about:telemetry I see an entry:

UITelemetry 	{"metro-appbar": {"countableEvents":{}}} 

..clicking on the view on desktop menu item, and just before we actually restart a "view-on-desktop" property should get incremented.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8349064 [details] [diff] [review]
WIP: Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

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

Looks fine to me, though I'm not really familiar with Telemetry code so I don't know whether there are any gotchas to look out for.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

:mconley, see Matt's comment above. We haven't got plans to instrument all the Metro UI right now, so I'm striking a balance between one-off code and possible future needs. This should increment aUITelemetry.metro-appbar.countableEvents.switch-to-desktop-button, which AFAICT is parallel to the switch-to-metro-button on the desktop side. 

With your f+ I'll get mbrubeck or another metro peer to review for landing.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

Quick update, fixing style in the _incrementCountableEvent method per your earlier comments on the other telemetry patch.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350404 [details] [diff] [review]
Use UITelemetry to capture clicks on the 'switch to desktop' appbar item

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

It's hard for me to give good feedback here because metro is a little bit of a mystery to me, but assuming the addSimpleMeasureFunction is in a path that is guaranteed to only run once, then this is probably OK.

::: browser/metro/base/content/appbar.js
@@ +26,5 @@
>      // tilegroup selection events for all modules get bubbled up
>      window.addEventListener("selectionchange", this, false);
> +
> +    // gather appbar telemetry data
> +    UITelemetry.addSimpleMeasureFunction("metro-appbar",

Is this guaranteed to run once?

Comment 6

[Sam Foster [:sfoster] (he/him)]

Its goes like this: 

browser.xul calls Browser.startup() from its onload handler, Browser.startup() calls singleton BrowserUI.init() 
which calls singleton ContextUI.init() which calls singleton Appbar.init();

We dont enforce singleton-ness, nor do we always guard against calling init() twice, i.e. it is *supposed* to run only once and other bad things would happen if it were called again. But I'll wrap it in a try/catch for good measure I think. 

Side-note: if UITelementy.jsm provided a hasSimpleMeasuresFunction, or a getSimpleMeasuresFunction we could guard against this case without resorting to try/catch. Was that a design decision?

Comment 7

[Sam Foster [:sfoster] (he/him)]

oh, in Metro land we only ever have a single instance of the browser chrome. Thats probably the source of the confusion here.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Sam Foster [:sfoster] from comment #7)
> oh, in Metro land we only ever have a single instance of the browser chrome.
> Thats probably the source of the confusion here.

Ah, OK, good to know. Then yes, this answers my question.

(In reply to Sam Foster [:sfoster] from comment #6)
> Side-note: if UITelementy.jsm provided a hasSimpleMeasuresFunction, or a
> getSimpleMeasuresFunction we could guard against this case without resorting
> to try/catch. Was that a design decision?

It wasn't so much a design decision as just the shortest path to getting something landed. Having a hasSimpleMeasuresFunction would be perfectly acceptable to me.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: Use UITelemetry to capture clicks on the 'switch to desktop' appbar ite

Added the try/catch to swallow the (shouldn't happen) exception that might occur if Appbar.init is run twice, per mconleys feedback above.

Comment 10

[Sam Foster [:sfoster] (he/him)]

I'm also taking the shortest path to landing here, by just swallowing the exception thrown if my simple measure is already registered. We can add hasSimpleMeasuresFunction or similar if that starts to smell bad, but its consistent with usage elsewhere.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/b3b7920dfbb9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8355364 [details] [diff] [review]
Use UITelemetry to capture clicks on the 'switch to desktop' appbar ite

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a (new feature)
User impact if declined: No visible impact to the user. The telemetry probe will help us understand and prioritize future development
Testing completed (on m-c, etc.): Tested
Risk to taking this patch (and alternatives if risky): Low-risk, metro-only patch
String or IDL/UUID changes made by this patch: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfc367d67c4a