807872 - Telemetry for database counts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

It would be nice to get some telemetry on the number of history/bookmarks/favicons/thumbnails stored in our databases.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 677625 [details] [diff] [review]
Patch

Gian-Carlo should take a pass at this since he has done some Telemetry work already

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated with comments

Comment 6

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e659528b5e9a

Comment 8

[Wesley Johnston (:wesj)]

Missed the description change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f71e3fc81b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e659528b5e9a
https://hg.mozilla.org/mozilla-central/rev/90f71e3fc81b