Add telemetry for the last maintenance time

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: manu.jain13, Mentored)

Tracking

Trunk
mozilla35
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Based on users bug reports, I'm under the impression we are not running Places Maintenance often enough, that means idle-daily would be slightly broken.

I think it's worth to add to PlacesDBUtils::telemetry a probe that will report days from the last maintenance (can be obtained by some calculation like this http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesCategoriesStarter.js#82)

the telemetry probe could be named PLACES_MAINTENANCE_DAYSFROMLAST, low 7 high 60, 10 buckets, exponential.
(Assignee)

Comment 1

4 years ago
I would like to patch this bug. Can you assign this to me? We need to make changes in these files - 1."/toolkit/components/places/PlacesDBUtils.jsm" 
2."/toolkit/components/places/tests/unit/test_telemetry.js"
3."/toolkit/components/telemetry/Histograms.json"
Is there any other file to which we need to make changes. Thanks!!
(Reporter)

Comment 2

4 years ago
(In reply to Manu Jain from comment #1)
> I would like to patch this bug. Can you assign this to me? We need to make
> changes in these files - 1."/toolkit/components/places/PlacesDBUtils.jsm" 
> 2."/toolkit/components/places/tests/unit/test_telemetry.js"
> 3."/toolkit/components/telemetry/Histograms.json"
> Is there any other file to which we need to make changes. Thanks!!

Hi, we assign the mentored bugs when the first patch is attached

The list of files you mentioned looks correct, for any other needs please feel free to needinfo? me.
(Assignee)

Comment 3

4 years ago
Created attachment 8494618 [details] [diff] [review]
bug1065285.patch

Patch attached.
Attachment #8494618 - Flags: review?(mak77)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8494618 [details] [diff] [review]
bug1065285.patch

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

good, but not ready yet.

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +959,5 @@
>          query:     "SELECT SUM(LENGTH(content))/1024 FROM moz_annos" },
> +
> +      { histogram: "PLACES_MAINTENANCE_DAYSFROMLAST",
> +        callback: function () {
> +          let lastMaintenance = Services.prefs.getIntPref("places.database.lastMaintenance");

the pref might not be set, and in such a case this would throw. I'm surprised test_telemetry.js passes, did you try to run it? it would be safer to preset places.database.lastMaintenance to a past value in the test.

It would be interesting to know how many databases never went through maintenance (and thus have the pref unset), though we don't have a way to calculate the profile age synchronously. I think the simplest way here would be to try/catch the getIntPref and if it throws just return 60 days (the maximum of the histogram).

We'll surely have mistakes in the data, also cause some user might use the profile only once in a while, but I hope on average we'll get useful data.

@@ +961,5 @@
> +      { histogram: "PLACES_MAINTENANCE_DAYSFROMLAST",
> +        callback: function () {
> +          let lastMaintenance = Services.prefs.getIntPref("places.database.lastMaintenance");
> +          let nowSeconds = parseInt(Date.now() / 1000);
> +          return parseInt(nowSeconds - lastMaintenance);

the result here is seconds, while we want days. you should do parseInt((nowSeconds - lastMaintenance) / 86400)

@@ +963,5 @@
> +          let lastMaintenance = Services.prefs.getIntPref("places.database.lastMaintenance");
> +          let nowSeconds = parseInt(Date.now() / 1000);
> +          return parseInt(nowSeconds - lastMaintenance);
> +        }
> +      },      

trailing spaces
Attachment #8494618 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 5

4 years ago
Created attachment 8496378 [details] [diff] [review]
bug_1065285_v2.patch

It passed the tests successfully.
Attachment #8494618 - Attachment is obsolete: true
Attachment #8496378 - Flags: review?(mak77)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8496378 [details] [diff] [review]
bug_1065285_v2.patch

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

Thank you.

Do you need help to push this to Try?

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +964,5 @@
> +            let lastMaintenance = Services.prefs.getIntPref("places.database.lastMaintenance");
> +            let nowSeconds = parseInt(Date.now() / 1000);
> +            return parseInt((nowSeconds - lastMaintenance) / 86400);
> +          } catch (ex) {
> +            return parseInt(60);

no need to parseInt a number, just return 60
Attachment #8496378 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8496414 [details] [diff] [review]
bug_1065285_v3.patch

Changes made.
Attachment #8496378 - Attachment is obsolete: true
Attachment #8496414 - Flags: review?(mak77)
(Assignee)

Comment 8

4 years ago
Marco, I've not gain access to push this to Try. So, can you you do this? Also, can you assign this bug to me. Thanks!!
(Reporter)

Comment 9

4 years ago
Comment on attachment 8496414 [details] [diff] [review]
bug_1065285_v3.patch

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=11b5a1385dfc

once this is green, you can set the checkin-needed keyword to ask for someone to push this.

Thank you for your contribution.
Attachment #8496414 - Flags: review?(mak77) → review+
(Reporter)

Updated

4 years ago
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9687a6f92e7d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good second bug][lang=js] → [good second bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9687a6f92e7d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good second bug][lang=js][fixed-in-fx-team] → [good second bug][lang=js]
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.