828546 - Firefox Health Report provider to record Places metrics

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

We have a P2 probe requirement for Firefox Health Report to record some Places counts:

* Total number of pages in history
* Total number of bookmarks

Marco: I was looking through the older patches implementing this and they were obtaining a handle on the Places database and issuing a "SELECT COUNT(*) ..." statement manually. I want your opinion on whether this is acceptable or whether you'd prefer something else - perhaps an explicit getCount() API in PlacesUtils or similar.

Comment 1

[Marco Bonardo [:mak]]

we are already reporting all of that data (and more) to telemetry. The code is here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#823

Comment 2

[Gregory Szorc [:gps]]

Per IRC discussion, the preferred solution is to refactor PlacesDBUtils.jsm so FHR and Telemetry can share code.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1: Add "health report" mode to PlacesDBUtils.telemetry(), v1

This is kinda hacky but it seems to work.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 2: Add Places provider to FHR, v1

I could have put this in PlacesUtils.jsm but decided against it for now.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 708629 [details] [diff] [review]
Part 2: Add Places provider to FHR, v1

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

Good start. Only thing that needs to be addressed in v1 is un-merging the files, and whatever you want to do for collection interval.

How do we go about defining or expanding the dataset?

::: services/healthreport/providers.jsm
@@ +20,5 @@
>    "AddonsProvider",
>    "AppInfoProvider",
>    "CrashDirectoryService",
>    "CrashesProvider",
> +  "PlacesProvider",

This needs to live somewhere else, because it doesn't make sense on Android. Back to the preprocessor, I'm afraid.

@@ +897,5 @@
> +
> +  configureStorage: function () {
> +    return Task.spawn(function registerFields() {
> +      yield this.registerStorageField("pages", this.storage.FIELD_DAILY_LAST_NUMERIC);
> +      yield this.registerStorageField("bookmarks", this.storage.FIELD_DAILY_LAST_NUMERIC);

If we're able to choose what we collect here, I'd like to advocate for collecting some or all of:

* Folder count and/or max/average depth.
* Unsorted Bookmarks count.
* Counts per type of bookmark: livemark users, etc.
* Tags count.

We can clamp/round/bucketize values, or not collect all of these, if we think that'll be too user-identifying, but this will help us know the kinds of bookmark usage patterns that we need to support for real-world users.

@@ +913,5 @@
> +  name: "org.mozilla.places",
> +
> +  measurementTypes: [PlacesMeasurement],
> +
> +  // FUTURE collect at daily interval.

Looks like you're already addressing this.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #5)
> If we're able to choose what we collect here, I'd like to advocate for
> collecting some or all of:
> 
> * Folder count and/or max/average depth.
> * Unsorted Bookmarks count.
> * Counts per type of bookmark: livemark users, etc.
> * Tags count.
> 
> We can clamp/round/bucketize values, or not collect all of these, if we
> think that'll be too user-identifying, but this will help us know the kinds
> of bookmark usage patterns that we need to support for real-world users.

I agree. However the spreadsheet is only advocating for pages and bookmarks counts. We can always add more later.

Comment 7

[Daniel Einspanjer [:dre] [:deinspanjer]]

(In reply to Gregory Szorc [:gps] from comment #6)
> (In reply to Richard Newman [:rnewman] from comment #5)
> > If we're able to choose what we collect here, I'd like to advocate for
> > collecting some or all of:
> > 
> > * Folder count and/or max/average depth.
> > * Unsorted Bookmarks count.
> > * Counts per type of bookmark: livemark users, etc.
> > * Tags count.
> > 
> > We can clamp/round/bucketize values, or not collect all of these, if we
> > think that'll be too user-identifying, but this will help us know the kinds
> > of bookmark usage patterns that we need to support for real-world users.
> 
> I agree. However the spreadsheet is only advocating for pages and bookmarks
> counts. We can always add more later.

Yeah, Metrics committed to privacy that we would not add any new data points for the initial release of the feature unless there was a pressing need that was discussed with them.

The places and bookmark counts, combined with session count/duration are intended to be used as a proxy for measuring intensity of usage rather than specific feature usage.

I think it is likely a case could be made to collect information about various types of feature usage such as what you are describing.  We just need to present the justification and get sign off for it at some point after the initial release.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 708591 [details] [diff] [review]
Part 1: Add "health report" mode to PlacesDBUtils.telemetry(), v1

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

This module is quite old, would need a refactoring, but not really a priority now!

::: toolkit/components/places/tests/unit/test_telemetry.js
@@ +128,5 @@
>      validate(snapshot.sum);
>      do_check_true(snapshot.counts.reduce(function(a, b) a + b) > 0);
>    }
>    do_test_finished();
>  });

looks like this do_test finished() call should not be here...

Comment 9

[Gregory Szorc [:gps]]

Attachment: Part 2: Add Places provider to FHR, v2

Rebased on top of collectDailyData().

I didn't address the Android compat issue from previous review. I figure we can address Android compat in bulk when we need to cross that bridge.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #9)

> I didn't address the Android compat issue from previous review. I figure we
> can address Android compat in bulk when we need to cross that bridge.

I think Chenxia is crossing that bridge right now -- last I checked she had FHR building on Android. Please check in with her about how urgent this is.

Comment 11

[Gregory Szorc [:gps]]

See comment #10.

Comment 12

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/4654abd41ddf
https://hg.mozilla.org/services/services-central/rev/e2e1ea12ddc9

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/4654abd41ddf
https://hg.mozilla.org/mozilla-central/rev/e2e1ea12ddc9

Comment 14

[u279076]

Using the SQLite Manager add-on I can confirm that I have an org.mozilla.places provider which is recording "pages" and "bookmarks" measurements. Is there another way I can view the actual payload to confirm or is this sufficient to call this bug verified?

Comment 15

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #14)
> Using the SQLite Manager add-on I can confirm that I have an
> org.mozilla.places provider which is recording "pages" and "bookmarks"
> measurements. Is there another way I can view the actual payload to confirm
> or is this sufficient to call this bug verified?

FYI, was using the add-on to look at my healthreport.sqlite in today's Aurora 21.0a2 build.

Comment 16

[Gregory Szorc [:gps]]

about:healthreport -> Show Details

FHR will need to have run for a day or two for you to see data.

Comment 17

[u279076]

(In reply to Gregory Szorc [:gps] from comment #16)
> about:healthreport -> Show Details
> 
> FHR will need to have run for a day or two for you to see data.

Richard Newman advised me to set datareporting.policy.firstRunTime to a time >12 hours in the past to test bug 830090. Changing this setting as helped out here too. I can now see the following in Show Details:

"org.mozilla.places.places": {
  "_v": 1,
  "bookmarks": 32,
  "pages": 48
}

I suppose this is what I'm looking for, right Gregory?

Comment 18

[Gregory Szorc [:gps]]

That's exactly what you are looking for!

Comment 19

[u279076]

Thanks Gregory. I'm calling this one verified fixed for Firefox Aurora 21.