1241571 - Add UI telemetry to synced tabs menu panel and sidebar

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Chris Karlof [:ckarlof]]

Exact measures TBD.

Comment 1

[Chris Karlof [:ckarlof]]

Ryan, could you work with Zach to determine the highest priority measures of how users are interacting with the menu panel and sidebar?

Comment 2

[Ryan Feeley [:rfeeley]]

Track these states when opened Synced Tabs (menu panel) & Synced Tabs Sidedar:
– not signed in to sync
– not verified
- single device
- multi-device no open tabs
- multi-device tab syncing disabled
- mutli-device syncing

UI to track (by priority):
Synced Tabs Sidebar – (track open web page)
Synced Tabs (menu panel) — (open web page)
Customize Mode — Synced Tabs button moved
Synced Tabs (menu panel) — Sync Now
View – Sidebar — Synced Tabs (track opens)
View – Sidebar — Synced Tabs (track closes)
History Menu — Synced Tabs (opens menu panel)
Synced Tabs (menu panel) — View Synced Tabs Sidebar (track opens)
Synced Tabs (menu panel) — View Synced Tabs Sidebar (track closed)
History (menu panel) — Synced Tabs (opens menu panel)
Synced Tabs Sidebar – (track expanding device name)
Synced Tabs Sidebar – (track collapsing device name)
Synced Tabs Sidebar – X (track sidebar closes)
Synced Tabs Sidebar – (track entering text into search box)
Synced Tabs Sidebar – (track clearing text in search box)

Comment 3

[Edwin Wong [:edwong]]

:ckarlof - NI you so we can track this task

Comment 4

[Chris Karlof [:ckarlof]]

It's already got firefox-backlog+ and a priority.

Comment 5

[Edwin Wong [:edwong]]

Need problem statement, artifacts supported by measurement/probes.

Comment 6

[Javaun Moradi [:javaun]]

Some questions we want to answer:


1. How often do users access sync tabs in Sidebar? (i.e. is it open)
2.               "                      in Menu bar? (i.e. is it open)
 (Said another way: how do the two different UI methods compare?)
3, 4. How often do users open a sync tab from either UI? 

1,2 and 3,4 are a bit different. 1,2 may indicate interest in the feature and demonstrate awareness of the feature and ability to find the UI. 3,4 are about completing an actual task.

Accessing a synced tab is a continuity task. We'll likely compare behavior here to other continuity tasks, like accessing bookmarks.

Comment 7

[Edwin Wong [:edwong]]

it also might be good to know if we have 'active users/profiles' or someone who uses the synced tabs UI once every 7 days. This may be able to be derived once we have counts.  Some may try things out one time and churn and never use it again. It would be good to be able to see our feature churn/retention model

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Ryan,
  In this bug we are trying to reduce the scope from "measure all the things" to "measure if a user positively engages with the feature". We feel that, for example, tracking whether a user expanded and collapsed a device doesn't really measure engagement, whereas measuring "did the user select a tab to be opened" does.

Would it be possible for you to prune or otherwise adjust the list in comment 2 down to the minimal set of measurements that reflects user engagement? My gut feel is that this list could literally just be "did the user select a tab?", but YMMV.

Comment 9

[Ryan Feeley [:rfeeley]]

Pared down.

Synced Tabs Sidebar – (track opens and current state)
Synced Tabs Menu Panel – (track opens and current state)

Possible states:
– not signed in to sync
– not verified
- single device
- multi-device no open tabs
- multi-device tab syncing disabled
- mutli-device syncing

Other interactions to track (by priority):
Synced Tabs Menu Panel — (track open web page)
Synced Tabs Sidebar – (track open web page)
Synced Tabs (menu panel) — Sync Now
Customize Mode — Synced Tabs button moved
History Menu — Synced Tabs (opens menu panel)

I think the last three are worth having, but they are the lowest priority of the bunch.

Comment 10

[Edwin Wong [:edwong]]

Triage: NI rfeeley - let's polish our metrics user stories

Comment 11

[Javaun Moradi [:javaun]]

Our initial telemetry task is to validate that the new Sync Tabs UI is even being used. Our MVP recommendation is to launch minimal probes to record the basics, and use off-the-shelf dashboards before building anything else out. I'm attaching a graphic of two very simple sketches of what an artifact might look like for each probe, and reusing them (referenced as A or B). The timeline happens to be months, but "time" should be the general x-axis. 

For these probes, the canned Telemetry plots or histograms are absolutely fine.
 
Question area: UI discovery/engagement 
* Probe: Opens of toolbar panel UI (artifact a)
* Probe: Opens of sidebar UI (artifact a)

Question area: Task completion/Engagement
* Probe: Opens of tabs from panel (artifact a)
* Probe: Opens of tabs from sidebar (artifact a)

Question area: Sync Tab in Awesome Bars
* Probe: Displays of a tab in Awesome Bar (artifact a)
* Probe: User Clicks a tab in Awesome Bar (artifact a)

Adoption:
* Probe: Users/Instances who have moved the toolbar icon from the Customize menu (artifact b)

Here are some high-level needs for the Above Probes: 
1. We should instrument such that we have the ability to segment each probe above by single (=1) vs. multi-device (>1) users
2. We should instrument such that we can tie browser actions (i.e a panel open) to a unique instance (i.e. a proxy for a user), so that *later* we can answer questions like: what percentage of Daily Average Instances (DAIs or DAUs) out of total Sync users are taking this action? (i.e. calculate engagement).

Comment 12

[Javaun Moradi [:javaun]]

Attachment: artifacts.png

Quick drawing mapping to probes in comment 11

Comment 13

[Javaun Moradi [:javaun]]

Note: for Artifact A, we don't need a single stacked-line graph showing both single/multi device users on the same chart. We could have one automated chart that shows single device users over time, and then toggle to see multidevice users over time. (Essentially artifact B above). Again, I'd rather go w/ whatever we get out of the box for free, rather than tweak the format at the outset to create the perfect presentation

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-wip-to-collect-synced-tabs-ui-telemetry.patch

While we almost certainly will end up creating a custom ping for Sync/FxA, I think we should try and do this particular bug via existing UITelemetry mechanisms, as most of what we need is already in place for that. We'd need to add a couple of extra probes, but the existing mechanism already has support for, eg, users moving the toolbar icons from their default place. Unfortunately, the existing dashboard for UITelemetry is broken, but I'm in discussions with Roberto Vitillo and Ilana Segal about that - but even in a worst-case, we'd have no dashboard for interpreting the data, which is the exact same position we'd be in if we decided to use a custom ping - and we don't need to add sync-specific telemetry code to the browser UI.

The attached patch shows a WIP for this, and best I can tell, records basically all the info we need here - but no way to report on it. With this patch applied, the UITelemetry payload looks like:

"UITelemetry": {
"toolbars": {
  "sizemode": "normal",
  "bookmarksBarEnabled": true,
  "menuBarEnabled": true,
  "titleBarEnabled": false,
  "defaultKept": [
<snip all toobar buttons that have not moved>
  ],
  "defaultMoved": [
    "sync-button" // this profile moved the sync-button
  ],
<snip more>
      "click-builtin-item": {
        "sync-button": {
          "left": 1 // The sync button was left-clicked once.
        },
...
      "synced-tabs": {
        "open": {
          "menu": 1 // One tab was opened via the SyncedTabs menu
        }
      }
    }
  },
"syncstate": {
  "desktop": 8, // this profile has 8 desktop devices.
  "mobile": 1 // and 1 mobile device.
}

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Ilana,
  Thanks for the chat about this and your kind offer to help us out here :) To confirm our discussion, I think we agreed that I should add additional data to the existing UITelemetry data in the main ping, and you will be able to create ad-hoc spark queries for us in the short term. In the longer term, the data format of UITelemetry will be changing in a way that makes it easier to access without using spark, at which time we will look into creating our own dashboard without relying on your assistance. This means that for the next couple of months we'll periodically ask you to extract the data for us - so while we wont have a dashboard, we will have some visibility into the data. I hope I got that right.

In comment 11, Javaun has added a description of the data we are trying to track. My patch here adds some data to UITelemetry, which I'll describe in more detail below. Can you please confirm that comment 11 is fairly clear, and that given the additional data I describe below, you will be able to write a query that helps answer those questions?

With the changes in my patch, countableEvents now looks something like:

>          "countableEvents": {
>            "__DEFAULT__": {
>              "click-builtin-item": {
>                "sync-button": {
>                  "left": 2
>                },

This is already in the payload for all buttons, but it reflects I made 2 left-clicks the sync-button. The existing "defaultMoved" entry will also reflect if the button has been moved.

>                ...
>              },
>              "synced-tabs": {
>                "open": {
>                  "menu": 2,
>                  "sidebar": 1
>                }
>              },

This is a new entry added by my patch - it shows that I selected 2 tabs from the synced-tabs button, and 1 from the synced-tabs sidebar.

>              "sidebar": {
>                "viewTabsSidebar": {
>                  "open": 1
>                }
>              }
>            }
>          },

This is also new in my patch and tracks all opens and closes of all sidebars - in this example I only opened the "viewTabsSidebar" - the bookmarks and history sidebars may also appear here, and they may get a "close" count if the user closes the sidebar.

Also, I've added a new top-level object to UITelemetry, that looks like:

>        "syncstate": {
>          "desktop": 5,
>          "mobile": 1
>        }

Which indicates that my Sync account has 5 desktop devices attached (including the one where the data is gathered, so 4 additional desktops) and 1 mobile device. If the sum of these 2 figures are 0 then the user doesn't have sync configured, if the sum is 1 then the user is a "single device user", which comment 11 mentions. Note too that it would be (obviously) easy for me to sum these in the client, but I wouldn't be surprised to find later analysis might ask about mobile device users. Also, these values *are* already in histograms (WEAVE_DEVICE_COUNT_DESKTOP/WEAVE_DEVICE_COUNT_MOBILE, but the histograms will only be updated after a Sync has completed, so may not be accurate in all cases (although it may be reasonable to completely ignore submissions without those histograms - it implies a very short session). If you think the histograms will be OK, I can remove this entry from my patch.

I hope the above makes sense and I'm happy to have another chat if you'd like to clarify anything here. Once I've got your feedback I'll go through the normal steps of getting the code itself reviewed and landed.

Thanks alot for your help!

Comment 16

[Ilana]

This looks fantastic! 

A few clarification questions:

- We were not interested in any orderings of events, correct?
- Will the syncstate be updated at the beginning or end of the session? Without these states, is there a way to know that sync has been configured by a user except to see that the state is 0 in a session then >=1 in following ones?
- Do we capture clicks on the Tools menu, or is that a deprecated feature at this point?
- Is there an event which actually captures the act of signing into sync besides deducing that it is open and has been interacted with?


Excuse my ignorance of the sync interface!

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ilana from comment #16)
> This looks fantastic! 
> 
> A few clarification questions:
> 
> - We were not interested in any orderings of events, correct?

At this stage that's correct. As you mentioned in our vidyo chat, the fact we might later be able to dig a little deeper into things if we had ordering might end up useful, but at this stage just the fact they clicked at all will be an excellent start.

> - Will the syncstate be updated at the beginning or end of the session?

It is updated as all the data is collected by the telemetry framework (ie, by TelemetrySession.jsm) as it is putting together the payload - looking at the telemetry code, it seems like at this time the UITelemetry is asked "give me all your data", and we respond with all the information at that time - all the click information we've so far collected and also the sync state. IOW, it is updated exactly when everything else in UITelemetry is updated. I hope that answers your question :)

> Without these states, is there a way to know that sync has been configured
> by a user except to see that the state is 0 in a session then >=1 in
> following ones?

The FXA_CONFIGURED histogram can tell you that - it will be reported once per browser-session as Sync initializes, which is generally 10s after startup. It looks to me like the histogram data is collected at the same time as the UITelemetry (ie, when building the payload) - but obviously that only works if the histogram itself has been recorded by the time the payload is requested. I'm afraid I don't know enough about telemetry to answer that question, but in general, 10s after startup we should expect to see FXA_CONFIGURED set if Sync is configured at all, and a few seconds after that we should see WEAVE_DEVICE_COUNT_DESKTOP/WEAVE_DEVICE_COUNT_MOBILE also recorded.

> - Do we capture clicks on the Tools menu, or is that a deprecated feature at
> this point?

Best I can tell, only the context menu and hamburger menus record UITelemetry, not the top-level tools menu. I don't think it would be a problem to record the top-level menu when you look at revamping how this telemetry is recorded.

> - Is there an event which actually captures the act of signing into sync
> besides deducing that it is open and has been interacted with?

In general, users sign into Sync exactly once, then remain signed in - eg, I haven't done a "sign in" to my main browser in well over a year. The FXA_CONFIGURED probe I mentioned above records that Sync has been initialized - most likely due to an existing logged in user but also due to a new user signing in. There's no specific way currently to note that a new user signed in, but it would be quite trivial to add a new histogram to record that. It also wouldn't be difficult to record directly in UITelemetry that this happened.

Thanks for your help! I'm re-adding needinfo just incase the above isn't clear or you need any further information.

Thanks!

Comment 18

[Ilana]

Looks good to me! I think it's safe to defer the multiple account signin case unless it's something you're actively interested in right now. Otherwise, this is more than enough to get going. Thank you!

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ilana from comment #18)
> Looks good to me! I think it's safe to defer the multiple account signin
> case unless it's something you're actively interested in right now.
> Otherwise, this is more than enough to get going. Thank you!

Thanks Ilana! It's not clear to me if you think we do need the new "sync-status" field in the UITelemetry record, or if you will be able to use the information already recorded in histograms for that? Once I know whether I should keep or remove that I'll update the patch and request review.

Thanks.

Comment 20

[Ilana]

If it's not too much trouble to add it to the UITelemetry field, let's do that - it makes the queries much faster and easier. However, if necessary, I can use what's in the histogram.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui telemetry. r?Gijs

Adds UI telemetry for sidebar opening and closing and actions taken in the
SyncedTabs menu and side. Also adds a "sync-state" object so that analysis
of the Synced Tabs data can determine if the user has Sync configured at the
time.

Review commit: https://reviewboard.mozilla.org/r/52421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52421/

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8752092 [details]
MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui telemetry. r?Gijs

Hey Gijs! This patch adds some information to the UITelemetry payload, via BrowserUITelemetry.jsm, to measure the synced tabs work. I've been working with Ilana to ensure the data is "actionable" and I'll get a rubber-stamp from a "data peer" before landing, and hopefully the bug has any additional context you need.

It's got tests, but BrowserUITelemetry doesn't have much in the way of existing tests, so I'm a bit brutal with it in them :) Let me know if you'd prefer a different testing approach.

Thanks!

Comment 23

[:Gijs (he/him)]

Comment on attachment 8752092 [details]
MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui telemetry. r?Gijs

https://reviewboard.mozilla.org/r/52421/#review49407

<3 for tests.

::: browser/components/customizableui/CustomizableWidgets.jsm:490
(Diff revision 1)
>        // We need to use "click" instead of "command" here so openUILink
>        // respects different buttons (eg, to open in a new tab).
>        item.addEventListener("click", e => {
>          doc.defaultView.openUILink(tabInfo.url, e);
>          CustomizableUI.hidePanelForNode(item);
> +        BrowserUITelemetry.countSyncedTabEvent("open", "menu");

Seems like this should be called "toolbarbutton-subview" rather than "menu", especially if there are ways this is / will be possible from the top menubar ("old style"  menu).

::: browser/modules/BrowserUITelemetry.jsm:640
(Diff revision 1)
> +    // "where" will be "menu" or "sidebar"
> +    this._countEvent(["synced-tabs", what, where]);
> +  },
> +
> +  countSidebarEvent: function(sidebarId, what) {
> +    this._countEvent(["sidebar", sidebarId, what]);

Nit: I think "what" would more accurately be called "action".

I also think it would be a teeny bit nicer to have the actions (currently "open" and "close") match the names of the methods on the sidebar code, so be "show" and "hide", respectively. This also clarifies that they do not correspond 1:1 with opening and closing the sidebar as a whole.

::: browser/modules/test/browser_BrowserUITelemetry_sidebar.js:49
(Diff revision 1)
> +    sidebar: {
> +      viewTabsSidebar: { open: 3, close: 2 },
> +      viewBookmarksSidebar: {open: 1, close: 1},
> +    }
> +  });
> +  yield SidebarUI.hide();

Nit: make this a toggle too and assert that that gets recorded as a close as well?

::: browser/modules/test/browser_BrowserUITelemetry_syncedtabs.js:52
(Diff revision 1)
> +}
> +
> +mockSyncedTabs();
> +
> +function promiseTabsUpdated() {
> +  const topic = "synced-tabs-menu:test:tabs-updated";

Nit: use the const or omit it.

::: browser/modules/test/browser_BrowserUITelemetry_syncedtabs.js:81
(Diff revision 1)
> +  let tabEntry = tabList.firstChild.nextSibling;
> +  tabEntry.click();
> +
> +  let counts = BUIT._countableEvents[BUIT.currentBucket];
> +  Assert.deepEqual(counts, {
> +    "click-builtin-item": { "sync-button": { "left": 1 } },

Nit: here and below, I would personally prefer all the object properties that don't include dashes to be rendered without quotes, as that is also valid. You do this in the next line, and in the previous test, but the rest of this file uses things like "open" and "sidebar" with quotes when it doesn't need to. :-)

(It would probably not have bothered me enough to get you to change it if it had been consistent, but it isn't... now that we're picking, it feels like just settling on non-stringified keys would be more readable in the long term. :-) )

::: browser/modules/test/browser_BrowserUITelemetry_syncedtabs.js:94
(Diff revision 1)
> +
> +  yield SidebarUI.show('viewTabsSidebar');
> +
> +  let syncedTabsDeckComponent = SidebarUI.browser.contentWindow.syncedTabsDeckComponent;
> +
> +  syncedTabsDeckComponent._accountStatus = () => Promise.resolve(true);

This is a little evil. I don't suppose there's a cleaner way to do this?

::: browser/modules/test/browser_BrowserUITelemetry_syncedtabs.js:100
(Diff revision 1)
> +
> +  yield syncedTabsDeckComponent.updatePanel();
> +  // This is a hacky way of waiting for the view to render. The view renders
> +  // after the following promise (a different instance of which is triggered
> +  // in updatePanel) resolves, so we wait for it here as well
> +  yield syncedTabsDeckComponent.tabListComponent._store.getData();

Can you add a BTU helper for MutationObserver to watch for child node updates instead? You can model it on BTU's waitForAttributes. That way we don't need to depend on the implementation of the syncedTabsDeckComponent.

::: services/sync/modules/engines/clients.js:229
(Diff revision 1)
>      SyncEngine.prototype._uploadOutgoing.call(this);
>    },
>  
>    _syncFinish() {
> -    // Record telemetry for our device types.
> +    // Record telemetry for our device types, and also write them to a pref
> +    // so telemetry etc has easy access to them.

Nit: "so we can record them in UITelemetry and analyze them in one place"

"telemetry etc" is a bit odd because we're writing to telemetry here as well... :-)

Comment 24

[Ilana]

Gijs - The redundancy here actually makes life a lot easier, and I'd also propose that this is the proper place for the data to be stored (instead of in histograms). If we hate the redundancy, I'd propose removing it from histograms altogether.

Comment 25

[:Gijs (he/him)]

(In reply to Ilana from comment #24)
> Gijs - The redundancy here actually makes life a lot easier, and I'd also
> propose that this is the proper place for the data to be stored (instead of
> in histograms). If we hate the redundancy, I'd propose removing it from
> histograms altogether.

Yes, I think it's fine like this, though I wouldn't object to removing from histograms. My point was solely about the comment Mark added in his patch. :-)

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #23)
> Comment on attachment 8752092 [details]
> MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui
> telemetry. f?Gijs

Thanks!

I addressed all comments, except:

> > +  syncedTabsDeckComponent._accountStatus = () => Promise.resolve(true);
> 
> This is a little evil. I don't suppose there's a cleaner way to do this?

Not really :( Bug 1191162 is on file to provide a better story for using FxA in browser tests.

> Can you add a BTU helper for MutationObserver to watch for child node
> updates instead? You can model it on BTU's waitForAttributes. That way we
> don't need to depend on the implementation of the syncedTabsDeckComponent.

Turns out I can just use waitForAttribute.

Review request coming up!

Comment 27

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ilana from comment #24)
> Gijs - The redundancy here actually makes life a lot easier, and I'd also
> propose that this is the proper place for the data to be stored (instead of
> in histograms). If we hate the redundancy, I'd propose removing it from
> histograms altogether.

FTR: Poking around sql.telemetry.mozilla.org, I notice some of these histograms are being used currently to help with some of the "retention" data being generated - otherwise, yeah, I probably would have removed the histograms. We should be rationalizing all Sync telemetry soon, so we can clean up the sync histograms later (possibly by removing all the histograms it generates!)

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8752092 [details]
MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui telemetry. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52421/diff/1-2/

Comment 29

[:Gijs (he/him)]

Comment on attachment 8752092 [details]
MozReview Request: Bug 1241571 - Collect synced tabs and sidebar ui telemetry. r?Gijs

https://reviewboard.mozilla.org/r/52421/#review49964

::: browser/modules/BrowserUITelemetry.jsm:635
(Diff revision 2)
>      this._countEvent(["tab-audio-control", action, reason || "no reason given"]);
>    },
>  
> +  countSyncedTabEvent: function(what, where) {
> +    // "what" will be, eg, "open"
> +    // "where" will be "menu" or "sidebar"

Nit: still need to update this comment to not say "menu", I think?

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/59d131412f85

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59d131412f85

Comment 32

[Azmina]

This Bug was about to implement Synced Tabs Sidebar, Synced Tab Menu Panel and many others mentioned as Comment 2. I have seen the features being implemented with Aurora 49.0a2 on  Windows 8.1 , 64 bit!

This bug's fix is verified on Latest Aurora 49.0a2 

Build ID : 20160723004004
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160722]