Closed
Bug 679937
Opened 13 years ago
Closed 12 years ago
report profile age in telemetry
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: taras.mozilla, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Comment 2•13 years ago
|
||
(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•13 years ago
|
||
day level is more than enough. If issues this is the hierarchy: days week month year (not so good)
Reporter | ||
Comment 4•13 years ago
|
||
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 6•13 years ago
|
||
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)
Updated•13 years ago
|
Keywords: privacy-review-needed
Reporter | ||
Comment 7•13 years ago
|
||
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.
Reporter | ||
Comment 8•13 years ago
|
||
Comment on attachment 556138 [details] [diff] [review] profile age mak r-ed this
Attachment #556138 -
Flags: review?(mh+mozilla) → review-
Comment 9•13 years ago
|
||
(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).
Reporter | ||
Comment 10•13 years ago
|
||
addressed mak's comments
Attachment #556138 -
Attachment is obsolete: true
Attachment #556180 -
Flags: review?(mh+mozilla)
Comment 11•13 years ago
|
||
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).
Attachment #556180 -
Flags: review?(mh+mozilla) → review+
Comment 12•13 years ago
|
||
(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•13 years ago
|
||
Removing the privacy review keyword, since taras suggested this may no longer ship. Please re-flag if we decide to deploy this metric.
Keywords: privacy-review-needed
Reporter | ||
Comment 14•12 years ago
|
||
We decided we no longer need this
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•