Trello card: https://trello.com/c/bSZAO6XY/129-highlights-as-a-user-i-want-to-manage-my-higlights-items-so-i-have-control-over-what-is-being-shown We want to track user interactions with action items we present for highlights.

Correction: we want to track user interactions with all action items - for Top Sites, for Highlights, etc.

While tracking interactions, we need to include type of the item for which action menu is being displayed. For example, we want to distinguish between dismissals of highlights and top sites.

We need to distinguish between context menus for top sites vs. menus for highlights. "Method" field seems like a decent way to differentiate between the two. We already have "contextmenu" method which is currently used by all home panels. I suggest we add two more: "as_topsite_contextmenu" and "as_highlight_contextmenu". Additionally, we might want to track extra information, such as position of an item that brought up a menu, or global/item's sync status (see Bug 1318551, Bug 1318550, if we can get this data easily enough). We have an "extras" field, which has been normally used to store simple string titles (a name of a menu item clicked, for example). Instead, since we're querying against a presto db, we should be able to make use of its JSON functions - https://prestodb.io/docs/current/functions/json.html For example, consider this event: - user with sync enabled clicks on a third highlight's (whose origin is 'bookmarked' and it's not pinned) menu icon, and presses dismiss. We can track it like so: { "event": "action", "method": "as_highlight_contextmenu", "extras": { "item": "dismiss", "position": 3, "highlight_origin": "bookmarked", "pinned": false, "sync_enabled": true } } This should give us a lot of flexibility in what we "extra" information we track about data, we'll be able to add extra fields to events over time, and we'll be able to slice this data in many ways to answer question such as "what are the most frequent actions people perform with highlights that originate from recent bookmarks".

It seems that some basic telemetry already landed in Bug 1301468! Somehow I've missed that tree of bugs. It doesn't seem like all of the events are tracked, and if we're able to use JSON extras tracking various additional fields seems valuable.

Confirmed with folks over at #telemetry that dumping JSON strings into the extras field and querying against it should work just fine. I'm going to re-purpose this bug to track "richer AS telemetry" work.

Other than landing this patch, my TODO list for this work is: - document new rich AS telemetry pings - build enough re:dash queries (and ideally combine them into a dashboard) to ensure that a) data is flowing through, b) JSON querying is easy enough to use and to start looking at pretty graphs!

One thing that we might need to address at some point (or, now) is bandwidth implications of this, both for the user (sending all of the telemetry might add up over time), and for us on the receiving end. As a starting point, this could be as simple as replacing keys and/or values with indexes.

In case you haven't seen it yet, this document describes the metrics of the desktop add-on: https://github.com/mozilla/activity-stream/blob/master/data_events.md

(In reply to :Grisha Kruglov from comment #12) > One thing that we might need to address at some point (or, now) is bandwidth > implications of this, both for the user (sending all of the telemetry might > add up over time), and for us on the receiving end. As a starting point, > this could be as simple as replacing keys and/or values with indexes. See bug 1249373 too.

(In reply to :Grisha Kruglov from comment #6) > Confirmed with folks over at #telemetry that dumping JSON strings into the > extras field and querying against it should work just fine. I'm going to > re-purpose this bug to track "richer AS telemetry" work. You can test this in re:dash (do the JSON functions work there too?) with the telemetry we send for preferences. Those probes already send JSON-formatted extras.

Comment on attachment 8820450 [details] Bug 1319245 - Documentation for Activity Stream telemetry https://reviewboard.mozilla.org/r/99948/#review100462 Nit: a couple of trailing spaces Thanks for the detailed docs describing what each probe is measuring! When this lands, consider linking it to the A-S wiki page (if there is one). I'm not convinced that people know where to find this kind of documentation. The only thing that I could see potentially being a problem is if A-S starts overloading action-method pairs (e.g., loadurl-listitem), where multiple telemetry probes start to look indistinguishable. You might consider including a "probe name" parameter in the extras json blob anticipating this (or just add it later). (If you do end up adding it now, also add an example in the mobile redash docs [1] of how to check json parameters [2]) [1] https://wiki.mozilla.org/Mobile/Metrics/Redash#Useful_Queries [2] https://prestodb.io/docs/current/functions/json.html ::: mobile/android/docs/activitystreamtelemetry.rst:17 (Diff revision 1) > + > +Global extras > +============= > +A concept of a "global" extra is meant to support recording certain context information with every event that is being sent, regardless of its type. > + > +``fx_account_present``, values: true, false Is this available under some other probe? Though including it here would certainly make your data query simpler (and it's possible this is in a separate table from ui-telemetry in redash). ::: mobile/android/docs/activitystreamtelemetry.rst:57 (Diff revision 1) > + ... > + "source_type": "highlights", > + "source_subtype": "visited"/"bookmarked" > +} > + > +Subtype indicates reson an item being which is being interacted with appeared in the Highlights: typo "reson"

Comment on attachment 8819159 [details] Bug 1319245 - Track rich telemetry data for Activity Stream https://reviewboard.mozilla.org/r/98994/#review100750 I like it! r+ if we can make sure that ... * ... this matches the needs of barbara/marina. From previous meetings I have the impression that they want to have at least the same metrics the desktop team has right now. The desktop metrics are described on GitHub [1]. They are more verbose and some of those things we do not need to collect in the AS probe because we can get it from somewhere else. * ... we can compare AS with the current top sites panel. You didn't remove the existing telemetry, so I guess this won't be a problem. * ... we can parse the JSON from re:dash. I wonder how performant that is? My naiive thinking is that this can't use any traditional query optimizations? Maybe we can do a test run with some of the preferences JSON telemetry data? [1] https://github.com/mozilla/activity-stream/blob/master/data_events.md ::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStream.java:31 (Diff revision 4) > import java.util.Arrays; > +import java.util.HashMap; > import java.util.List; > > public class ActivityStream { > + public final static class TelemetryExtra { I just noticed that I made a mistake creating this generic ActivityStream class. It now becomes a dumping ground for all kinds of stuff. :) Maybe it makes sense to move this to some kind of ActivityStreamTelemetry class and move the ActivityStreamExtras in there as well? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:63 (Diff revision 4) > final Resources resources = getResources(); > desiredTileWidth = resources.getDimensionPixelSize(R.dimen.activity_stream_desired_tile_width); > desiredTilesHeight = resources.getDimensionPixelSize(R.dimen.activity_stream_desired_tile_height); > tileMargin = resources.getDimensionPixelSize(R.dimen.activity_stream_base_margin); > + > + org.mozilla.gecko.activitystream.ActivityStream.TelemetryExtra.setGlobal( This line would be nicer with a dedicated class too :) ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamItem.java:193 (Diff revision 4) > ActivityStreamContextMenu.show(v.getContext(), > menuButton, > + extras, > ActivityStreamContextMenu.MenuMode.HIGHLIGHT, > title, url, isBookmarked, isPinned, > onUrlOpenListener, onUrlOpenInBackgroundListener, > vIconView.getWidth(), vIconView.getHeight()); This method is getting bigger and bigger. At some point we should think about something better here too. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamItem.java:295 (Diff revision 4) > public void onIconResponse(IconResponse response) { > vIconView.updateImage(response); > } > } > > - private static HighlightItem.HighlightSource highlightSource(final Cursor cursor) { > + /* package-local */ static HighlightItem.HighlightSource highlightSource(final Cursor cursor) { If we open up those methods (and the one below) - maybe it makes sense to move them out of the view? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamRecyclerAdapter.java:127 (Diff revision 4) > > onUrlOpenListener.onUrlOpen(url, EnumSet.of(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)); > > - Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM, "as_highlights"); > + ActivityStream.TelemetryExtra.Builder extras = ActivityStream.TelemetryExtra.builder(); > + > + HighlightItem.setHighlightSourceToExtras(HighlightItem.highlightSource(highlightsCursor), extras); This line is super hard to read somehow. :) If 'extras' is a builder couldn't I just provide it with the cursor and it does whatever it needs to do itself? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:163 (Diff revision 4) > protected Void doInBackground() { > final Cursor cursor = BrowserDB.from(context).getHistoryForURL(context.getContentResolver(), url); > - try { > + if (cursor == null) { > - if (cursor != null && > - cursor.getCount() == 1) { > - hasHistory = true; > - } else { > - hasHistory = false; > + hasHistory = false; > + return null; > - } > + } > + try { > + hasHistory = cursor.getCount() == 1; > } finally { > cursor.close(); > } > return null; > } Not your code, but as you are editing this: This AsyncTask is weird. Instead of a class property (hasHistory) this should just return the boolean from doInBackground() and then it will receive it in onPostExecute() automatically. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesCard.java:106 (Diff revision 4) > + ActivityStream.TelemetryExtra.Builder extras = ActivityStream.TelemetryExtra.builder(); > + > + extras.set(TelemetryContract.ActivityStreamExtras.SOURCE_TYPE, TelemetryContract.ActivityStreamExtras.TYPE_TOPSITES); > + > + if (isPinned(type)) { > + extras.set(TelemetryContract.ActivityStreamExtras.SOURCE_SUBTYPE, TelemetryContract.ActivityStreamExtras.SUBTYPE_PINNED); > + > + } else if (isSuggested(type)) { > + extras.set(TelemetryContract.ActivityStreamExtras.SOURCE_SUBTYPE, TelemetryContract.ActivityStreamExtras.SUBTYPE_SUGGESTED); > + > + // While we also have a "blank" type, it is not used by Activity Stream. > + } else { > + extras.set(TelemetryContract.ActivityStreamExtras.SOURCE_SUBTYPE, TelemetryContract.ActivityStreamExtras.SUBTYPE_TOP); > + } There's so much telemetry code in the view now. I wonder if we could move this into the builder and just pass it the TopSitesCard object? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesCard.java:141 (Diff revision 4) > private static boolean isPinned(int type) { > return type == BrowserContract.TopSites.TYPE_PINNED; > } > + > + private static boolean isSuggested(int type) { > + return type == BrowserContract.TopSites.TYPE_SUGGESTED; > + } I wonder why they are static and not just methods on TopSitesCard.

Comment on attachment 8819159 [details] Bug 1319245 - Track rich telemetry data for Activity Stream https://reviewboard.mozilla.org/r/98994/#review100750 Reading through their doc, this mostly provides the same data and more (with an exception for position of top sites interactions, for which we need to first shuffle a few things - see follow-up Bug 1325303). The queries are probably not going to be fast, depending on how we'll be slicing this up. We'll have to see if they're so slow it becomes a problem once more people are using A-S. From the point of view of those consuming results of the queries, it's probably OK if we can only run them bi-weekly, for example. I'll start creating some JSON queries to test-drive this (and other json) data. > I just noticed that I made a mistake creating this generic ActivityStream class. It now becomes a dumping ground for all kinds of stuff. :) > > Maybe it makes sense to move this to some kind of ActivityStreamTelemetry class and move the ActivityStreamExtras in there as well? Moved AS telemetry stuff (extras builder, constants) into a separate class, added some docs. > If we open up those methods (and the one below) - maybe it makes sense to move them out of the view? Yup, cleaned this up; created a utils class to hold some constants and statics. > This line is super hard to read somehow. :) > > If 'extras' is a builder couldn't I just provide it with the cursor and it does whatever it needs to do itself? Reading this line now: lol. Yeah, not sure what I was thinking. Moved this to the builder. > There's so much telemetry code in the view now. I wonder if we could move this into the builder and just pass it the TopSitesCard object? Yeah, I like that approach more. I've added a few AS-specific builder methods.

Comment on attachment 8820450 [details] Bug 1319245 - Documentation for Activity Stream telemetry https://reviewboard.mozilla.org/r/99948/#review100980 Nice. I think I'm going to clone this for bug 1272354. :)

Comment on attachment 8821038 [details] Bug 1319245 - Post: clean up async tasks and access levels https://reviewboard.mozilla.org/r/100412/#review100982

Comment on attachment 8819159 [details] Bug 1319245 - Track rich telemetry data for Activity Stream https://reviewboard.mozilla.org/r/98994/#review100984 Like!

Pushed by gkruglov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e6bdb4d5fab Track rich telemetry data for Activity Stream r=sebastian https://hg.mozilla.org/integration/autoland/rev/f783ca75362d Documentation for Activity Stream telemetry r=liuche,sebastian https://hg.mozilla.org/integration/autoland/rev/a5f4f62bf0fa Post: clean up async tasks and access levels r=sebastian

Comment on attachment 8820450 [details] Bug 1319245 - Documentation for Activity Stream telemetry https://reviewboard.mozilla.org/r/99948/#review101298 ::: mobile/android/docs/activitystreamtelemetry.rst:65 (Diff revision 1) > + > +For "loadurl.1" event, the following extra information is also recorded: > + > +extras: { > + ... > + "action_position": number, /* 0-based index of a highlight being interacted with */ is this the position the highlights item appears in the list? If so, should we have the same for topsites items as well? ::: mobile/android/docs/activitystreamtelemetry.rst:108 (Diff revision 1) > +method="contextmenu" > +extras="{ > + 'fx_account_present': true, > + 'source_type': 'highlights', > + 'source_subtype': 'visited' > +}" How come only example 2) shows count and action_position?

(In reply to Barbara Bermes [:barbara] - NI please! from comment #31) > is this the position the highlights item appears in the list? If so, should > we have the same for topsites items as well? Bug 1325303 :)

(In reply to Barbara Bermes [:barbara] - NI please! from comment #31) > ::: mobile/android/docs/activitystreamtelemetry.rst:108 > (Diff revision 1) > > +method="contextmenu" > > +extras="{ > > + 'fx_account_present': true, > > + 'source_type': 'highlights', > > + 'source_subtype': 'visited' > > +}" > > How come only example 2) shows count and action_position? Position/count are currently only tracked for clicks on Highlights, and there's a bug 1325303 to track them for clicks on Top Sites. Would you like to track them for "open context menu" actions as well?

https://hg.mozilla.org/mozilla-central/rev/7e6bdb4d5fab https://hg.mozilla.org/mozilla-central/rev/f783ca75362d https://hg.mozilla.org/mozilla-central/rev/a5f4f62bf0fa