Closed Bug 775825 Opened 7 years ago Closed 4 years ago

Add telemetry to measure index and type of the selected awesomebar result

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- fixed

People

(Reporter: ahurle, Assigned: adw)

References

Details

(Whiteboard: [suggestions][unifiedcomplete][metrics][fxsearch])

Attachments

(1 file, 6 obsolete files)

In order to effectively resolve bug 583683, we need to know how many results to display in the awesome bar.  If the user clicks (or presses Enter) on a result in the awesome bar, we should measure which index the user has selected.  We should make sure to measure the case where the dropmarker is clicked *separately* from the case where the user is typing, just in case there is a big discrepancy there.
Andrew, are you going to post a patch?
Adds an _isHistoryPopup field to the autocomplete-base-popup binding in order to distinguish the two cases, and a beforeResultChosen method to the autocomplete binding in order to let telemetry hook in on the URL bar at that point.
Assignee: nobody → fracture91
Status: NEW → ASSIGNED
Attachment #645614 - Flags: review?(dao)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=cf736137e761

I chose 50 as the upper bound for the histogram since maxRichResults can be set arbitrarily high by the user past the default of 12, but 30 might be more reasonable...
Comment on attachment 645614 [details] [diff] [review]
Patch to add telemetry for chosen result index

+HISTOGRAM_ENUMERATED_VALUES(FX_CHOSEN_URLBAR_RESULT_INDEX, 50, "Firefox: Index of chosen URL bar result in normal autocomplete case")
+HISTOGRAM_ENUMERATED_VALUES(FX_CHOSEN_URLBAR_DROP_RESULT_INDEX, 50, "Firefox: Index of chosen URL bar result in history dropmarker case")

50 is way too much. Use 13. Values past 12 will get recorded in bucket #12(cos things are 0-indexed).

If people regularly choose large values(which sounds highly unlikely), it'll show up in the histogram average being very high.
Attachment #645614 - Flags: feedback-
Attached patch v2 - Use 12 instead of 50 (obsolete) — Splinter Review
(In reply to Taras Glek (:taras) from comment #4)
> 50 is way too much. Use 13. Values past 12 will get recorded in bucket
> #12(cos things are 0-indexed).

Well, if we want 13 buckets, I should use 12 there, not 13.  HISTOGRAM_ENUMERATED_VALUES will create n+1 buckets.
Attachment #645614 - Attachment is obsolete: true
Attachment #645614 - Flags: review?(dao)
Attachment #647294 - Flags: review?(dao)
Comment on attachment 647294 [details] [diff] [review]
v2 - Use 12 instead of 50

It would be nice if we could avoid introducing a new pseudo-API here (beforeResultChosen). Keeping _isHistoryPopup out of autocomplete.xml would be nice too.

Hopefully Marco can come up with some more specific suggestions on how to make this patch less invasive.
Attachment #647294 - Flags: review?(dao) → review?(mak77)
(In reply to Dão Gottwald [:dao] from comment #6)
> Hopefully Marco can come up with some more specific suggestions on how to
> make this patch less invasive.

So, I think a very good guess here may be enough, without over-complicating the code to achieve perfection.
First, we were already thinking to remove the empty search dropdown (in favor of the newtab page), so I'm not sure the benefit of measuring it, we may just go measuring the normal searches.
Regardless I expect the empty dropdown results to be jumpy and less meaningful; it's just a list of _unrelated_ frecent pages, the fact the user chooses the last one doesn't mean much, if we'd show 100 he may need the 99th at that specific time (exactly because the results are unrelated to the user's task). 
The common search case is more meaningful cause all results are related to the search text, so the index gives us a measurement of how good the autocomplete algorithm is and how many results we need.

Second, measuring the index with the common search may be much easier to do just hooking here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3696.
More specifically, autocomplete controller generates 2 notifications "autocomplete-will-enter-text" and "autocomplete-did-enter-text", we already use one in Places, and would be easy to just store the index from there, or alternatively add another listener for the same notification elsewhere.
I think it's worth a try cause a similar solution would be far less intrusive.

Thoughts?
This appears to be feature usage measurement which is inline with the current policy
Comment on attachment 647294 [details] [diff] [review]
v2 - Use 12 instead of 50

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

clearing, see comment 6 and comment 7
Attachment #647294 - Flags: review?(mak77)
This is not an issue for the Telemetry toolkit. Over to Firefox to implement the probe.
Assignee: fracture91 → nobody
Component: Telemetry → General
Product: Toolkit → Firefox
this is still worth doing, using the autocomplete-did-enter-text notification.
We can either use telemetry or UI telemetry, the latter is already listening to such notification, see http://mxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#199 so it should be trivial.
Blocks: 1071461
After a brief discussion with shorlander we figured we need this asap to make better choices, additionally to the selected index, we also want to telemetry the entry type, whether it's a bookmark, history, tag, search, switch to tab and so on...
Blocks: 958204
Flags: firefox-backlog?
Priority: -- → P1
Whiteboard: [suggestions][fxsearch][unifiedcomplete]
Summary: Add telemetry probe to measure index of selected awesomebar result → Add telemetry to measure index and type of the selected awesomebar result
we need the simplest patch possible, since it's likely something we want to uplift.
Rank: 19
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: [suggestions][fxsearch][unifiedcomplete] → [suggestions][unifiedcomplete][metrics][fxsearch]
Assignee: nobody → adw
Attached patch patch (obsolete) — Splinter Review
Attachment #647294 - Attachment is obsolete: true
Attachment #8629059 - Flags: review?(mak77)
Comment on attachment 8629059 [details] [diff] [review]
patch

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

The patch looks good, but I couldn't figure out if UITelemetry is a still alive project, I couldn't find a working dashboard for it, the latest data I found was from January.
Indeed bug 1178446 points out the dashboard is down from 2 months and nobody is working on a fix (there's some discussion about Q3 work)
I'd not want that we end up collecting valuable data, but that's unaccessible for months, until someone moves it into an usable store.

If we can't get the data in a month from now, maybe we should go back to usual toolkit telemetry :/

Thoughts?

::: browser/modules/BrowserUITelemetry.jsm
@@ +619,5 @@
> +      !action ?
> +        "(no action)" :
> +      action.type == "searchengine" && action.params.searchSuggestion ?
> +        "search-suggestion" :
> +      action.type;

I wonder if in the "no action" case, we should also check if getStyleAt contains "autofill", "tag", "bookmark", otherwise just fallback to "history"
Attachment #8629059 - Flags: review?(mak77)
Blake, what's the UI telemetry status, do we have a way to collect the data if, let's say, we land this today and want data in a couple weeks from now? Is that complicated? do we need the dashboard?
Flags: needinfo?(bwinton)
The dashboard would be the easiest way to look at the data, if it was working.  (Getting it back up is one of my Q3 goals, but that probably won't happen in the next couple of weeks.)

But, we don't need the dashboard to process the data.  We can always run a custom report to answer any questions we may have, and in this case, I think a custom report would be the best idea.  (Email me when you want the report run, and I'll either run it myself, or show you how, or put you in touch with the right people.  ;)
Flags: needinfo?(bwinton)
thanks, sounds good. this is likely something shorlander and I would like to check once we collected some amount of data.

So looks like we can proceed with this.
I still think it might be interesting to have also bookmarks, autofill and tags data though (but mostly bookmarks and autofill)
Attached patch patch v2 (obsolete) — Splinter Review
Benjamin, would you please review this BrowserUITelemetry change?
Attachment #8629059 - Attachment is obsolete: true
Attachment #8630200 - Flags: review?(mak77)
Attachment #8630200 - Flags: review?(benjamin)
Comment on attachment 8630200 [details] [diff] [review]
patch v2

I'm reviewing the docs. I'm not the right person for a code review.

Is it really useful to do this using UITelemetry instead of a keyed histogram? In general the histogram mechanism is preferable to the custom queries, because it has the ability to auto-expire or expand to release population if appropriate.

Other questions:
* AIUI, the purpose of this measurement is to answer the questions in bug 583683. If that's correct, this measurement doesn't need to be permanent and should expire.
* who is creating the reports for this measurement?
Flags: needinfo?(adw)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> I'm reviewing the docs. I'm not the right person for a code review.

Yes, thanks.

> Is it really useful to do this using UITelemetry instead of a keyed
> histogram? In general the histogram mechanism is preferable to the custom
> queries, because it has the ability to auto-expire or expand to release
> population if appropriate.

This is my first interaction with UITelemetry.  I'm not sure what it is or why it exists apart from regular telemetry to be honest.  Maybe Marco can comment.

> Other questions:
> * AIUI, the purpose of this measurement is to answer the questions in bug
> 583683. If that's correct, this measurement doesn't need to be permanent and
> should expire.

That may be part of it, but the immediate point as I understand it is to help us decide how we should weight search suggestions in the results -- maybe how we should weight many of the types of results (comment 12).

> * who is creating the reports for this measurement?

Blake volunteered (comment 17).
Flags: needinfo?(adw)
If bwinton will be producing the report, he can probably comment on why UITelemetry instead of a standard histogram.

I'd still like this to expire, since none of the reporting you mention needs to be permanent and I can't imagine bwinton signing up to make this a permanent report.
Flags: needinfo?(bwinton)
I normally go with UITelemetry because that's what the dashboard consumes.  Comment 11 also seems relevant.  ;)
If we want more of a one-off report to answer some questions we have now, then regular telemetry would be a better choice, and someone else would be a better choice to produce it.  (Which I'm totally not against, in case that's not clear!)

As I understand it, UITelemetry is different than regular telemetry because counting clicks on built-in buttons is something that we want an ongoing record of, and would be difficult to do in regular telemetry.  (Because it would create too-many or too-complicated buckets, perhaps?)
Flags: needinfo?(bwinton)
I actually suggested UITelemetry for 2 reasons:
1. it was already listening to the same notification, thus making this patch very trivial.
2. with toolkit telemetry we'd need to write specific code to accumulate data, ui telemetry does that for us. I guess it may be nice to have a telemetry module that allows to accumulate counts like ui telemetry.

On the other side, from my point of view of Places owner, I'd prefer toolkit telemetry, cause I can have the data always accessible through the usual dashboard without having to rely on someone generating a report (or having to learn how to generate one on demand).

I am not sure if this data should expire, it would be generally useful to have telemetry about how users habits on the awesomebar change when we make changes to its behavior. If by expiring we mean just retaining data for the last 6 months-1 year, that sounds good. If instead we'd just stop collecting data after a month, then we would not have good comparisons when we actually need it in future.
I don't know what "specific code to accumulate data" means in this context. It looks like you can use a linear histogram for the position and an enumerated histogram for the type and wouldn't need anything other than a standard histogram.accumulate call.

But with those, the data should just show up in the standard telemetry dash.
Ah, for some reasons I thought the telemetry .add method was only retaining the last value, or reporting immediately instead of accumulating, silly me.

So, I think I'd also be fine with toolkit/telemetry, we'd just need to add another listener to the same notification somewhere in toolkit/places (maybe the PlacesCategoriesStarter could be reused, or unified complete itself could listen to the notification and report telemetry itself).
Comment on attachment 8630200 [details] [diff] [review]
patch v2

OK, I'll work on a toolkit telemetry version.
Attachment #8630200 - Flags: review?(mak77)
Attachment #8630200 - Flags: review?(benjamin)
Keyed histograms for the result type would be really useful, but currently they aren't shown on the dashboard: bug 1151756

Since we want this data ASAP, I'll go with a plain enumerated histogram that maps types to integers, unless you disagree, Marco.
Attached patch patch v3 (toolkit telemetry) (obsolete) — Splinter Review
Every time I reload about:telemetry FX_URLBAR_SELECTED_RESULT_INDEX's 0 bucket gets incremented even though the code that increments the histogram is definitely not running... what...  Maybe my build is messed up.

Since the URL bar lives in browser, it didn't seem right to do this telemetry in UnifiedComplete or anywhere else in toolkit, so I added it to nsBrowserGlue.
Attachment #8630200 - Attachment is obsolete: true
Attachment #8630683 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #29)
> Maybe my build is messed up.

Yeah, I don't see it after rebuilding the world.
Comment on attachment 8630683 [details] [diff] [review]
patch v3 (toolkit telemetry)

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

::: browser/components/nsBrowserGlue.js
@@ +513,5 @@
> +    if (action) {
> +      actionType =
> +        action.type == "searchengine" && action.params.searchSuggestion ?
> +        "search-suggestion" :
> +        action.type;

nit: indent options

@@ +523,5 @@
> +    }
> +
> +    Services.telemetry.
> +      getHistogramById("FX_URLBAR_SELECTED_RESULT_INDEX").
> +      add(idx);

nit: I prefer indentation on dots
Services.telemetry
        .getHistogram
        .add

@@ +539,5 @@
> +      "switchtab",
> +      "tag",
> +      "visiturl",
> +    ];
> +    let bucketIdx = 1 + buckets.indexOf(actionType);

why didn't you use the 0 bucket? Or did you intend to use that for "unknown"?

Could we rather have a direct index map? I fear someone could mess with the simple array (add or remove in the middle) and break all of the data. Also because one would try to preserve the alphabetical order :D
something like 
let buckets = new Map([
 [ "autofill", 0 ],
 [ "bookmark", 1 ],
 ...
]);
Also a warning comment stating one should never change the value of an entry or he will break all of the data, would be nice.
Bonus points if you cache the map in a local property so we don't have to rebuild it every time.

Finally, just to prevent possible bugs, check if from the map we get undefined, if so reportError and don't add to telemetry. Someone could add a new action type and not figure out he has to add something to browserglue, it's not due that reviews will catch that and having an unknown bucket is not really useful for stats.
Attachment #8630683 - Flags: review?(mak77) → review+
Attached patch patch v4 (obsolete) — Splinter Review
(In reply to Marco Bonardo [::mak] from comment #31)
> Could we rather have a direct index map? I fear someone could mess with the
> simple array (add or remove in the middle) and break all of the data. Also
> because one would try to preserve the alphabetical order :D

Good point!  I switched to a JS object literal, which is a little simpler than defining a Map I think.
Attachment #8630683 - Attachment is obsolete: true
Comment on attachment 8631120 [details] [diff] [review]
patch v4

Benjamin, could you review these two new histograms please?
Attachment #8631120 - Flags: review?(benjamin)
Iteration: --- → 42.1 - Jul 13
Comment on attachment 8631120 [details] [diff] [review]
patch v4

I am still uncomfortable with expires_in_version: never. If a measurement never expires, the requirement is that you have identified the person who will monitor that metric (typically a product manager or QA person). Who is signing up to monitor this metric permanently?

If nobody is committing to permanent monitoring, this should expire in no more than six months (4-5 releases). If the plan is to occasionally check the data but not permanently monitor, we can re-enable the histogram in the future very easily.
Attached patch patch v5Splinter Review
Same as previous but with "expires_in_version": "45" for both histograms.  Benjamin r+'ed with that change over IRC, so I'll land this when the tree reopens.
Attachment #8631120 - Attachment is obsolete: true
Attachment #8631120 - Flags: review?(benjamin)
Attachment #8631172 - Flags: review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #34)
> I am still uncomfortable with expires_in_version: never. If a measurement
> never expires, the requirement is that you have identified the person who
> will monitor that metric (typically a product manager or QA person). Who is
> signing up to monitor this metric permanently?

FWIW, I monitor all the Places related metrics every month.
https://hg.mozilla.org/mozilla-central/rev/43ec3f8db017
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1201017
You need to log in before you can comment on or make changes to this bug.