952568 - [australis-measuring] Determine if Australis Update experience tab is active

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Holly Habstritt Gaal [:Habber]]

We are currently working on improving the Firefox desktop onboarding experience. This includes First Run and Update pages. Our first focus is the Australis Update experience. (see a video of our prototype, here https://www.youtube.com/watch?v=X54Ur_Cp0io)


We would like to use telemetry to detect if the Australis Update tab is active while a user is interacting with the browser chrome. We want to be able to distinguish interactions that occur while the tab is active vs. those that happen once it has been closed or is inactive.

When talking with Mike and Blake, it also seemed possible to know if the Update tab was closed x minutes ago, for instance. Can you confirm if this is possible?

I can provide a specific URL for this tab before this goes live in Aurora. Our goal is to have this ready for Aurora, but it is likely that it will be ready a couple of weeks into Aurora and not right away.

Comment 1

[Holly Habstritt Gaal [:Habber]]

Hi Mike and Blake, any progress on this?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Holly,

Yes, we're going to assign this to Blair, who's more familiar with the work going on in UITour land.

I volunteer for reviewing duty on these.

Same answer for your other needinfo, so I'll clear that.

-Mike

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

This fixes both this bug and bug 952567, since they're so closely linked.

I'm purposefully *not* recording URLs here - since IMO that's too broad and too specific (especially considering we eventually want to use UITour APIs for things like SUMO). Instead, the page gets the chance to specify it's own unique identifier and we track things based on that. 

For counting interactions with browser chrome, everything is now put into "buckets". The default bucket is "__NORMAL__" (ie, we always have a bucket name) to help with querying the data.

I haven't done this for durations, because there's no clear behaviour for what should happen if a bucket changes when were still timing a duration (also, it wouldn't actually work with UITour anyway - since UITour sessions end as soon as you switch tabs). And I'm not cleanly handling the case where UITour sets a bucket, and then some other area of code sets a bucket, and then unsets it - because we don't need that yet.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Technical details for implementing this in the page: 

The page will want to register it's unique identifier every time it detects the document is visible, via the page visibility API. Like so:

  Mozilla.UITour.registerPageID("my-unique-id");

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey Holly,

So, here's how this patch seems to work:

When the user is viewing a tour, UITelemetry starts putting ALL UI interaction probes into a bucket, keyed off of a unique ID that the tour page provides.

If the user is *not* viewing a tour (it might still be open in a background tab, or it may have been closed down), all UI interaction probes are put back into the default bucket.

Is this going to give you the data that you need? Or did the tour "bucket" need to persist until the end of the session?

> When talking with Mike and Blake, it also seemed possible to know if the Update tab
> was closed x minutes ago, for instance.

Will we still need this?

-Mike

Comment 6

[Holly Habstritt Gaal [:Habber]]

Hi Mike, 

Thanks for explaining. If the user has the tab open, but is not currently viewing the tab, we will still want the interactions to show up in the tour "bucket". One use case we have to support this is: During the tour, we notice many users opening up the Customization tab directly from the menu panel when we are talking about it in the tour. Then they switch back to the tour tab and finish the rest of the steps. 

With how it is implemented now, if I understand correctly, we wouldn't be able to track the customization interactions because these take place in a different tab (and therefore, the tab containing the tour wouldn't be viewed at this time). This is just one example.

We would like to know if the tab exists in the browser at the time of interaction with elements in the browser. Is this possible?

Also, when interactions are being placed in the tour "bucket" are they also being put into the  "__NORMAL__" default bucket? Or is there only one bucket/ID being taken at a time.  


The other item mentioned (about the tab being closed x minutes ago) is a nice to have.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8367857 [details] [diff] [review]
Patch v1

Ok, thanks for explaining that, Holly. I think I have a way of doing each of those without too much trouble.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Holly Habstritt [:Habber] from comment #6)
> Also, when interactions are being placed in the tour "bucket" are they also
> being put into the  "__NORMAL__" default bucket? Or is there only one
> bucket/ID being taken at a time.  

Only one buckets is active at a time - so we never record an event more than once. We can merge the buckets when performing analysis, if need be.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Major changes from the previous revision:

* Supporting buckets for customization durations. I realized (at least for customize mode), that it makes sense to track a duration based on what bucket was active when the duration *started*.
* Added support for "expiring buckets", which are kind of like a countdown timer. Except you specify timing steps, so you can specify non-linear times, and it counts up. So you end up with times X minutes *after* an event. Once the bucket expires, it reverts back to the default bucket. The names of expiring buckets are in the form "bucket_NAME|TIME". For UITour, we further break that down - see below. Rationale for expiring buckets is that it doesn't make sense to keep tracking that a tour page was closed last week.
* When a tour page tab is closed, we start an expiring bucket based in that page's registered ID. Bucket name is in the form "bucket_UITour|PAGEID|closed|TIME". Time steps are 1 min, 3 min, 10 min, 1 hour (already checked with Holly this is good).
* Ditto for tour pages that are make inactive (person switches to another tab). Same time steps, bucket name is in the form "bucket_UITour|PAGEID|inactive|TIME".
* TIME in the above two points is in the form "1m", "3m", "10m", "1h".

Comment 10

[Blake Winton (:bwinton) (:☕️)]

(In reply to Blair McBride [:Unfocused] from comment #9)
> Rationale for expiring buckets is that it
> doesn't make sense to keep tracking that a tour page was closed last week.

Uh…  I believe we want to be able to see how the tour that the user saw affected their behaviour.  Won't not tracking the tour page lose the connection between which tour the user saw, and their future behaviour, or is that tracked in a different bucket?

I'm (hopefully understandably) a little nervous about throwing away data.  ;)

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8371201 [details] [diff] [review]
Patch v1

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

I mostly looked at BrowserUITelemetry.jsm, and just skimmed the rest.

I think this looks good - I just have some style / doc nits, and a question about the ... spread thing.

::: browser/base/content/tabbrowser.xml
@@ +1094,5 @@
>                // We've selected the new tab, so go ahead and notify listeners.
> +              let event = new CustomEvent("TabSelect", {
> +                bubbles: true,
> +                cancelable: false,
> +                detail: {

dev-doc-needed on this?

::: browser/modules/BrowserUITelemetry.jsm
@@ +18,5 @@
>    "resource:///modules/RecentWindow.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
>    "resource:///modules/CustomizableUI.jsm");
>  
> +

Nit - single newlines are probably fine around these.

@@ +161,5 @@
> +// Bucket prefix, for named buckets.
> +const BUCKET_PREFIX = "bucket_";
> +// Separator between the bucket name, and the time step string.
> +const BUCKET_SEPARATOR = "|";
> +

Nit - extra newline.

@@ +595,5 @@
> +
> +  /**
> +  * Sets a bucket that expires at the rate of a given series of time steps.
> +  * Once the bucket expires, the current bucket will automatically revert to
> +  * the default bucket. While the bucket is expiring, it's name is postfixed

TIL postfix means to append a suffix.

@@ +603,5 @@
> +  * still expiring, the old expiring bucket stops expiring and the new bucket
> +  * immediately takes over.
> +  *
> +  * @param aName   Name of bucket.
> +  * @param aTimes  An array of times in illiseconds to count up to before

aTimes -> aTimeSteps

also, "illiseconds" sound pretty cool, but we're probably talking about milliseconds here. :)

@@ +614,5 @@
> +  *                * bucket|5m - for the following 4 minutes
> +  *                              (until 5 minutes after the start)
> +  *                * bucket|10m - for the following 5 minutes
> +  *                               (until 10 minutes after the start)
> +  *                * __NORMAL__ - until a new bucket is set

What about aTimeOffset? Looking at the function, it's probably supposed to be used only during the recursion, but we should probably mention that.

@@ +627,5 @@
> +      clearTimeout(this._bucketTimer);
> +      this._bucketTimer = null;
> +    }
> +
> +    let steps = [...aTimeSteps];

aTimeSteps is already an array, is it not? Why do we have to do the ... thing?

@@ +641,5 @@
> +
> +  /**
> +   * Formats a time interval, in milliseconds, to a minimal non-localized string
> +   * representation. Format is: 'h' for hours, 'm' for minutes, 's' for seconds,
> +   * 'ms' for milliseconds.function

Not sure if function is supposed to be there...

@@ +652,5 @@
> +   * @param aTimeMS  Time in milliseconds
> +   *
> +   * @return Minimal string representation.
> +   */
> +  _toTimeStr: function(aTimeMS) {

This function is really quite elegant. :)

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blake Winton (:bwinton) from comment #10)
> (In reply to Blair McBride [:Unfocused] from comment #9)
> > Rationale for expiring buckets is that it
> > doesn't make sense to keep tracking that a tour page was closed last week.
> 
> Uh…  I believe we want to be able to see how the tour that the user saw
> affected their behaviour.  Won't not tracking the tour page lose the
> connection between which tour the user saw, and their future behaviour, or
> is that tracked in a different bucket?
> 
> I'm (hopefully understandably) a little nervous about throwing away data.  ;)

Oh, we still record that separately inside UITour.jsm itself. Sorry, should have mentioned that.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #11)
> dev-doc-needed on this?

Oh, yes!

The tabbrowser TabSelect event has a new addition. It's not a CustomEvent with a 'detail' property, which has a 'previousTab' property that specifies the tab that was previously selected, but made inactive by this event.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #11)
> What about aTimeOffset? Looking at the function, it's probably supposed to
> be used only during the recursion, but we should probably mention that.

Oh, yes. I could have made it completely private, but I didn't see any downside in exposing it - we may find some legit use for it in the future, and there's certainly no harm in using it.

> aTimeSteps is already an array, is it not? Why do we have to do the ...
> thing?

That's making the copy of the array, so we can safely modify it. Otherwise we end up modifying the array that external code has passed to us - in the case of UITour, it's a global variable we re-use (a pattern I'd expect other code to follow). I'd added a comment to clarify this.

> > +  _toTimeStr: function(aTimeMS) {
> 
> This function is really quite elegant. :)

:)

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8371201 [details] [diff] [review]
Patch v1

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

r=me with more tests

::: browser/modules/BrowserUITelemetry.jsm
@@ +9,5 @@
>  const {interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");

Why not defineLazyModuleGetter? It's not used immediately,

@@ +156,5 @@
>  // lasted.
>  const WINDOW_DURATION_MAP = new WeakMap();
>  
> +// Default bucket name, when no other bucket is active.
> +const BUCKET_DEFAULT = "__NORMAL__";

Nit: I like __DEFAULT__ better and it matches the const name but not a big deal.

@@ +160,5 @@
> +const BUCKET_DEFAULT = "__NORMAL__";
> +// Bucket prefix, for named buckets.
> +const BUCKET_PREFIX = "bucket_";
> +// Separator between the bucket name, and the time step string.
> +const BUCKET_SEPARATOR = "|";

Nit: You could export this value so you don't need to duplicate it in UITour.jsm: 
A) Prefix this const with BROWSER_UI_TELEMETRY_ (or something similar) and add it to EXPORTED_SYMBOLS. 
  or
B) make it a non-const property or getter on BrowserUITelemetry

@@ +616,5 @@
> +  *                * bucket|10m - for the following 5 minutes
> +  *                               (until 10 minutes after the start)
> +  *                * __NORMAL__ - until a new bucket is set
> +  */
> +  setExpiringBucket: function(aName, aTimeSteps, aTimeOffset = 0) {

Some tests for this method (preferably outside UITour ones) would be good.

::: browser/modules/UITour.jsm
@@ +325,5 @@
> +          // Delete this from the window cache, so if the window is closed we
> +          // don't expire this page ID twice.
> +          let window = tab.ownerDocument.defaultView;
> +          if (this.pageIDSourceWindows.get(window) == pageID)
> +            this.pageIDSourceWindows.delete(window);

I think there is an issue if a user has multiple tour tabs open with the same page ID (likely more common after bug 966933) but I think that's an edge case that we probably don't need to worry much about.

::: browser/modules/test/browser_UITour_registerPageID.js
@@ +15,5 @@
> +  });
> +  UITourTest();
> +}
> +
> +let tests = [

Can you add tests to check the various values of BrowserUITelemetry.currentBucket? e.g. for closed/inactive/default

Comment 16

[Matthew N. [:MattN]]

P3 since bug 952567 is.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #15)
> Why not defineLazyModuleGetter? It's not used immediately,

Because it doesn't support modules that export multiple symbols :( Bug 732385. But I'll fake it with defineLazyGetter.

> B) make it a non-const property or getter on BrowserUITelemetry

Done.

> Some tests for this method (preferably outside UITour ones) would be good.

Done.

> I think there is an issue if a user has multiple tour tabs open with the
> same page ID (likely more common after bug 966933) but I think that's an
> edge case that we probably don't need to worry much about.

Oh, hm, yea. Not sure if there's a simple way of dealing with that. Left it for a followup bug for now - bug 969221. If need be, we can revisit when we do bug 966933.


> Can you add tests to check the various values of
> BrowserUITelemetry.currentBucket? e.g. for closed/inactive/default

Done.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1

This addresses all review comments, and adds additional testing. Carrying forward review.


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

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8372029 [details] [diff] [review]
Patch v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None - no user-facing change. This is purely for collecting telemetry data.
Testing completed (on m-c, etc.): Automated tests on fx-team
Risk to taking this patch (and alternatives if risky): Should be minimal. Hasn't had much field testing for the telemetry data... but ideally we want the telemetry data rolling in on the first Aurora update.
String or IDL/UUID changes made by this patch: None

Comment 20

[Justin Dolske [:Dolske]]

Comment on attachment 8372029 [details] [diff] [review]
Patch v2.1

bukket=me

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4d8ec13d060