Telemetry for database counts

RESOLVED FIXED in Firefox 19

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 19
x86
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
It would be nice to get some telemetry on the number of history/bookmarks/favicons/thumbnails stored in our databases.
(Assignee)

Comment 1

6 years ago
Created attachment 677625 [details] [diff] [review]
Patch

This will fire when Telemetry:Gather is fired in Gecko (which fires on "idle-daily", which should fire for us unless I'm missing something).

I don't quite feel like BrowserApp is the right place for it, but we need somewhere we can get a ContentResolver and we've been trying to not do that in BrowserDB.
Attachment #677625 - Flags: review?(mark.finkle)
Comment on attachment 677625 [details] [diff] [review]
Patch

Gian-Carlo should take a pass at this since he has done some Telemetry work already
Attachment #677625 - Flags: review?(gpascutto)
Comment on attachment 677625 [details] [diff] [review]
Patch

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

::: mobile/android/base/BrowserApp.java
@@ +430,5 @@
>                              menu.findItem(R.id.settings).setEnabled(true);
>                      }
>                  });
> +            } else if (event.equals("Telemetry:Gather")) {
> +                Telemetry.HistogramAdd("PLACES_PAGES_COUNT", BrowserDB.getCount(getContentResolver(), "history"));

You should define your own Telemetry probes, it's explained in Telemetry.java where to do so (Histograms.json). Here you're overriding one from XUL Places, which is bound to generate confusion when generating statistics over it, aside from being wrong ("PAGES" refers to SQLite pages, which is not what you measure here for history, so the telemetry explanation will be wrong, too).

Fennec telemetry should have a FENNEC_ prefix, you can make them compiled out in desktop, see Histograms.json.

@@ +432,5 @@
>                  });
> +            } else if (event.equals("Telemetry:Gather")) {
> +                Telemetry.HistogramAdd("PLACES_PAGES_COUNT", BrowserDB.getCount(getContentResolver(), "history"));
> +                Telemetry.HistogramAdd("PLACES_BOOKMARKS_COUNT", BrowserDB.getCount(getContentResolver(), "bookmarks"));
> +                Telemetry.HistogramAdd("PLACES_FAVICONS_COUNT", BrowserDB.getCount(getContentResolver(), "favicons"));

This doesn't exist.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +180,5 @@
> +        Cursor cursor = null;
> +        int count = 0;
> +        String constraint = null;
> +        try {
> +            Uri uri = mHistoryUriWithProfile;

You're overruling this in every if branch, might as well make it null here, and check for null before firing the query. That should prevent a getCount(cr, "favicon") from returning the history result instead of an error.

@@ +181,5 @@
> +        int count = 0;
> +        String constraint = null;
> +        try {
> +            Uri uri = mHistoryUriWithProfile;
> +            if (database != null) {

No need for this check, you already do your x.equals(null) in reverse below.

@@ +186,5 @@
> +                if ("history".equals(database)) {
> +                    constraint = Combined.VISITS + " > 0";
> +                } else if ("bookmarks".equals(database)) {
> +                    uri = mBookmarksUriWithProfile;
> +                    // ignore folders

This will also ignore separators, livemarks and queries as well. Don't see any particular reason to exclude any of these if we're interested in the amount of data users have?
Attachment #677625 - Flags: review?(gpascutto) → review-
Comment on attachment 677625 [details] [diff] [review]
Patch

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

I'll keep the r- since the missing FAVICONS probe is a real thing that should be addressed and I want to have another look then.

::: mobile/android/base/BrowserApp.java
@@ +430,5 @@
>                              menu.findItem(R.id.settings).setEnabled(true);
>                      }
>                  });
> +            } else if (event.equals("Telemetry:Gather")) {
> +                Telemetry.HistogramAdd("PLACES_PAGES_COUNT", BrowserDB.getCount(getContentResolver(), "history"));

Sorry, I'm completely wrong here. Reusing these should be fine as the meaning of the data is identical.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +186,5 @@
> +                if ("history".equals(database)) {
> +                    constraint = Combined.VISITS + " > 0";
> +                } else if ("bookmarks".equals(database)) {
> +                    uri = mBookmarksUriWithProfile;
> +                    // ignore folders

And here I was wrong too, given that there are separate probes for tags, folders etc. So to keep them comparable you were doing the right thing.
(Assignee)

Comment 5

6 years ago
Created attachment 677830 [details] [diff] [review]
Patch v2

Updated with comments
Attachment #677625 - Attachment is obsolete: true
Attachment #677625 - Flags: review?(mark.finkle)
Attachment #677830 - Flags: review?(gpascutto)
Comment on attachment 677830 [details] [diff] [review]
Patch v2

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

Be sure to test this code, you shouldn't get any JavaScript errors when the telemetry collection runs (this patch would have given them and gotten you backed out).

::: toolkit/components/telemetry/Histograms.json
@@ +1749,5 @@
>      "high": "200",
>      "n_buckets": 10,
>      "description": "PLACES: Number of keywords"
>    },
> +  "FENNEC_FAVICONS_COUNT": {

This doesn't match what you're using in Telemetry.Add above. Add a "cpp_guard":"ANDROID" for probes that only exist in Fennec. Not sure if these should be named FENNEC_ or PLACES_, can't be consistent on both sides unfortunately :-/

@@ +1751,5 @@
>      "description": "PLACES: Number of keywords"
>    },
> +  "FENNEC_FAVICONS_COUNT": {
> +    "kind": "exponential",
> +    "high": "200",

What's a typical number of favicons in the DB? Will that match the expected upper range?

@@ +1759,5 @@
> +  "FENNEC_THUMBNAILS_COUNT": {
> +    "kind": "exponential",
> +    "high": "200",
> +    "n_buckets": 10,
> +    "description": "Fennec: Number of thumbnails stored"

Inconsistent description compared to the previous one. Maybe call em both PLACES: (Fennec) or something.
Attachment #677830 - Flags: review?(gpascutto) → review+
(In reply to Wesley Johnston (:wesj) from comment #8)
> Missed the description change:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/90f71e3fc81b

In the future, you can save resources by adding a DONTBUILD to your commit message so pushes like these aren't triggering a full set of builds/tests on all platforms.
https://hg.mozilla.org/mozilla-central/rev/e659528b5e9a
https://hg.mozilla.org/mozilla-central/rev/90f71e3fc81b
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.