1026338 - Tweak the loadurl telemetry events

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

1. Add methods to all the events. Some events, like reading list, history and bookmarks send NULL, but should send LIST_ITEM
2. Add extras for "private" URLs

Comment 1

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

Taking. We need this in Beta soon.

Comment 2

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

Attachment: tweak-loadurl

Simple patch. Makes sure all LOAD_URL probes have a Method. For the ACTIONBAR method, we can have "user" and "keyword" extras to differentiate.

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8448217 [details] [diff] [review]
tweak-loadurl

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

Do we also want to remove the Event only sendUIEvent from Telemetry? That would enforce requiring a Method for UI Telemetry, if you think that in the future we should always require Method. (Remember to update testUITelemetry as well, if you do decide to do that!)

::: mobile/android/base/home/BookmarksListView.java
@@ +92,5 @@
>          } else {
>              // Otherwise, just open the URL
>              final String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
>  
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);

I've noticed that we don't really use sessions in the dashboard, so this event would show up exactly the same as if the user tapped on a frecency item (in BrowserSearch - https://hg.mozilla.org/integration/fx-team/file/6fe53d58d0ae/mobile/android/base/home/BrowserSearch.java#l284 )

I don't mind this if we're going to integrate sessions into the dashboard/analysis, but otherwise, I'd like to see an extra to differentiate these two cases. I'd prefer sessions, but maybe we could use "bookmark" and "search."

::: mobile/android/base/home/HistoryPanel.java
@@ +97,5 @@
>                  position -= mAdapter.getMostRecentSectionsCountBefore(position);
>                  final Cursor c = mAdapter.getCursor(position);
>                  final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
>  
> +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);

Yeah, I think I'd like to see sessions in the dashboard, rather than more extras. Out of the scope of this bug, then.

Comment 4

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

(In reply to Chenxia Liu [:liuche] from comment #3)
> Comment on attachment 8448217 [details] [diff] [review]
> tweak-loadurl
> 
> Review of attachment 8448217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we also want to remove the Event only sendUIEvent from Telemetry? That
> would enforce requiring a Method for UI Telemetry, if you think that in the
> future we should always require Method. (Remember to update testUITelemetry
> as well, if you do decide to do that!)

I think I'll hold off for a new bug.

> I've noticed that we don't really use sessions in the dashboard, so this
> event would show up exactly the same as if the user tapped on a frecency
> item (in BrowserSearch -
> https://hg.mozilla.org/integration/fx-team/file/6fe53d58d0ae/mobile/android/
> base/home/BrowserSearch.java#l284 )
> 
> I don't mind this if we're going to integrate sessions into the
> dashboard/analysis, but otherwise, I'd like to see an extra to differentiate
> these two cases. I'd prefer sessions, but maybe we could use "bookmark" and
> "search."

The dashboard has "Panel" in the LOAD_URL tab (and others) which shows the active Home panel when the event was triggered, so we can use that.

Comment 5

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

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/01357a4a5326

Comment 7

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

Comment on attachment 8448217 [details] [diff] [review]
tweak-loadurl

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: We get more detailed telemetry
[Describe test coverage new/current, TBPL]: landed on m-c and is reporting correctly
[Risks and why]: low, just tweaks existing probes
[String/UUID change made/needed]: none

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8448217 [details] [diff] [review]
tweak-loadurl

Too late in the beta cycle.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 10

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

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