Closed Bug 1026338 Opened 5 years ago Closed 5 years ago

Tweak the loadurl telemetry events

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Whiteboard: [mentor=mfinkle][lang=java][good first bug]
Mentor: mark.finkle
Whiteboard: [mentor=mfinkle][lang=java][good first bug] → [lang=java][good first bug]
Taking. We need this in Beta soon.
Mentor: mark.finkle
Whiteboard: [lang=java][good first bug]
Attached patch tweak-loadurlSplinter Review
Simple patch. Makes sure all LOAD_URL probes have a Method. For the ACTIONBAR method, we can have "user" and "keyword" extras to differentiate.
Assignee: nobody → mark.finkle
Attachment #8448217 - Flags: review?(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.
Attachment #8448217 - Flags: review?(liuche) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/01357a4a5326
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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
Attachment #8448217 - Flags: approval-mozilla-beta?
Attachment #8448217 - Flags: approval-mozilla-aurora?
Comment on attachment 8448217 [details] [diff] [review]
tweak-loadurl

Too late in the beta cycle.
Attachment #8448217 - Flags: approval-mozilla-beta?
Attachment #8448217 - Flags: approval-mozilla-beta-
Attachment #8448217 - Flags: approval-mozilla-aurora?
Attachment #8448217 - Flags: approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.