1065285 - Add telemetry for the last maintenance time

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Manu Jain]

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!!

Comment 2

[Marco Bonardo [:mak]]

(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.

Comment 3

[Manu Jain]

Attachment: bug1065285.patch

Patch attached.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Manu Jain]

Attachment: bug_1065285_v2.patch

It passed the tests successfully.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Manu Jain]

Attachment: bug_1065285_v3.patch

Changes made.

Comment 8

[Manu Jain]

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!!

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9687a6f92e7d