1869307 - Fix abandonment / impression metrics for certain 0-prefix results (e.g. contextual opt-in)

Status: NEW

(Firefox :: Address Bar, defect, P2)

Description

[Dão Gottwald [:dao]]

Based on what we've discussed so far for bug 1865336, it seems that we don't correctly record an abandonment event (and consequentially no impression event either) when dismissing the results panel for 0-prefix. This is bad when we want to limit a result to a certain number of impressions as is the case with the clipboard result. It's also potentially problematic when we want to analyze impressions specifically for 0-prefx, as is the case with the Firefox Suggest contextual data collection opt-in result; the reason why that doesn't run into this problem is that it counts impressions differently (in startQuery, see https://searchfox.org/mozilla-central/rev/c003be8b9727672e7d30972983b375f4c200233f/browser/components/urlbar/UrlbarProviderQuickSuggestContextualOptIn.sys.mjs#245,270) but that's not a good long-term solution, as all parts of the address bar should share an understanding of what an impression is.

Comment 1

[Dão Gottwald [:dao]]

Adjusting the severity and priority based on what I had discussed with Chris. Real-world user impact is currently low but that's just by luck, e.g. this presumably would have screwed up telemetry for the Firefox Suggest experiment that I mentioned. Seems like we need to fix this if we want to implement more features for 0-prefix.

Comment 2

[Marco Bonardo [:mak]]

Using startQuery to count an impression for the purpose of hiding a result after N impressions is, imo, not correct. From when a result is returned to when it's actually being shown in the view, many things can happen preventing the result from being shown at all. For example there may be a focus change, a further input event, or the Muxer may decide it's not the right time to show that result.

Unless of course the thing escapes the normal path of results. If it's surely shown every single time, it's ok to use onStartQuery.

In the past we added context.visibleResults also to account for this difference, the fact a result is in the .results array, doesn't necessarily mean it has been shown. Though you'd still need a way from the provider to be notified that a result has been shown. We only have onEngagement, for when a result is picked, and I already filed bug 1857236, because I think notifying to every provider is error prone. And I think the reason we do that is to measure impressions (otherwise why would a provider care that some other result has been picked?)

From telemetry so far we used to rely on engagement/abandonment telemetry to measure impressions, as they contain the whole list of results at the time of the event.
Though for counting impressions locally, it may be better to have a specific onResultShown callback invoked on the provider of that result, and fix
bug 1857236.
My only concern is notifying each provider each time a result is shown, will have a perf cost. We could make it declarative, a Provider may have a notifyOnResultShown bool getter, and be notified only if that's set. I'm sure many providers don't care.

Comment 3

[Marco Bonardo [:mak]]

Updated plan:

1. Remove the impression event (https://bugzilla.mozilla.org/show_bug.cgi?id=1878983)
2. Modify the abandonment event, to also happen in case of search string replacement (e.g. tab switch)
3. Add a reason property for the abandonment event, to distinguish focus loss from other causes
4. Split onEngagement in Providers to onEngagement (on providers whose result is picked), onAbandonment (check current use to see where it makes sense to fire), onImpression (on providers whose results are visible when engagement or abandonment happen)

Comment 4

[Karandeep Lubana [:klubana]]

Attachment: Bug 1869307 - Convert the QuickSuggest provider onLegacyEngagement to use the new notifications. r=adw

Comment 5

[Drew Willcoxon :adw]

The status of this bug is a little confusing now. Comment 0 and the summary talk about 0-prefix telemetry. We should evaluate whether that's still a problem. Actually I think we should evaluate whether that telemetry is even necessary anymore. I added it for weather suggestions but they no longer use it. The Suggest contextual opt-in used it but I don't think we're moving forward with that? Does anything else use it? Can we remove it? Should it be ported to Glean?

The architecture changes Marco commented on seem orthogonal to me or at least larger in scope, but regardless they've been addressed by Karandeep's recent work. I stole the patch that Karandeep posted here and fixed the Suggest provider in bug 1897244.

Comment 6

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 7

[Marco Bonardo [:mak]]

Based on comment 5 this is probably a wontfix.
The Jira ticket ended up covering bug 1897244.