Closed Bug 1065285 Opened 6 years ago Closed 6 years ago

Add telemetry for the last maintenance time

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

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

Details

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

Attachments

(1 file, 2 obsolete files)

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.
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!!
(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.
Attached patch bug1065285.patch (obsolete) — Splinter Review
Patch attached.
Attachment #8494618 - Flags: review?(mak77)
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+
Attached patch bug_1065285_v2.patch (obsolete) — Splinter Review
It passed the tests successfully.
Attachment #8494618 - Attachment is obsolete: true
Attachment #8496378 - Flags: review?(mak77)
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+
Changes made.
Attachment #8496378 - Attachment is obsolete: true
Attachment #8496414 - Flags: review?(mak77)
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 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+
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
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
Closed: 6 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.