Remove code for some of the Places telemetry probes

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: Manu Jain, Mentored)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 14 obsolete attachments)

12.34 KB, patch
mak
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
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

3 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).
(Assignee)

Comment 3

3 years ago
Created attachment 8488036 [details] [diff] [review]
folder_count.patch

probe PLACES_FOLDER_COUNT patch.
Attachment #8488036 - Flags: review?(mak77)
(Assignee)

Comment 4

3 years ago
Created attachment 8488044 [details] [diff] [review]
backups_hashing_ms.patch

probe BACKUPS_HASHING_MS patch
Attachment #8488044 - Flags: review?(mak77)
(Assignee)

Comment 5

3 years ago
Created attachment 8488050 [details] [diff] [review]
database_journalsize_mb.patch

probe database_journalsize_mb patch
Attachment #8488050 - Flags: review?(mak77)
(Assignee)

Comment 6

3 years ago
Created attachment 8488054 [details] [diff] [review]
annos_bookmarks_size_kb.patch

probe PLACES_ANNOS_BOOKMARKS_SIZE_KB patch
Attachment #8488054 - Flags: review?(mak77)
(Assignee)

Comment 7

3 years ago
Created attachment 8488060 [details] [diff] [review]
annos_pages_size_kb.patch

probe PLACES_ANNOS_PAGES_SIZE_KB patch
Attachment #8488060 - Flags: review?(mak77)
(Assignee)

Comment 8

3 years ago
Created attachment 8488066 [details] [diff] [review]
frecency_calc_time_ms.patch

probe PLACES_FRECENCY_CALC_TIME_MS patch.
Attachment #8488066 - Flags: review?(mak77)
(Reporter)

Comment 9

3 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

3 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

3 years ago
Attachment #8488050 - Flags: review?(mak77) → review+
(Reporter)

Updated

3 years ago
Attachment #8488054 - Flags: review?(mak77) → review+
(Reporter)

Updated

3 years ago
Attachment #8488060 - Flags: review?(mak77) → review+
(Reporter)

Comment 11

3 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

3 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

3 years ago
Ok, I'll make changes which you told. Can you assign this bug to me. Thanks!!
(Reporter)

Comment 14

3 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

3 years ago
Created attachment 8489483 [details] [diff] [review]
backups_hashing_ms_new.patch

I've made the changes.
Attachment #8489483 - Flags: review?(mak77)
(Assignee)

Comment 16

3 years ago
Created attachment 8489489 [details] [diff] [review]
frecency_calc_time_ms_new.patch

I've removed include statement.
Attachment #8488066 - Attachment is obsolete: true
Attachment #8489489 - Flags: review?(mak77)
(Reporter)

Comment 17

3 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

3 years ago
Attachment #8489489 - Flags: review?(mak77) → review+
(Assignee)

Comment 18

3 years ago
Created attachment 8490028 [details] [diff] [review]
FOLDERS_COUNT_v2.patch

patch rebased.
Attachment #8488036 - Attachment is obsolete: true
Attachment #8490028 - Flags: review?(mak77)
(Assignee)

Comment 19

3 years ago
Created attachment 8490029 [details] [diff] [review]
BACKUPS_HASHING_MS_v2.patch

patch rebased.
Attachment #8488044 - Attachment is obsolete: true
Attachment #8489483 - Attachment is obsolete: true
Attachment #8490029 - Flags: review?(mak77)
(Assignee)

Comment 20

3 years ago
Created attachment 8490030 [details] [diff] [review]
DATABASE_JOURNALSIZE_MB_v2.patch

patch rebased.
Attachment #8488050 - Attachment is obsolete: true
Attachment #8490030 - Flags: review?(mak77)
(Assignee)

Comment 21

3 years ago
Created attachment 8490031 [details] [diff] [review]
ANNOS_BOOKMARKS_SIZE_KB_v2.patch

patch rebased.
Attachment #8488054 - Attachment is obsolete: true
Attachment #8490031 - Flags: review?(mak77)
(Reporter)

Updated

3 years ago
Attachment #8490028 - Flags: review?(mak77) → review+
(Assignee)

Comment 22

3 years ago
Created attachment 8490032 [details] [diff] [review]
ANNOS_PAGES_SIZE_KB_v2.patch

patch rebased.
Attachment #8488060 - Attachment is obsolete: true
Attachment #8490032 - Flags: review?(mak77)
(Reporter)

Updated

3 years ago
Attachment #8490029 - Flags: review?(mak77) → review+
(Assignee)

Comment 23

3 years ago
Created attachment 8490034 [details] [diff] [review]
FRECENCY_CALC_TIME_MS_v2.patch

patch rebased.
Attachment #8489489 - Attachment is obsolete: true
Attachment #8490034 - Flags: review?(mak77)
(Reporter)

Updated

3 years ago
Attachment #8490030 - Flags: review?(mak77) → review+
(Reporter)

Updated

3 years ago
Attachment #8490031 - Flags: review?(mak77) → review+
(Reporter)

Updated

3 years ago
Attachment #8490032 - Flags: review?(mak77) → review+
(Reporter)

Updated

3 years ago
Attachment #8490034 - Flags: review?(mak77) → review+
(Assignee)

Comment 24

3 years ago
Are the tests passed?
(Reporter)

Comment 25

3 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

3 years ago
Created attachment 8497587 [details] [diff] [review]
merged patch

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

3 years ago
Flags: needinfo?(mak77)
(Reporter)

Updated

3 years ago
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
Last Resolved: 3 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.