Closed Bug 671001 Opened 9 years ago Closed 9 years ago

Use telemetry to collect common Places stats

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [inbound])

Attachments

(1 file)

Some interesting stats we may need
- page cache size
- database size
- number of places
- number of bookmarks
- expiration (time/hits/stepsize)
Summary: Add telemetry to collect common Places stats → Use telemetry to collect common Places stats
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 671042
I'rather using this for Places, since bug 671042 is also about form history
Blocks: 671042
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → mak77
Status: REOPENED → ASSIGNED
After analyzing our needs, I've come up with this list of measures, there's some:

* Number of unique pages
* Average visits per page
* Number of bookmarks
* Number of tags
* Number of folders
* Number of keywords
* Percentage of bookmarks organized in folders
* Percentage of tagged bookmarks
* Frecency distribution
* Database filesize (MB)
* Database pagecache size (MB)
* Per thousand rate of memory used by the database pagecache
* Database journal size (MB)
* Average size per page (bytes)
* Time for first autocomplete result (ms)
* Time for all autocomplete results (ms)
* Bookmarking average time (ms)
* Expiration steps to cleanup the database

Actually, there may also be the need to measure effectiveness of inputhistory (hit ratio of adaptive autocomplete) but the strategy to measure it seems trickier.
Frecency is as complicated as adaptive. It's really easy to measure values, but measuring useful values is not, where by useful I mean getting measures that will allow to gather an improvement in performances or functionality.  I'll split all awesomebar telemetry to a follow-up bug, since we should figure out possible directions for improvements before measuring if they may have a positive impact.
Attached patch patch v1.0Splinter Review
This is a first batch of measures we may find useful to improve the database or the queries.
I left out some measure because I don't think they are that useful now, for example the bookmarking time may be bad only in case of wrong api usage (was the case of mobile) or if some add-ons does really bad things in observers (and we can't do much). We already have ideas there like async api and measures would not help.
Avg number of visits per page is not really useful as well, due to high number of pages it would flatten down to values between 1 and 2.
The current cache size is not measureable by js, about:memory can measure it through sqlite api, at that point would be better to add telemetry to about:memory probes.
Since we can add measures at any time, I think these ones (13 measures) are a really good first batch of them.

11 measures are done during maintenance, that happens once a week, 1 measure is in expiration and tracks long expirations to check if current values are too large or too small, 1 measure is in autocomplete and tracks the time it takes to return the first matching result once the queries are fired, but only if it's larger than 50ms that we consider being 'responsive'.

In the test I couldn't replace the telemetry factory to override it, since that idl can't be implemented in js :(
Attachment #548405 - Flags: review?(dietrich)
Depends on: 671078
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 548405 [details] [diff] [review]
patch v1.0

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

r=me. please get Taras to review for the telemetry-specific bits.

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +845,5 @@
> +
> +    // Hash of telemetry probes.  Each one uses the historygram name as key,
> +    // and may be either a database query or an helper function.
> +    let probes = {
> +      PLACES_PAGES: "SELECT count(*) FROM moz_places",

I'd prefer PLACES_PAGES_COUNT (and for the others) so that it's immediately obvious what the data is.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +729,5 @@
>      result.setDefaultIndex(result.matchCount ? 0 : -1);
>      this._listener.onSearchResult(this, result);
> +    if (this._telemetryStartTime) {
> +      let elapsed = Date.now() - this._telemetryStartTime;
> +      if (elapsed > 50) {

this is good. we'll know the range of badness. however, i'd also like to know how often these happen relative to <50ms queries, so we know just how bad of a problem it is. doesn't need to block this bug though.
Attachment #548405 - Flags: review?(dietrich) → review+
(In reply to comment #6)
> ::: toolkit/components/places/nsPlacesAutoComplete.js
> @@ +729,5 @@
> >      result.setDefaultIndex(result.matchCount ? 0 : -1);
> >      this._listener.onSearchResult(this, result);
> > +    if (this._telemetryStartTime) {
> > +      let elapsed = Date.now() - this._telemetryStartTime;
> > +      if (elapsed > 50) {
> 
> this is good. we'll know the range of badness. however, i'd also like to
> know how often these happen relative to <50ms queries, so we know just how
> bad of a problem it is. doesn't need to block this bug though.

Sure that would be extremely interesting, but it's problematic, we should collect stats along a certain amount of time and submit them only if they cover the full timeframe.  To be clear, telemetry just stores a number, so we have to workaround that limitation locally, if we would like to know the slow/fast queries ratio in a day, we should add some method to collect and store these numbers on disk and. on each day change, calculate the ratio, send it and remove the originating data.  Not impossible obviously, but largely more invasive.  I'll file a follow-up.
Blocks: 675636
http://hg.mozilla.org/mozilla-central/rev/638c7c0953da
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Depends on: 675878
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Depends on: 676246
You need to log in before you can comment on or make changes to this bug.