1800810 - Quick suggest engagement telemetry may not match visible results in the view

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

Bug 1800184 fixed the case where a quick suggest result is in the view but hidden, but I found one more case it didn't fix: The user can pick a result (i.e., complete an engagement) after the provider add()'ed it but before it's added to the view. [Edit: This sentence is confusing. What I mean is: After the quick suggest provider adds a result to the context but before that result is shown in the view, the user can pick another unrelated result in the view.] That means we can end up recording telemetry for results that aren't actually visible in the view, since the telemetry is based on _resultFromLastQuery, which is simply the result the provider added last.

In short we need to base engagement telemetry on the result that's visible in the view, not on the result the provider added last.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1800810 - Always record visible quick suggest results in engagement telemetry.

This is a follow up to D161866 and effectively reverts and replaces it with a
different approach. Please see bug 1800810 for background. In short, engagement
telemetry should be based on the result that's visible in the view, not on the
result the provider added last.

D161866 fixed the case where the last-added result is in the view but hidden.
There's another case we need to handle, when the last-added result is not in the
view at all. That can happen when you type something and hit enter really
quickly, before the view can update. I was able to trigger it several times.
When that happens, there are two possibilities:

• No quick suggest result is visible in the view. result.rowIndex is its
  initial value of -1, so we record an engagement for a result that is not
  visible and that doesn't have a valid index.

• A quick suggest result from a previous query is visible in the view. Here
  again, result.rowIndex is -1 so we record an engagement for a result that is
  not visible and that doesn't have a valid index, and we miss recording an
  engagement for the result that's actually visible.

Right now it's not easy to fix this inside the provider because providers don't
know anything about the view(s). I could record this telemetry in the view but
that's not its role. I think it makes sense to include the view in the query
context, so that's what this does. I added view.visibleResults to make getting
the visible results easy.

Daisuke's new Glean telemetry in D160193 uses context.results, which has this
same problem of not being based on actually visible results. We'll need to use
context.view.visibleResults there too, and there may be other existing cases
as well.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e0b7316fdbf
Always record visible quick suggest results in engagement telemetry. r=dao

Comment 3

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9e0b7316fdbf

Comment 4

[Drew Willcoxon :adw]

STR for QA:

1. Type a keyword to trigger a Firefox Suggest suggestion
2. Hit enter
3. Open/reload about:telemetry#search=contextual.services.quicksuggest.impression and verify that all the numbers in the Name field under the contextual.services.quicksuggest.impression section are between 1 and 10 inclusive. You will probably only see 10, but it depends on how many other suggestions were present when you hit enter.
4. Repeat the steps at different speeds (very fast, normal, etc.) and using different keywords ("nike", "amazon", "betty", etc.)

The number in the Name field is the index of the Firefox Suggest suggestion when you hit enter. The suggestion is always shown at the bottom, and there are 10 suggestions total, so that's why you will probably only see 10. We want to make sure that number is never zero or larger than 10.

The bug depends on timing which is why the steps should be repeated at different speeds.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 9303636 [details]
Bug 1800810 - Always record visible quick suggest results in engagement telemetry.

Beta/Release Uplift Approval Request

• User impact if declined: This is an important fix to Firefox Suggest telemetry that we'd like sooner than later.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Please see comment 4
• List of other uplifts needed: Bug 1800184 should be uplifted first
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not a user-visible change, has a test.
• String changes made/needed:
• Is Android affected?: No

Comment 6

[Dianna Smith [:diannaS]]

Comment on attachment 9303636 [details]
Bug 1800810 - Always record visible quick suggest results in engagement telemetry.

Approved for 108.0b3

Comment 7

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/275a3d150584

Comment 8

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I have verified this issue on the latest Nightly 109.0a1 build (Build ID: 20221118094451) and Firefox Beta 108.0b3 (Build ID: 20221117185908) on Windows 10 x64, macOS 12.4 and Linux Mint 20 x64.

• In order to verify the issue I have followed the STR provided in comment 4.