Closed Bug 1379226 Opened 7 years ago Closed 7 years ago

Add telemetry probe(s) to measure use of session restore button in tab bar

Categories

(Firefox :: Session Restore, enhancement)

56 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: pdol, Assigned: ewright)

References

Details

User Story

As a Firefox product manager, I want to be able to determine the following information with respect to the session restore button in tab bar (bug 1219725):
- How often does the button get shown? (totals, average display per user, percentage of users that see it, in a given time period)
- At what rate is the button actually used? (as a percentage of how many times it is shown and also as a percentage of unique users that it is shown to in a given time period.)
- How many sessions are restored via this button vs. other ways? (as a percentage)
- How many tabs are restored via the button? (average, median, 90th percentile)

That way I can tell if the feature is actually used by users.

Attachments

(1 file)

      No description provided.
Erica/Blake, feel free to add anything else that you think might be useful.
Flags: needinfo?(ewright)
Flags: needinfo?(bwinton)
- a percentage of how many sessions are restored via this button vs. other ways
Flags: needinfo?(ewright)
Yeah, what Erica said.  :)
Flags: needinfo?(bwinton)
User Story: (updated)
Assignee: nobody → ewright
The on-click ping is already in, it can be found under `UITelemetry.toolbars.countableEvents.__DEFAULT__.click-builtin-item.restore-tabs-button.left` on about:telemetry.
There seems to be tracking already built in for how many tabs/windows are restored each from each session restore - and we can get from that how many times session restore is used in general.
I've added a measurement to send along whether the button was ever available.

with this I believe all cases are covered.
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review161750

Unfortunately I don't think this is quite right. Sorry for the delays - I'll be away tomorrow and Friday, but should be back on Monday (reviews are still open, they just likely won't happen before then).

::: browser/modules/BrowserUITelemetry.jsm:531
(Diff revision 1)
> +    // Determine if the tabbar's session restore button is visible.
> +    let restoreTabsButtonWrapper = document.getAnonymousElementByAttribute(

This fires off the search service initialization, which will race with the session store initialization, so it's not clear this will provide a reliable answer to whether the button is visible.

This also doesn't answer the other questions in the user story (percentage of sessions restored via the button, number of times it's actually used vs. shown, etc.).

Finally, I'm worried that my most recent understanding is that we don't have a good way of looking at BrowserUITelemetry, so maybe "normal" histograms would be more appropriate? I expect Blake has the most up-to-date view into that stuff though, so if we think BrowserUITelemetry is the best place for this, we can use it...
Attachment #8885441 - Flags: review?(gijskruitbosch+bugs)
:gijs

> This also doesn't answer the other questions in the user story (percentage of sessions restored via the button, number of times > it's actually used vs. shown, etc.).

From this information, I believe all of the requested info is able to be calculated, because the click event on the button was merged with the first bug.
I found evidence that the amount of tabs restored is info we already get, and how many sessions are restored this way vs other ways is info we'd already get.
(In reply to Erica from comment #7)
> :gijs
> 
> > This also doesn't answer the other questions in the user story (percentage of sessions restored via the button, number of times > it's actually used vs. shown, etc.).
> 
> From this information, I believe all of the requested info is able to be
> calculated, because the click event on the button was merged with the first
> bug.
> I found evidence that the amount of tabs restored is info we already get,
> and how many sessions are restored this way vs other ways is info we'd
> already get.

Can you clarify where you believe we get this data? I'm especially confused about checking how many sessions get restored via the button vs. via other means. I thought the button did exactly the same thing as, say, clicking the menuitem in the history view (ie it calls SessionStore.restoreLastSession()) which I assume is also the same as clicking the item in about:home.

I guess based on bug 1219725 we now have UI telemetry, but it might be tricky to do the calculations from that as well as an unrelated histogram. In fact, I am not sure where to look at UI telemetry at all...
Flags: needinfo?(ewright)
After speaking with :bwinton about it, we are going to go with Scalars to get (allegedly) free graphs. So I'll be redoing all the pings
Flags: needinfo?(ewright)
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156290/#review164628

::: toolkit/components/telemetry/Scalars.yaml:281
(Diff revision 2)
> +  tabbar_restore_available:
> +    bug_numbers:
> +      - 1379226
> +    description: >
> +      Recorded on startup. Boolean stating whether the tabbar session
> +      restore button was ever available. 

will fix the whitespace after you comment, :gijs, as I'm expecting you'll want other changes :D
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review164886

Definitely going in the right direction, but I'm confused about the window/tab count (I kind of assumed we would want to record something like "5 windows with 15 tabs across those windows", rather than record "1 window, 3 tabs" 5 times), and the additional observer topics (which I guess are probably my fault from our slack conversation - I assumed the telemetry would go outside of the button's init() thing - sorry!).

I don't see something recording the source of the restore. Do we intend to just infer the number of cases where users restore via other entry points using the total number of restores, minus the number of people with startup set to autorestore, minus the number of people that clicked our button, or something? Is it worth doing something else? (for context, for the migration dialog, we basically made the migration dialog startup telemetry-store a number that indicated how it got opened (automatically on startup, via the passwords manager, via the bookmarks manager (library), etc.) ). FWIW, I can live with "no", I just want to make sure that we get the data we need. I remember having to repeatedly add more telemetry to automigration and I would like to get it done in one go this time around. :-)

::: browser/base/content/browser.js:8268
(Diff revision 2)
>    return false;
>  }
>  
>  var RestoreLastSessionObserver = {
>    init() {
> +    Services.telemetry.scalarSet("browser.session.restore.browser_tabs_restorebutton", Services.prefs.getIntPref("browser.tabs.restorebutton"));

Nit: just store the pref value in a local variable to avoid fetching it twice.

::: browser/base/content/browser.js:8279
(Diff revision 2)
>          let restoreTabsButtonWrapper = restoreTabsButton.parentNode;
>          restoreTabsButtonWrapper.setAttribute("session-exists", "true");
>          gBrowser.tabContainer.updateSessionRestoreVisibility();
>          restoreTabsButton.style.maxWidth = `${restoreTabsButtonWrapperWidth}px`;
>          gBrowser.tabContainer.addEventListener("TabOpen", this);
> +        Services.telemetry.scalarSet("browser.session.restore.tabbar_restore_available", true);

I think this should also be called when the button is not available (with the value `false`), right?

::: browser/base/content/browser.js:8284
(Diff revision 2)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
> +      Services.obs.addObserver(this, "sessionstore-windows-restored", true);
> +      Services.obs.addObserver(this, "sessionstore-browser-state-restored", true);

Sorry, I'm confused about what the point of these additional observers is in the current scheme of things. I think they will all fire observe() and then remove only the last-session-cleared observer, but not the other ones. That doesn't seem right. What exactly is this trying to do? Is it fixing some other bug, maybe? It's not clear to me off-hand (maybe just because it's late and I'm tired). :-)

Generally, I guess this is hairy because we need to record some things independent of whether we have just auto-restored a session, or if we're offering the button to restore things, and when the user restores through some other means than the button. But I kind of assume that the scalars you're adding directly in sessionstore as well as the scalars you're adding from the init() method at the top cover this (apart from the restore_available one that I mentioned).

::: browser/base/content/browser.js:8313
(Diff revision 2)
>      restoreTabsButtonWrapper.removeAttribute("session-exists");
>      restoreTabsButton.style.maxWidth = 0;
>      gBrowser.tabContainer.removeEventListener("TabOpen", this);
>    },
>  
> -  observe() {
> +  observe(aSubject, aTopic, aData) {

These args are unused right now, so this can stay as observe() if we don't need to distinguish topics. :-)

::: browser/components/sessionstore/SessionStore.jsm:3410
(Diff revision 2)
> +      Services.telemetry.scalarAdd("browser.session.restore.number_of_tabs", winData.tabs.length);
> +      Services.telemetry.scalarAdd("browser.session.restore.number_of_win", 1);

This looks like it's hardcoded to 1 window (which seems like it's definitely wrong - you can restore multiple windows), and will record the number of tabs per window for each window. Is that what you want, or do you want the total number of tabs in one value? Perhaps this needs to live elsewhere in sessionstore.jsm ?

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:10
(Diff revision 2)
>  from marionette_harness import MarionetteTestCase
>  from marionette_driver import Wait
>  
>  
>  class TestBaseTabbarSessionRestoreButton(PuppeteerMixin, MarionetteTestCase):
> -    def setUp(self, prefValue=True):
> +    def setUp(self, restore_button_pref=1):

Neat changes to the test here, nice!
Attachment #8885441 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review164886

> I think this should also be called when the button is not available (with the value `false`), right?

I thought about that and decided it was unecessary. Since it will still send info on whether the user is a part of our test group or not. And it will send info on if the user has restored.

> Sorry, I'm confused about what the point of these additional observers is in the current scheme of things. I think they will all fire observe() and then remove only the last-session-cleared observer, but not the other ones. That doesn't seem right. What exactly is this trying to do? Is it fixing some other bug, maybe? It's not clear to me off-hand (maybe just because it's late and I'm tired). :-)
> 
> Generally, I guess this is hairy because we need to record some things independent of whether we have just auto-restored a session, or if we're offering the button to restore things, and when the user restores through some other means than the button. But I kind of assume that the scalars you're adding directly in sessionstore as well as the scalars you're adding from the init() method at the top cover this (apart from the restore_available one that I mentioned).

this one is my bad :(
leftovers from testing

> These args are unused right now, so this can stay as observe() if we don't need to distinguish topics. :-)

also, oops

> This looks like it's hardcoded to 1 window (which seems like it's definitely wrong - you can restore multiple windows), and will record the number of tabs per window for each window. Is that what you want, or do you want the total number of tabs in one value? Perhaps this needs to live elsewhere in sessionstore.jsm ?

what's happening here with `scalarAdd` is that it is adding 1 each time this code runs, and this code runs once per restored window.
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review164886

> I don't see something recording the source of the restore

unfortunately I can't do that. I can record if it was from us, or if it was not from us - which is what I have done. I suppose I could find all the ways to restore and send a message for each of those ways, or send additional data to the restore event from each call to it, but there are a lot...
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156290/#review165938

::: browser/base/content/browser.js:8325
(Diff revision 3)
>    return false;
>  }
>  
>  var RestoreLastSessionObserver = {
>    init() {
> +    let browser_tabs_restorebutton_pref = Services.prefs.getIntPref("browser.tabs.restorebutton");

:gijs just so you know, this was actually the portion I was asking about during our slack messaging.
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review166284

OK, if you're confident this gets you the data you want, r=me on the implementation of it. Note that you will still need to request data-review on the scalar additions. :-)

::: browser/components/sessionstore/SessionStore.jsm:3418
(Diff revision 3)
>      // set smoothScroll back to the original value
>      tabstrip.smoothScroll = smoothScroll;
>  
>      TelemetryStopwatch.finish("FX_SESSION_RESTORE_RESTORE_WINDOW_MS");
> +    if (Services.prefs.getIntPref("browser.tabs.restorebutton") != 0 ) {
> +      Services.telemetry.scalarAdd("browser.session.restore.number_of_tabs", winData.tabs.length);

Oh, gah, so I should read better, for scalarAdd vs. scalarSet.

I think this works, then, though in theory telemetry could subsession split at midnight according to the local user's clock if that happens between restoring 2 windows. Not really worth complicating this code over though, given how few people I expect do restores, and then multi-window restores, and then time it exactly right.
Attachment #8885441 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review166284

Thank you! Trying to arrange a data-review now :)
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

Let's ask Chenxia for data-review… :)
Attachment #8885441 - Flags: review?(liuche)
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

https://reviewboard.mozilla.org/r/156292/#review178822

Collecting Type 2 interaction data, clearly documented and has a point of contact. r+ (on data-review only)
Attachment #8885441 - Flags: review?(liuche) → review+
Pushed by bwinton@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fe8219b6cdc
Add telemetry probes to measure use of session restore button in tab bar. r=Gijs,liuche
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1219725 added a (preffed-off) session restore button to the toolbar, we would like to use Shield to test it.
[User impact if declined]: We won't be able to get the data we need in the Shield study.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: I don't _think_ so.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: I don't think so.
[Why is the change risky/not risky?]: It only adds some telemetry info.
[String changes made/needed]: None.
Attachment #8885441 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/9fe8219b6cdc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8885441 [details]
Bug 1379226 - Add telemetry probes to measure use of session restore button in tab bar.

Adds a probe, OK from data review. Let's uplift for beta 8.
Attachment #8885441 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Some of these probes were removed in bug 1451709.

See Also: → 1451709
You need to log in before you can comment on or make changes to this bug.