Closed
Bug 1065925
Opened 10 years ago
Closed 10 years ago
Remove code for some of the Places telemetry probes
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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!!
Reporter | ||
Comment 2•10 years ago
|
||
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).
probe PLACES_FOLDER_COUNT patch.
Attachment #8488036 -
Flags: review?(mak77)
probe BACKUPS_HASHING_MS patch
Attachment #8488044 -
Flags: review?(mak77)
probe database_journalsize_mb patch
Attachment #8488050 -
Flags: review?(mak77)
probe PLACES_ANNOS_BOOKMARKS_SIZE_KB patch
Attachment #8488054 -
Flags: review?(mak77)
probe PLACES_ANNOS_PAGES_SIZE_KB patch
Attachment #8488060 -
Flags: review?(mak77)
probe PLACES_FRECENCY_CALC_TIME_MS patch.
Attachment #8488066 -
Flags: review?(mak77)
Reporter | ||
Comment 9•10 years ago
|
||
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+
Reporter | ||
Comment 10•10 years ago
|
||
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-
Reporter | ||
Updated•10 years ago
|
Attachment #8488050 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8488054 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8488060 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 11•10 years ago
|
||
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+
Reporter | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
Ok, I'll make changes which you told. Can you assign this bug to me. Thanks!!
Reporter | ||
Comment 14•10 years ago
|
||
(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
Assignee | ||
Comment 15•10 years ago
|
||
I've made the changes.
Attachment #8489483 -
Flags: review?(mak77)
Assignee | ||
Comment 16•10 years ago
|
||
I've removed include statement.
Attachment #8488066 -
Attachment is obsolete: true
Attachment #8489489 -
Flags: review?(mak77)
Reporter | ||
Comment 17•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8489489 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 18•10 years ago
|
||
patch rebased.
Attachment #8488036 -
Attachment is obsolete: true
Attachment #8490028 -
Flags: review?(mak77)
Assignee | ||
Comment 19•10 years ago
|
||
patch rebased.
Attachment #8488044 -
Attachment is obsolete: true
Attachment #8489483 -
Attachment is obsolete: true
Attachment #8490029 -
Flags: review?(mak77)
Assignee | ||
Comment 20•10 years ago
|
||
patch rebased.
Attachment #8488050 -
Attachment is obsolete: true
Attachment #8490030 -
Flags: review?(mak77)
Assignee | ||
Comment 21•10 years ago
|
||
patch rebased.
Attachment #8488054 -
Attachment is obsolete: true
Attachment #8490031 -
Flags: review?(mak77)
Reporter | ||
Updated•10 years ago
|
Attachment #8490028 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 22•10 years ago
|
||
patch rebased.
Attachment #8488060 -
Attachment is obsolete: true
Attachment #8490032 -
Flags: review?(mak77)
Reporter | ||
Updated•10 years ago
|
Attachment #8490029 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 23•10 years ago
|
||
patch rebased.
Attachment #8489489 -
Attachment is obsolete: true
Attachment #8490034 -
Flags: review?(mak77)
Reporter | ||
Updated•10 years ago
|
Attachment #8490030 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8490031 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8490032 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8490034 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Are the tests passed?
Reporter | ||
Comment 25•10 years ago
|
||
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.
Reporter | ||
Comment 26•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(mak77)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(mak77)
Keywords: checkin-needed
Comment 27•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 28•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•