1065925 - Remove code for some of the Places telemetry probes

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Manu Jain]

I'm interested in patching this bug. Can you assign this to me? But I need a little help in going forward. Thanks!!

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Manu Jain]

Attachment: folder_count.patch

probe PLACES_FOLDER_COUNT patch.

Comment 4

[Manu Jain]

Attachment: backups_hashing_ms.patch

probe BACKUPS_HASHING_MS patch

Comment 5

[Manu Jain]

Attachment: database_journalsize_mb.patch

probe database_journalsize_mb patch

Comment 6

[Manu Jain]

Attachment: annos_bookmarks_size_kb.patch

probe PLACES_ANNOS_BOOKMARKS_SIZE_KB patch

Comment 7

[Manu Jain]

Attachment: annos_pages_size_kb.patch

probe PLACES_ANNOS_PAGES_SIZE_KB patch

Comment 8

[Manu Jain]

Attachment: frecency_calc_time_ms.patch

probe PLACES_FRECENCY_CALC_TIME_MS patch.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8488036 [details] [diff] [review]
folder_count.patch

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

LGTM, thank you!

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Manu Jain]

Ok, I'll make changes which you told. Can you assign this bug to me. Thanks!!

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Manu Jain]

Attachment: backups_hashing_ms_new.patch

I've made the changes.

Comment 16

[Manu Jain]

Attachment: frecency_calc_time_ms_new.patch

I've removed include statement.

Comment 17

[Marco Bonardo [:mak]]

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

Comment 18

[Manu Jain]

Attachment: FOLDERS_COUNT_v2.patch

patch rebased.

Comment 19

[Manu Jain]

Attachment: BACKUPS_HASHING_MS_v2.patch

patch rebased.

Comment 20

[Manu Jain]

Attachment: DATABASE_JOURNALSIZE_MB_v2.patch

patch rebased.

Comment 21

[Manu Jain]

Attachment: ANNOS_BOOKMARKS_SIZE_KB_v2.patch

patch rebased.

Comment 22

[Manu Jain]

Attachment: ANNOS_PAGES_SIZE_KB_v2.patch

patch rebased.

Comment 23

[Manu Jain]

Attachment: FRECENCY_CALC_TIME_MS_v2.patch

patch rebased.

Comment 24

[Manu Jain]

Are the tests passed?

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[Marco Bonardo [:mak]]

Attachment: merged patch

I merged the patches to simplify landing, and pushed to try.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=081ad02553d9

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/692d4aebd622

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/692d4aebd622