620837 - Modify blocklist pingCount parameter to track pingCount for profile and for version

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

With the pingCount added in bug 597247, we can track the retention and age for installations of a particular version, but since the counter resets, we have no sense of the retention for the product as a whole.

While we could use the blocklist cookie to calculate this measure, I want to completely eliminate any need for the cookie, so we need to be able to measure the product level retention as well.

The easiest way to do so would be to have the pingCount report both numbers.  The existing one that resets on every app change event, and one that doesn't reset.

While it would be nice to be able to backfill this new number by calculating the days since the profile was created, it sounds like this isn't a timestamp we currently keep, so just having the total pingCount start at whatever the version pingCount number is would be acceptable.

I also created bug 616835 which provides a cleaner way for us to be able to count unique installations for an arbitrary time range, but that feature is orthogonal to this one.

Comment 1

[Robert Strong (they/them - no direct email)]

Since pingCounts don't happen on a daily basis are you sure this would be adequate? We could use the profile dir's creation time which would be accurate for the most part or create a pref with the current date when the pref doesn't exist. We could do a combination of the two by storing the profile dir's creation date in a pref too.

Comment 2

[Daniel Einspanjer [:dre] [:deinspanjer]]

Having a pingCount rather than days since profile creation is actually preferred at this point.  I just had a call with Blake to verify that statement.  pingCount effectively translates to "days of active usage" whereas the days since creation includes days where the user was completely inactive.

For the retention measures we are using, active usage is the important factor.  Further, if we were to introduce the reporting of profile age, that would be data that we do not currently have access to with the blocklist cookie, so I'd rather save a measurement like that for a new system that has gone through the vetting process of an open community discussion.

tl;dr: yes, we want the simple pingCount rather than any calculation of profile creation date for this feature request.

Comment 3

[Robert Strong (they/them - no direct email)]

Just to verify with this bug and bug implemented there will be:

1. pingCount = pings since new or last reset and will always start at 1 for the first ping for readability since that is somewhat of concern (e.g. pingCount=1 equals 1 ping vs. pingCount=0 equals 1 ping).

2. totalPingCount = count of all pings and will start at 1 unless pingCount already exists in which case it will start at the number represented by pingCount.

3. daysSinceLast = "new" when this is the first ping, "reset" when the count is reset, and an integer representing the number of days rounded down since the last ping.

note: the above is per profile

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #3)
> Just to verify with this bug and bug implemented there will be:
...and bug bug 616835...

Comment 5

[Daniel Einspanjer [:dre] [:deinspanjer]]

This sounds like an accurate summary of the logic to me.
Just to clarify, for item #3, "number of days rounded down since last ping" means that if for some reason it has been less than 24 hours since the last ping, it would round down to 0.  I know that is what you said, but I just want to be clear since the other count is being changed to never have zeros. :)  No need to reply unless I somehow got it wrong.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch 1

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 502928 [details] [diff] [review]
patch 1

Looks good, wants a test though

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: test patch rev1

Daniel, since it is possible that the last update time doesn't exist I made the daysSinceLastPing parameter return invalid for that case.

Comment 9

[Daniel Einspanjer [:dre] [:deinspanjer]]

Comment on attachment 503609 [details] [diff] [review]
test patch rev1

># HG changeset patch
># Parent 0427e1c057250158a9187c18d7240ee10aeb7ec3
>
>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>@@ -436,18 +436,22 @@ Blocklist.prototype = {
>       daysSinceLastPing = pingCount == 0 ? "new" : "reset";

Here, we set daysSinceLastPing to the string "new" or "reset" in the case of first ever ping or first ping after install

>       pingCount = 1;
>     }
>     else {
>       // Seconds in one day is used because nsIUpdateTimerManager stores the
>       // last update time in seconds.
>       let secondsInDay = 60 * 60 * 24;
>       let lastUpdateTime = getPref("getIntPref", PREF_BLOCKLIST_LASTUPDATETIME, 0);
>-      let now = Math.round(Date.now() / 1000);
>-      daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
>+      if (lastUpdateTime != 0) {
>+        let now = Math.round(Date.now() / 1000);
>+        daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
>+      }
>+      else
>+        daysSinceLastPing = "invalid";

Why do we need to use invalid here instead of "new"?  What is the difference in context?


>     }
> 
>     dsURI = dsURI.replace(/%APP_ID%/g, gApp.ID);
>     dsURI = dsURI.replace(/%APP_VERSION%/g, gApp.version);
>     dsURI = dsURI.replace(/%PRODUCT%/g, gApp.name);
>     dsURI = dsURI.replace(/%VERSION%/g, gApp.version);
>     dsURI = dsURI.replace(/%BUILD_ID%/g, gApp.appBuildID);
>     dsURI = dsURI.replace(/%BUILD_TARGET%/g, gApp.OS + "_" + gABI);
>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bug620837.js b/toolkit/mozapps/extensions/test/xpcshell/test_bug620837.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bug620837.js
>@@ -0,0 +1,84 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+do_load_httpd_js();
>+
>+const PREF_BLOCKLIST_LASTUPDATETIME = "app.update.lastUpdateTime.blocklist-background-update-timer";
>+const PREF_BLOCKLIST_PINGCOUNT      = "extensions.blocklist.pingCount";
>+
>+const SECONDS_IN_DAY = 60 * 60 * 24;
>+
>+var gExpectedQueryString = null;
>+var gNextTest = null;
>+var gTestserver = null;
>+
>+function notify_blocklist() {
>+  var blocklist = AM_Cc["@mozilla.org/extensions/blocklist;1"].
>+                  getService(AM_Ci.nsITimerCallback);
>+  blocklist.notify(null);
>+}
>+
>+function pathHandler(metadata, response) {
>+  do_check_eq(metadata.queryString, gExpectedQueryString);
>+  gNextTest();
>+}
>+
>+function run_test() {
>+  createAppInfo("[e-mail]", "XPCShell", "1", "1");
>+
>+  gTestserver = new nsHttpServer();
>+  gTestserver.registerPathHandler("/", pathHandler);
>+  gTestserver.start(4444);
>+
>+  Services.prefs.setCharPref("extensions.blocklist.url",
>+                             "http://localhost:4444/?%PING_COUNT%&%TOTAL_PING_COUNT%&%DAYS_SINCE_LAST_PING%");
>+
>+  do_test_pending();
>+  test1();
>+}
>+
>+function getNowInSeconds() {
>+  return Math.round(Date.now() / 1000);
>+}
>+
>+function test1() {
>+  gNextTest = test2;
>+  gExpectedQueryString = "1&1&new";
>+  notify_blocklist();
>+}
>+
>+function test2() {
>+  gNextTest = test3;
>+  gExpectedQueryString = "2&2&invalid";
>+  notify_blocklist();
>+}
>+
>+function test3() {
>+  Services.prefs.setIntPref(PREF_BLOCKLIST_LASTUPDATETIME,
>+                            (getNowInSeconds() - SECONDS_IN_DAY));
>+  gNextTest = test4;
>+  gExpectedQueryString = "3&3&1";
>+  notify_blocklist();
>+}
>+
>+function test4() {
>+  Services.prefs.setIntPref(PREF_BLOCKLIST_PINGCOUNT, -1);
>+  Services.prefs.setIntPref(PREF_BLOCKLIST_LASTUPDATETIME,
>+                            (getNowInSeconds() - (SECONDS_IN_DAY * 2)));
>+  gNextTest = test5;
>+  gExpectedQueryString = "1&4&reset";
>+  notify_blocklist();
>+}
>+
>+function test5() {
>+  Services.prefs.setIntPref(PREF_BLOCKLIST_LASTUPDATETIME,
>+                            (getNowInSeconds() - (SECONDS_IN_DAY * 3)));
>+  gNextTest = finish;
>+  gExpectedQueryString = "2&5&3";
>+  notify_blocklist();
>+}
>+
>+function finish() {
>+  gTestserver.stop(do_test_finished);
>+}

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to comment #9)
> Comment on attachment 503609 [details] [diff] [review]
> test patch rev1
> 
> ># HG changeset patch
> ># Parent 0427e1c057250158a9187c18d7240ee10aeb7ec3
> >
> >diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
> >--- a/toolkit/mozapps/extensions/nsBlocklistService.js
> >+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
> >@@ -436,18 +436,22 @@ Blocklist.prototype = {
> >       daysSinceLastPing = pingCount == 0 ? "new" : "reset";
> 
> Here, we set daysSinceLastPing to the string "new" or "reset" in the case of
> first ever ping or first ping after install
new means the pingCount pref doesn't exist which is typically a new profile creation to be exact and reset when the app version changes.

> 
> >       pingCount = 1;
> >     }
> >     else {
> >       // Seconds in one day is used because nsIUpdateTimerManager stores the
> >       // last update time in seconds.
> >       let secondsInDay = 60 * 60 * 24;
> >       let lastUpdateTime = getPref("getIntPref", PREF_BLOCKLIST_LASTUPDATETIME, 0);
> >-      let now = Math.round(Date.now() / 1000);
> >-      daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
> >+      if (lastUpdateTime != 0) {
> >+        let now = Math.round(Date.now() / 1000);
> >+        daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
> >+      }
> >+      else
> >+        daysSinceLastPing = "invalid";
> 
> Why do we need to use invalid here instead of "new"?  What is the difference in
> context?
In the instance where the lastUpdateTime for the blocklist doesn't exist. The reason it is needed is that it will be a rather largish number when it doesn't exist.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 503609 [details] [diff] [review]
test patch rev1

>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>@@ -436,18 +436,22 @@ Blocklist.prototype = {
>       daysSinceLastPing = pingCount == 0 ? "new" : "reset";
>       pingCount = 1;
>     }
>     else {
>       // Seconds in one day is used because nsIUpdateTimerManager stores the
>       // last update time in seconds.
>       let secondsInDay = 60 * 60 * 24;
>       let lastUpdateTime = getPref("getIntPref", PREF_BLOCKLIST_LASTUPDATETIME, 0);
>-      let now = Math.round(Date.now() / 1000);
>-      daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
>+      if (lastUpdateTime != 0) {
>+        let now = Math.round(Date.now() / 1000);
>+        daysSinceLastPing = Math.floor((now - lastUpdateTime) / secondsInDay);
>+      }
>+      else
>+        daysSinceLastPing = "invalid";

Wrap the else part in braces too please.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch 2 updated to comments

Comment 13

[Robert Strong (they/them - no direct email)]

Looks like all of the people from applications other than Firefox are already cc'd to this bug. :)

If you'd like to get the same behavior just modify the extensions.blocklist.url pref as shown in attachment #502928 [details] [diff] [review]

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

Comment 15

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central... thanks dholbert
http://hg.mozilla.org/mozilla-central/rev/c9420f27b9dc

Also sent an email to Daniel to be doubly sure metrics picks up this change

Comment 16

[Daniel Einspanjer [:dre] [:deinspanjer]]

Fantastic. thanks guys, I'll look for the data in tomorrow's nightlies.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre based on the automated test, which covers the calculation of the ping counts and the days since the last ping.

Manually checked the data we send out:
https://addons.mozilla.org/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/4.0b11pre/Firefox/20110127030333/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/nightly/Darwin%2010.6.0/default/default/3/9/0/