Closed Bug 1065925 Opened 6 years ago Closed 6 years ago

Remove code for some of the Places telemetry probes

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

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

Details

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

Attachments

(1 file, 14 obsolete files)

12.34 KB, patch
mak
: review+
Details | Diff | Splinter Review
The following probes are not measuring useful data and thus are set to expire in 40, we should remove the generating code for them:

PLACES_FOLDERS_COUNT
PLACES_BACKUPS_HASHING_MS
PLACES_DATABASE_JOURNALSIZE_MB
PLACES_ANNOS_BOOKMARKS_SIZE_KB
PLACES_ANNOS_PAGES_SIZE_KB
PLACES_FRECENCY_CALC_TIME_MS
I'm interested in patching this bug. Can you assign this to me? But I need a little help in going forward. Thanks!!
Hi, I guess you already went through https://developer.mozilla.org/en-US/docs/Introduction and you have a working build?

The scope of this bug is obsolete code removal.

you can search in mxr (http://mxr.mozilla.org/mozilla-central/) or dxr (http://dxr.mozilla.org/mozilla-central/source/) for the above probes and remove the code that handles them.

for example:
http://mxr.mozilla.org/mozilla-central/search?string=PLACES_FOLDERS_COUNT
shows that there is some code to be removed from Histograms.json, PlacesDBUtils.jsm and test_telemetry.js

Making a small patch for each probe is probably the easiest path forward. You can use Mercurial queues to manage multiple patches.

For any other question please set a needinfo on me (the form for that is here after the bug comments).
Attached patch folder_count.patch (obsolete) — Splinter Review
probe PLACES_FOLDER_COUNT patch.
Attachment #8488036 - Flags: review?(mak77)
Attached patch backups_hashing_ms.patch (obsolete) — Splinter Review
probe BACKUPS_HASHING_MS patch
Attachment #8488044 - Flags: review?(mak77)
Attached patch database_journalsize_mb.patch (obsolete) — Splinter Review
probe database_journalsize_mb patch
Attachment #8488050 - Flags: review?(mak77)
Attached patch annos_bookmarks_size_kb.patch (obsolete) — Splinter Review
probe PLACES_ANNOS_BOOKMARKS_SIZE_KB patch
Attachment #8488054 - Flags: review?(mak77)
Attached patch annos_pages_size_kb.patch (obsolete) — Splinter Review
probe PLACES_ANNOS_PAGES_SIZE_KB patch
Attachment #8488060 - Flags: review?(mak77)
Attached patch frecency_calc_time_ms.patch (obsolete) — Splinter Review
probe PLACES_FRECENCY_CALC_TIME_MS patch.
Attachment #8488066 - Flags: review?(mak77)
Comment on attachment 8488036 [details] [diff] [review]
folder_count.patch

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

LGTM, thank you!
Attachment #8488036 - Flags: review?(mak77) → review+
Comment on attachment 8488044 [details] [diff] [review]
backups_hashing_ms.patch

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +161,3 @@
>                  .add(Date.now() - startTime);
>        } catch (ex) {
>          Components.utils.reportError("Unable to report telemetry.");

this change is wrong, you should remove "startTime = Date.now()", and the whole try/catch that is trying to report this telemetry probe
Attachment #8488044 - Flags: review?(mak77) → review-
Attachment #8488050 - Flags: review?(mak77) → review+
Attachment #8488054 - Flags: review?(mak77) → review+
Attachment #8488060 - Flags: review?(mak77) → review+
Comment on attachment 8488066 [details] [diff] [review]
frecency_calc_time_ms.patch

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

::: toolkit/components/places/SQLFunctions.cpp
@@ -456,5 @@
>      nsresult rv = aArguments->GetNumEntries(&numEntries);
>      NS_ENSURE_SUCCESS(rv, rv);
>      NS_ASSERTION(numEntries > 0, "unexpected number of arguments");
>  
> -    Telemetry::AutoTimer<Telemetry::PLACES_FRECENCY_CALC_TIME_MS> timer;

Please also remove #include "mozilla/Telemetry.h" from the top of this file.
Attachment #8488066 - Flags: review?(mak77) → feedback+
Please note that most of these patches needs un-bitrot cause the probes were just set to expire in "40" on Trunk. should be easy but will require to update all of the patches to make them pushable.
Ok, I'll make changes which you told. Can you assign this bug to me. Thanks!!
(In reply to manu.jain13 from comment #13)
> Ok, I'll make changes which you told. Can you assign this bug to me. Thanks!!

oops, sure I can!
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
Attached patch backups_hashing_ms_new.patch (obsolete) — Splinter Review
I've made the changes.
Attachment #8489483 - Flags: review?(mak77)
Attached patch frecency_calc_time_ms_new.patch (obsolete) — Splinter Review
I've removed include statement.
Attachment #8488066 - Attachment is obsolete: true
Attachment #8489489 - Flags: review?(mak77)
Comment on attachment 8489483 [details] [diff] [review]
backups_hashing_ms_new.patch

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

it looks good, but please rebase all of the patches against the current tree
Attachment #8489483 - Flags: review?(mak77) → review+
Attachment #8489489 - Flags: review?(mak77) → review+
Attached patch FOLDERS_COUNT_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8488036 - Attachment is obsolete: true
Attachment #8490028 - Flags: review?(mak77)
Attached patch BACKUPS_HASHING_MS_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8488044 - Attachment is obsolete: true
Attachment #8489483 - Attachment is obsolete: true
Attachment #8490029 - Flags: review?(mak77)
Attached patch DATABASE_JOURNALSIZE_MB_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8488050 - Attachment is obsolete: true
Attachment #8490030 - Flags: review?(mak77)
Attached patch ANNOS_BOOKMARKS_SIZE_KB_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8488054 - Attachment is obsolete: true
Attachment #8490031 - Flags: review?(mak77)
Attachment #8490028 - Flags: review?(mak77) → review+
Attached patch ANNOS_PAGES_SIZE_KB_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8488060 - Attachment is obsolete: true
Attachment #8490032 - Flags: review?(mak77)
Attachment #8490029 - Flags: review?(mak77) → review+
Attached patch FRECENCY_CALC_TIME_MS_v2.patch (obsolete) — Splinter Review
patch rebased.
Attachment #8489489 - Attachment is obsolete: true
Attachment #8490034 - Flags: review?(mak77)
Attachment #8490030 - Flags: review?(mak77) → review+
Attachment #8490031 - Flags: review?(mak77) → review+
Attachment #8490032 - Flags: review?(mak77) → review+
Attachment #8490034 - Flags: review?(mak77) → review+
Are the tests passed?
nobody asked to push this to Try, so no :/
Next time please needinfo your mentor when you need him to push patches for you.

I will do now.
Attached patch merged patchSplinter Review
I merged the patches to simplify landing, and pushed to try.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=081ad02553d9
Attachment #8490028 - Attachment is obsolete: true
Attachment #8490029 - Attachment is obsolete: true
Attachment #8490030 - Attachment is obsolete: true
Attachment #8490031 - Attachment is obsolete: true
Attachment #8490032 - Attachment is obsolete: true
Attachment #8490034 - Attachment is obsolete: true
Attachment #8497587 - Flags: review+
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/692d4aebd622
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/692d4aebd622
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.