1061409 - Telemetry for initial share overlay release

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Richard Newman [:rnewman]]

Capture:

* Activity displayed
* … and canceled
* Adding a bookmark (new method)
* Displaying remote devices and picking one
  * … or canceling
* Adding to reading list

* Title present or not?
* Favicon known or not?

We'll want this to follow the share overlay into release.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Bolt some telemetry to the overlays stuff.

There will now be a brief, frantic interlude in which I add favicon support to the share overlays.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Telemetry for share overlays

Comment 3

[Chris Kitching [:ckitching]]

Note that the favicon-present telemetry update is added in the patch in Bug 1064031 (along with the actual favicon support).

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8485497 [details] [diff] [review]
Telemetry for share overlays

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

Can you explain why you used 'regular' telemetry versus UITelemetry?

::: toolkit/components/telemetry/Histograms.json
@@ +3115,5 @@
>    },
> +  "FENNEC_OVERLAY_EVENTS": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 5,

5 sure looks smaller than 10 to me.

It's also good practice to pad this to allow for future expansion.

Comment 5

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 8485497 [details] [diff] [review]
> Telemetry for share overlays
> 
> Review of attachment 8485497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you explain why you used 'regular' telemetry versus UITelemetry?

Because I've never done this before, I have no idea what I'm doing, and there doesn't seem to be any documentation handy?

> ::: toolkit/components/telemetry/Histograms.json
> @@ +3115,5 @@
> >    },
> > +  "FENNEC_OVERLAY_EVENTS": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 5,
> 
> 5 sure looks smaller than 10 to me.
> 
> It's also good practice to pad this to allow for future expansion.

Whoops. That's not a compile error? Doesn't this json get used to spit out C++ code?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #5)

> > Can you explain why you used 'regular' telemetry versus UITelemetry?
> 
> Because I've never done this before, I have no idea what I'm doing, and
> there doesn't seem to be any documentation handy?

You should discuss the advantages and drawbacks with Chenxia (and Margaret, if she's online when you are), and figure out what kinds of questions each one can answer.


> Whoops. That's not a compile error? Doesn't this json get used to spit out
> C++ code?

¯\_(ツ)_/¯

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8485497 [details] [diff] [review]
Telemetry for share overlays

Clearing review flag pending design discussions with SFOites.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Telemetry for share overlays

Comment 9

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

Comment on attachment 8487617 [details] [diff] [review]
Telemetry for share overlays

Review ping

Comment 10

[Chris Kitching [:ckitching]]

For once I was perhaps too terse: I poked the appropriate SFO people and rejigged it to use UITelemetry.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8487617 [details] [diff] [review]
Telemetry for share overlays

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

So our actions should be:

  save.1, share.1, action.1, cancel.1

and our method will always be
 
  shareoverlay

and our events will be

  choose.1, sendtab.picker.1, sendtab.send.1 readinglist.1, bookmark.1

We'll use extras on 'choose' to indicate whether the request came with a URL, title, or both.

::: mobile/android/base/TelemetryContract.java
@@ +158,5 @@
>          // Note: Only used in JavaScript for now, but here for completeness.
>          PAGEACTION("pageaction"),
>  
> +        // Actions triggered from the share overlay.
> +        SHARE_OVERLAY("shareoverlay"),

Where my docs at?

mobile/android/base/docs/index.rst

::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java
@@ +103,5 @@
>          if (currentState == State.SHOW_DEVICES) {
>              row.setOnClickListener(new OnClickListener() {
>                  @Override
>                  public void onClick(View view) {
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "device_selection");

"sendtab.picker.1"

::: mobile/android/base/overlays/ui/SendTabList.java
@@ +156,5 @@
> +        });
> +        builder.setOnCancelListener(new DialogInterface.OnCancelListener() {
> +            @Override
> +            public void onCancel(DialogInterface dialogInterface) {
> +                Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY, "device_selection_cancel");

"sendtab.picker.1"

The "cancel" part comes from the Event.CANCEL.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +109,5 @@
>  
>              return;
>          }
>  
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "opened_with_url");

This doesn't fit with UI telemetry except as an extra. So use an extra for it, attaching it to the single UI telemetry event for Event.ACTION, SHARE_OVERLAY, "choose.1".

@@ +128,5 @@
>          // TODO: Consider polling Fennec databases to find better information to display.
>          String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT);
>          if (subjectText != null) {
>              ((TextView) findViewById(R.id.title)).setText(subjectText);
> +            Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "opened_with_title");

Same.

@@ +224,5 @@
>  
>          startService(serviceIntent);
>          slideOut();
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "sendtab");

Event.SHARE, sendtab.send.1

@@ +235,5 @@
>  
>      public void addToReadingList() {
>          startService(getServiceIntent(ShareMethod.Type.ADD_TO_READING_LIST));
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "reading_list");

Probably this should be Event.SAVE. Modify the docs for "save.1" to no longer say it's only used from JS.

readinglist.1

@@ +241,5 @@
>  
>      public void addBookmark() {
>          startService(getServiceIntent(ShareMethod.Type.ADD_BOOKMARK));
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.SHARE_OVERLAY, "bookmark");

Event.SAVE, bookmark.1

@@ +280,5 @@
>       */
>      @Override
>      public void onBackPressed() {
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY, "overlay_cancel");

"choose.1"

@@ +289,5 @@
>       */
>      @Override
>      public boolean onTouchEvent(MotionEvent event) {
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY, "overlay_cancel");

"choose.1"

Comment 12

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

(In reply to Richard Newman [:rnewman] from comment #11)

> and our events will be
> 
>   choose.1, sendtab.picker.1, sendtab.send.1 readinglist.1, bookmark.1
> 

Just for the record, our Extras (not Events in this case) are not versioned anywhere else.

Comment 13

[Richard Newman [:rnewman]]

Oh, you're right, these are extras. Piss.

In that case, I think we want a "SHOW" event to mirror "CANCEL", and let's not bother instrumenting the multi-device picker:

   SHOW .. shareoverlay,title=true,url=true
   CANCEL .. shareoverlay
   ACTION .. sendtab.choose         -- no cancel, don't care
   SAVE .. bookmark, readinglist
   SHARE .. sendtab

Does that make sense?

Comment 14

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

(In reply to Richard Newman [:rnewman] from comment #13)
> Oh, you're right, these are extras. Piss.
> 
> In that case, I think we want a "SHOW" event to mirror "CANCEL", and let's
> not bother instrumenting the multi-device picker:
> 
>    SHOW .. shareoverlay,title=true,url=true
>    CANCEL .. shareoverlay
>    ACTION .. sendtab.choose         -- no cancel, don't care
>    SAVE .. bookmark, readinglist
>    SHARE .. sendtab
> 
> Does that make sense?

Seems reasonable

Comment 15

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #13)
> Oh, you're right, these are extras. Piss.
> 
> In that case, I think we want a "SHOW" event to mirror "CANCEL", and let's
> not bother instrumenting the multi-device picker:
> 
>    SHOW .. shareoverlay,title=true,url=true

Shouldn't that be ACTION instead of SHOW, or do you want me to add a "SHOW" just for the overlay?

>    CANCEL .. shareoverlay
>    ACTION .. sendtab.choose         -- no cancel, don't care
>    SAVE .. bookmark, readinglist

Is this going to be sufficiently obviously a save from the share overlay as opposed to from somewhere else?

>    SHARE .. sendtab
> 
> Does that make sense?

Comment 16

[Chris Kitching [:ckitching]]

Attachment: Telemetry for share overlays

Third time lucky?

Comment 17

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #15)
 
> >    SHOW .. shareoverlay,title=true,url=true
> 
> Shouldn't that be ACTION instead of SHOW, or do you want me to add a "SHOW"
> just for the overlay?

I was thinking the latter. We have three fields, which vaguely correspond to verb, object, ad{jective,verb}.

"Action" is pretty vague, so given that you do various things with the share overlay, it'd be nice to avoid having to look at the extras in order to figure out what you did.

The alternative is that we use a session instead of SHOW/CANCEL, but using a session doesn't allow us to attach the two boolean extras.


> >    SAVE .. bookmark, readinglist
> 
> Is this going to be sufficiently obviously a save from the share overlay as
> opposed to from somewhere else?

That's what the method (which I elided as "..") is for.

It'd be

  Telemetry.sendUIEvent(TelemetryContract.Event.SAVE,              // I saved...
                        TelemetryContract.Method.SHARE_OVERLAY,    // ... via the overlay
                        "bookmark");                               // ... a bookmark.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8500064 [details] [diff] [review]
Telemetry for share overlays

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

::: mobile/android/base/TelemetryContract.java
@@ +79,5 @@
>          // Sharing content.
>          SHARE("share.1"),
>  
> +        // Show a UI element.
> +        SHOW("show.1"),

Remember to add to the docs.

::: mobile/android/base/overlays/ui/SendTabList.java
@@ +158,5 @@
> +            @Override
> +            public void onCancel(DialogInterface dialogInterface) {
> +                Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY, "device_selection_cancel");
> +            }
> +        });

y u no chain?

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +144,5 @@
>  
>          // If provided, we use the subject text to give us something nice to display.
>          // If not, we wing it with the URL.
> +
> +        String telemetryExtras = "url=true";

If the URL is missing, we should *record an event*.

Decide if this should be "showed with no URL" or "rejected". I think the latter, so we can omit this.

@@ +149,5 @@
>          // TODO: Consider polling Fennec databases to find better information to display.
>          String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT);
>          if (subjectText != null) {
>              ((TextView) findViewById(R.id.title)).setText(subjectText);
> +            telemetryExtras += ",title=true";

final String extras = "title=" + (subjectText != null);

@@ +308,5 @@
>  
>          startService(serviceIntent);
>          slideOut();
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.SEND_TAB);

See my comment; our pattern has been to use simple extras for this, e.g.,

  sendUIEvent(TelemetryContract.Event.SANITIZE, TelemetryContract.Method.BUTTON, "history")

so this should be

  Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.SHARE_OVERLAY, "sendtab");

@@ +319,5 @@
>  
>      public void addToReadingList() {
>          startService(getServiceIntent(ShareMethod.Type.ADD_TO_READING_LIST));
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.READING_LIST);

same

@@ +325,5 @@
>  
>      public void addBookmark() {
>          startService(getServiceIntent(ShareMethod.Type.ADD_BOOKMARK));
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.BOOKMARK);

same

@@ +368,5 @@
>       */
>      @Override
>      public void onBackPressed() {
>          slideOut();
> +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY);

same

Comment 19

[Richard Newman [:rnewman]]

Status update, Chris?

Comment 20

[Chris Kitching [:ckitching]]

Attachment: Telemetry for share overlays

Comment 21

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #18)
> ::: mobile/android/base/TelemetryContract.java
> @@ +79,5 @@
> >          // Sharing content.
> >          SHARE("share.1"),
> >  
> > +        // Show a UI element.
> > +        SHOW("show.1"),
> 
> Remember to add to the docs.

This is the first time in two years someone has decided to say this to me.

> ::: mobile/android/base/overlays/ui/SendTabList.java
> @@ +158,5 @@
> > +            @Override
> > +            public void onCancel(DialogInterface dialogInterface) {
> > +                Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY, "device_selection_cancel");
> > +            }
> > +        });
> 
> y u no chain?

Because I've still not figured out how to get IDEA to automatically indent that how I like and couldn't be bothered to fix it by hand.

It has now been made "beautiful". Someday I'll write an extension for that... :P

... I also have a horrible suspicion this might make the bytecode smaller. :/

> ::: mobile/android/base/overlays/ui/ShareDialog.java
> @@ +144,5 @@
> >  
> >          // If provided, we use the subject text to give us something nice to display.
> >          // If not, we wing it with the URL.
> > +
> > +        String telemetryExtras = "url=true";
> 
> If the URL is missing, we should *record an event*.

I omitted this because it can be inferred from the other events we're sending in.

I'll add an explicit one for this, then...

> Decide if this should be "showed with no URL" or "rejected". I think the
> latter, so we can omit this.
> 
> @@ +149,5 @@
> >          // TODO: Consider polling Fennec databases to find better information to display.
> >          String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT);
> >          if (subjectText != null) {
> >              ((TextView) findViewById(R.id.title)).setText(subjectText);
> > +            telemetryExtras += ",title=true";
> 
> final String extras = "title=" + (subjectText != null);

Fail. Particularly since what I wrote will compile to a slightly larger nightmare of StringBuilder.

> @@ +368,5 @@
> >       */
> >      @Override
> >      public void onBackPressed() {
> >          slideOut();
> > +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.SHARE_OVERLAY);
> 
> same

Wait, why does this one need an extra? We're doing "cancel" to "share_overlay". That's all the information needed, right? Unclear how to rephrase this? (CANCEL, SHARE_OVERLAY, "share_overlay") or something doesn't seem to make much sense.


... It's amusing how long this is taking. Did I miss some documentation somewhere, or am I just *that* thick? :P

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8515414 [details] [diff] [review]
Telemetry for share overlays

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

LGTM.

::: mobile/android/base/overlays/ui/SendTabList.java
@@ +140,5 @@
>          }
>  
>          builder.setTitle(R.string.overlay_share_select_device)
>                 .setItems(dialogElements, new DialogInterface.OnClickListener() {
> +                    public void onClick(DialogInterface dialog, int index) {

Come back, Mr @Override. I miss you.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +153,4 @@
>          // TODO: Consider polling Fennec databases to find better information to display.
>          String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT);
> +
> +        String telemetryExtras = "url=true,title=" + (subjectText != null);

Omit the url=true.

Comment 23

[Chris Kitching [:ckitching]]

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/330405bbbc9a