974086 - [australis-measuring] UITour.seenPageIDs should persist across sessions

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

For telemetry analysis, it's better if UITour's seenPageIDs persists across sessions - at the moment it's only kept for the session the ID was seen in.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 2

[Matthew N. [:MattN]]

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

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

I don't think seenPageIDs should persist indefinitely so I'd like a solution for that before r+. This is both for privacy reasons and so the modified pref doesn't linger.

It seems like we want them to persist even after all tour tabs are closed. If not, we could clear them when originTabs is empty. Perhaps they could expire when the milestone changes? I'd rather they didn't pollute the pref file indefinitely but if we think that's fine then we should at least include an expiry date with each ID so that we can cleanup old ones the next time there are used (in restoreSeenPageIDs).

Note: I didn't review the test yet because of the above.

::: browser/modules/UITour.jsm
@@ +25,5 @@
>  
>  
>  const UITOUR_PERMISSION   = "uitour";
>  const PREF_PERM_BRANCH    = "browser.uitour.";
> +const PREF_SEENPAGEIDS    = "browser.uitour.seenPageIDs";

This feels like an abuse of the pref service but it's not the first. I can't think of another persistent storage that is appropriate though (ideally with built-in expiration).

@@ +171,5 @@
>          if (typeof data.pageID == "string" &&
>              !data.pageID.contains(BrowserUITelemetry.BUCKET_SEPARATOR)) {
>            this.seenPageIDs.add(data.pageID);
> +          Services.prefs.setCharPref(PREF_SEENPAGEIDS,
> +                                     JSON.stringify([...this.seenPageIDs]));

Why do we need the spread here?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #2)
> I don't think seenPageIDs should persist indefinitely so I'd like a solution
> for that before r+. This is both for privacy reasons and so the modified
> pref doesn't linger.

The problem it's trying to solve is we want to know how a tour (which variation, or no tour) affects behaviour beyond just the current telemetry session. At the moment, we have no way of answering that.

But, keeping that around *indefinitely* does seem over the top now you mention it. I'll check with the data guru (joy) to make sure expiring over an X time period wouldn't mess things up.


> This feels like an abuse of the pref service but it's not the first. I can't
> think of another persistent storage that is appropriate though (ideally with
> built-in expiration).

Yea, I know :( We need something like bug 866238, but I doubt we'll see that anytime soon.


> > +          Services.prefs.setCharPref(PREF_SEENPAGEIDS,
> > +                                     JSON.stringify([...this.seenPageIDs]));
> 
> Why do we need the spread here?

To convert the Set to an Array - JSON.stringify doesn't handle Sets, unfortunately (grr). Alternative was to use a replacer function, which is just overkill.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Well, in lieu of being able to catch you on IRC...

See first part of comment 2 and first part of comment 3.

Comment 5

["Saptarshi Guha[:joy]"]

I agree we do not want the data forever. This is going into beta and beta has a few million payloads coming in everyday(?)
We want enough payloads from this version. 

Suggestion 1.
For two weeks after this gets merged? Hopefully enough profiles update and use this build and submit payloads back within 2 weeks.
This 'feature' should be  present on all builds released in those 2 weeks. The latter is required because profiles move from build to build  and this 'feature' should expire once they have moved to a build released after the two week period.


Sounds okay?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

2 weeks it is.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8380152 [details] [diff] [review]
Patch v2

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

r=me with the two added UITelemetry.enabled checks. I don't know what our current policies are for privacy review so please figure that out.

::: browser/modules/UITour.jsm
@@ +124,5 @@
>                                           this.getTelemetry.bind(this));
>    },
>  
> +  restoreSeenPageIDs: function() {
> +    delete this.seenPageIDs;

Perhaps should check UITelemetry.enabled here and return with an empty map if it's false as I don't think we should be persisting telemetry data in that case.

@@ +126,5 @@
>  
> +  restoreSeenPageIDs: function() {
> +    delete this.seenPageIDs;
> +
> +    let dateThreshold = Date.now() - SEENPAGEID_EXPIRY;

I just want to note that this cleanup will be triggered when UITelemetry collects its data from us even if the UITour hasn't been used in that session. This is good for cleaning up the pref without the user seeing a new UITour but there is some small overhead. 
This also means that we won't cleanup if a user later disabled telemetry (but it also won't be reported).

@@ +150,5 @@
> +    if (!this.seenPageIDs)
> +      this.seenPageIDs = new Map();
> +
> +    Services.prefs.setCharPref(PREF_SEENPAGEIDS,
> +                               JSON.stringify([...this.seenPageIDs]));

Nit: This doesn't fully clean up after itself when seenPageIDs is empty as the pref will contain "[]". A clearUserPref in this case would be ideal so the pref doesn't exist anymore.

@@ +193,4 @@
>      let tab = window.gBrowser._getTabForContentWindow(contentDocument.defaultView);
>  
>      switch (action) {
>        case "registerPageID": {

I think we should check UITelemetry.enabled here and break if it's false as I don't think we should be collecting/persisting telemetry data in that case. IIRC, registerPageID is only used for telemetry.

::: browser/modules/test/browser_UITour_registerPageID.js
@@ +21,5 @@
>  }
>  
> +function resetSeenPageIDsLazyGetter() {
> +  delete UITour.seenPageIDs;
> +  Object.defineProperty(UITour, "seenPageIDs", {

I can see someone forgetting to update this when the implementation in UITour.init changes and they may spend a while not understanding what was going on. Not keep this in sync means we wouldn't be testing what we're shipping either. I'm not sure what to do about this other than maybe add a comment in init to point this out? It seems kinda dirty that the implementation comment would be referring to a test. It's probably rare enough that we don't need to worry about this.

Comment 8

["Saptarshi Guha[:joy]"]

When does the two week period begin and expire?

Suppose a profile updates to the build that includes this diff.

Suppose the build was released on date D0 but the profile updated to the build on date D1 (e.g. D0 = 20140303 and D1 = 20140307) .
When does the 2 week period start?

1. From D0 (and ends on 0317)
2. From D1 (and ends on 0321)

?

Comment 9

[Matthew N. [:MattN]]

(In reply to "Saptarshi Guha[:joy]" from comment #8)
> When does the 2 week period start?

D1 (from when the tour page uses the registerPageID API). Is that fine?

Comment 10

["Saptarshi Guha[:joy]"]

Thanks. That's what i was hoping for.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1

Also added a check in UITour.addSeenPageID() itself, in case it's used by any external code.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Taras: I had a chat with Irving about this, from a Telemetry point of view. He seemed to think it's fine, but he suggested I get a SR from you (or someone) just in case.

What we're trying to do here is persist UITour Page IDs between sessions in telemetry, for upto 2 weeks. The problem it's trying to solve is we want to know how a tour (which variation, or no tour) affects behaviour beyond just the current telemetry session. At the moment, we have no way of answering that.

The Page IDs are *not* unique per user - they're unique per variation of the tour page they see. eg, we'll do a variation of a tour for people updating to Australis on 29, another variation for people newly installing Firefox 29, possibly another variation for people manually visiting the tour via the Help menu. And occasionally an A/B test on the tour (a random sample up to a certain percentage of all hits on the page). This also means Telemetry knows if a profile hasn't seen a tour page in the past 2 weeks.

The Page IDs are specified by the tour page itself, but we restrict that to whitelisted domains served only over HTTPS.

If Telemetry is disabled, we don't gather Page IDs at all, and we clear the cache of previously seen Page IDs.

Comment 13

[(dormant account)]

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

This seems fine. It's no different than reporting if some UX pref got set.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: None. Impact will be on us: We'll be unable to get information we want out of Telemetry regarding Australis and the update tour.
Testing completed (on m-c, etc.): Unit tests.
Risk to taking this patch (and alternatives if risky): Low. Nothing user-facing.
String or IDL/UUID changes made by this patch: None

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[:Gijs (he/him)]

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

Comment 18

[Catalin Varga [QA][:VarCat]]

Can the QA help with testing on this bug?
If yes, can we get some guidelines?

Comment 19

[Ioana (away)]

Where should one see the UITour.seenPageIDs value?

Comment 20

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

It looks like they're in about:config…

Comment 21

[Catalin Varga [QA][:VarCat]]

I've enabled the telemetry and ran the First run tour.
Checked on about:config and found browser.uitour.seenPageIDs : [["australis-tour-firstrun-29.0",{"lastSeen":1397641198715}]] parameter.
I've closed FF and started another session and checked for browser.uitour.seenPageIDs parameter.
The parameter was kept between sessions.
If there are additional scenarios that I missed please let me know.

I've verified the bug using:

FF 29.0b8
Build Id: 20140414143035
OS: Win 7 x64, Mac Os x 10.9.2, Ubuntu 13.04 x 64

Comment 22

[Catalin Varga [QA][:VarCat]]

I've verified the bug using:

FF 30
Build Id: 20140416004008
OS: Win 7 x64, Mac Os x 10.8, Ubuntu 13.04 x 64