679937 - report profile age in telemetry

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

mak suggested SELECT dateAdded FROM moz_bookmarks WHERE id = 1 as a good way to approximate profile age. Note this only goes back to 2008(release of ff3), should be good enough for metrics(i hope).

Comment 1

["Saptarshi Guha[:joy]"]

I would say yes.

Comment 2

[(dormant account)]

(In reply to Saptarshi Guha from comment #1)
> I would say yes.

What sort of precision do you want? Hours, days, months, years?

Comment 3

["Saptarshi Guha[:joy]"]

day level is more than enough. If issues this is the hierarchy:

days
week
month
year (not so good)

Comment 4

[(dormant account)]

Sid, how do you feel about this from privacy standpoint? I think days would be nice since it would notify us if users are abandoning firefox right away or after some usage

Comment 5

[(dormant account)]

Attachment: profile age

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 556138 [details] [diff] [review]
profile age

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

some drive-by comments

::: toolkit/components/telemetry/TelemetryPing.js
@@ +338,5 @@
> +    // populate histograms one last time
> +    this.gatherMemory();
> +
> +    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> +    let stmt = db.createStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");

use createAsyncStatement

@@ +339,5 @@
> +    this.gatherMemory();
> +
> +    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> +    let stmt = db.createStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");
> +    stmt.executeAsync({

your callback object is missing handleError

@@ +342,5 @@
> +    let stmt = db.createStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");
> +    stmt.executeAsync({
> +                        handleResult: function(aResultSet) {
> +                          let row = aResultSet.getNextRow();
> +                          // sqlite stores dates in microseconds

it's not sqlite, it's Places that is crazy.

@@ +348,5 @@
> +                        },
> +
> +                        handleCompletion: function(aReason) {
> +                          let directoryService = Cc["@mozilla.org/file/directory_service;1"]
> +                            .getService(Ci.nsIProperties);

Services.dirsvc

@@ +357,5 @@
> +                            gProfileTimestamp = lastModifiedTime;
> +                          // proceed to send the data
> +                          self.send(reason, server);
> +                        }
> +                      });

nit: the indentation is fancy here, but it's your module :D

Btw, just after executeAsync() you should invoke stmt.finalize(); (doesn't matter that is has not yet been executed), or you may be leaking the connection.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +56,5 @@
>                                               .decodeFromStream(s, s.available())
>  
>    do_check_eq(request.getHeader("content-type"), "application/json; charset=UTF-8");
> +  do_check_true(payload.simpleMeasurements.uptime >= 0);
> +  do_check_eq(payload.simpleMeasurements.profileAge, 0);

I hope your test harness invokes do_get_profile(), otherwise you are working with a ghost profile (and Places may even not shutdown properly)

Comment 7

[(dormant account)]

Thanks for the review! I was too shy to r? you since it only had a few lines of stuff you like to review.

> @@ +339,5 @@
> > +    this.gatherMemory();
> > +
> > +    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> > +    let stmt = db.createStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");
> > +    stmt.executeAsync({
> 
> your callback object is missing handleError

I wasn't obvious how it'd be useful, so I took it out. Is it useful for anything? All I want to know is when the query completed. Whether places db is corrupt or has no bookmarks is irrelevant. Am I wrong here?

> 
> @@ +342,5 @@
> > +    let stmt = db.createStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");
> > +    stmt.executeAsync({
> > +                        handleResult: function(aResultSet) {
> > +                          let row = aResultSet.getNextRow();
> > +                          // sqlite stores dates in microseconds
> 
> it's not sqlite, it's Places that is crazy.

:( Good to know.

> 
> @@ +348,5 @@
> > +                        },
> > +
> > +                        handleCompletion: function(aReason) {
> > +                          let directoryService = Cc["@mozilla.org/file/directory_service;1"]
> > +                            .getService(Ci.nsIProperties);
> 
> Services.dirsvc

Thanks! Can you tell I'm new at this? :)
> 
> nit: the indentation is fancy here, but it's your module :D
> 
> Btw, just after executeAsync() you should invoke stmt.finalize(); (doesn't
> matter that is has not yet been executed), or you may be leaking the
> connection.

Ok.
> 
> I hope your test harness invokes do_get_profile(), otherwise you are working
> with a ghost profile (and Places may even not shutdown properly)

It does, though I didn't know that was needed.

Comment 8

[(dormant account)]

Comment on attachment 556138 [details] [diff] [review]
profile age

mak r-ed this

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Taras Glek (:taras) from comment #7)
> I wasn't obvious how it'd be useful, so I took it out. Is it useful for
> anything? All I want to know is when the query completed. Whether places db
> is corrupt or has no bookmarks is irrelevant. Am I wrong here?

no, it's not useful for your case but xpcom will warn that you are missing it. there should be a bug filed about that xpcom craziness.

> > it's not sqlite, it's Places that is crazy.
> 
> :( Good to know.

fwiw for our needs seconds precision would be fine, indeed in a future schema I'd like to cut all timestamps to seconds to save space. I'm not sure why someone in the past decided we want microseconds precision in history.

> > I hope your test harness invokes do_get_profile(), otherwise you are working
> > with a ghost profile (and Places may even not shutdown properly)
> 
> It does, though I didn't know that was needed.

do_get_profile() creates the actual profile and sends notification about creation and destruction (mostly profile-after-change, profile-before-change and so on).

Comment 10

[(dormant account)]

Attachment: profile age

addressed mak's comments

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 556180 [details] [diff] [review]
profile age

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +205,5 @@
>             .telemetryValue;
> +  
> +  // Interval in ms -> days
> +  if (gProfileTimestamp != -1)
> +    ret.profileAge = Math.floor((new Date() - gProfileTimestamp) / 1000 / 60 / 60 / 24);

/ 3600 instead of / 60 / 60 would be understandable, too. I'd be tempted to make that 86400 together with / 24, but maybe I'm to used to DNS TTLs and as such immediately associate 86400 with 24 hours.

@@ +338,5 @@
> +    // populate histograms one last time
> +    this.gatherMemory();
> +
> +    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> +    let stmt = db.createAsyncStatement("SELECT dateAdded FROM moz_bookmarks WHERE id = 1");

Please add a comment why this works as a metric for the places database age: bookmark with id=1 is the root node of the bookmark folder, is created when the profile is created, and is timestamped then. It also always exists (no risk of it being deleted, at least not until major changes to the bookmarking system).

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mike Hommey [:glandium] from comment #11)
> / 3600 instead of / 60 / 60 would be understandable, too. I'd be tempted to
> make that 86400 together with / 24, but maybe I'm to used to DNS TTLs and as
> such immediately associate 86400 with 24 hours.

fwiw you may just have a const SECONDS_PER_DAY = 86400, then it's pretty damn readable.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Removing the privacy review keyword, since taras suggested this may no longer ship.  Please re-flag if we decide to deploy this metric.

Comment 14

[(dormant account)]

We decided we no longer need this