1575038 - Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks

Status: VERIFIED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Drew Willcoxon :adw]

We added engagement event telemetry in bug 1559136. It only covers engagements that start by clicking in the urlbar. So for example, if you use the keyboard shortcut or tab into the urlbar to start an engagement, we don't record it.

I'm not sure why. It looks to me like a deliberate choice since bug 1559136 instrumented mousedown specifically when it could have just as easily instrumented all focus events. I looked through the relevant bugs and doc but didn't see anything. Teon and Marco are both away.

Slack excerpt:

adw: @mconnor @teon re: engagement telemetry question earlier. so we decided we want to record engagement when the user starts it with ctrl/command-L. but what we really want is to record engagement regardless of how it starts, right? e.g. the user tabs the focus into the urlbar. if so, do we want to record how the engagement event started — i.e. whether with the mouse or keyboard (or some other level of detail)? right now we record how it ends, when the user chooses a result.
bmiroglio: teon is out, but I'd say knowing how the interaction started would be useful info to have as well, assuming it isn't a mess to implement. Knowing, for instance, what % of users interact using the shortcut could inform future changes.
mconnor: also, which shortcut
mconnor: i.e. Cmd+K vs. Cmd+L

Comment 1

[Drew Willcoxon :adw]

[Tracking Requested - why for this release]: We'd like this telemetry for quantumbar experiments on 69.

Comment 2

[Drew Willcoxon :adw]

Couple of amendments: We also record engagement when you type something, regardless of how you focused the urlbar; and also when you press down, up, page down, and page up to open the view, regardless of how you focused the urlbar. Neither of those cases capture the top sites interaction though since the view opens automatically on focus.

Comment 3

[Drew Willcoxon :adw]

This is from the North Star doc:

URLBar Time to Engagement is defined as the length of time from the initiation of a user-generated text-based interaction within the address bar (i.e. the time when the first character is typed or pasted into the address bar) to a user-generated action within the address bar that navigates to a URI (e.g. clicking on a selected link, pressing enter on a selected link).

https://docs.google.com/document/d/1ZcOuQVg_jTd8YLtMyaxiW5ikuxvs3zAqrrEqkGC4S0Q/edit?usp=sharing

Seems like what we really want is to start tracking engagement from the time the view is shown, not from the time the user starts typing/pastes. Typing shows the view, but typing isn't the only way to show the view.

Comment 4

[Drew Willcoxon :adw]

Actually I think the only issue is openViewOnFocus (i.e. where the view is automatically shown on input focus). It's true you can open the view without typing, but we currently capture the other two cases where that happens: clicking the dropmarker, and pressing up/down/page-up/page-down in the input. So we're just not accounting for openViewOnFocus when we probably should be.

Although mconnor's idea of capturing which shortcut was used wouldn't be addressed by fixing that. Maybe we should leave that to a follow-up.

Sorry for the bugspam as I'm thinking through this out loud.

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1575038 - Quantumbar: Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks.

We need to start engagement event recording when the view opens due to openViewOnFocus. We already do for mouse clicks since we call engagementEvent.start from _on_mousedown. But we don't for the Ctrl/Command-L key shortcut. The shortcut command calls openLocation in browser.js, which calls gURLBar.startQuery but not engagementEvent.start.

Every time we call engagementEvent.start, we do it before calling input.startQuery. The one exception is in input._on_drop because there we just handle the dropped value directly instead of starting a new query with it.

The inverse is also mostly true, i.e., every time we call input.startQuery, we also call engagementEvent.start. The three exceptions are: in UITour (where it looks like we should be calling urlbar.search instead), in UrlbarInput after picking a keyword offer result, and in openLocation in browser.js (mentioned above). So really the only valid place is after picking a keyword entry.

So, it makes sense to move engagementEvent.start() into input.startQuery so that callers don't have to call it. I added an event param to startQuery, since engagementEvent.start needs one. I considered removing that need. It's possible, but then we would need a way to avoid calling engagementEvent.start in the keyword offer case, so startQuery would need something like a suppressEngagementEvent param. event basically functions as that, so I left it.

Another thing to point out about this patch is that I chose to record a "typed" value when the pageproxystate is invalid and the view opens due to openViewOnFocus. The view does not show the user's top sites in that case, so "topsites" seems wrong.

Comment 6

[Drew Willcoxon :adw]

Re-summarizing to better reflect the problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee4bc6b776238bf6b1a3c0c9afaf67ef1ee5b036

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b89247dda00
Quantumbar: Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks. r=dao

Comment 8

[Drew Willcoxon :adw]

STR for QA:

1. Set pref browser.urlbar.eventTelemetry.enabled=true (it doesn't exist by default)
2. Focus the urlbar by clicking it with your mouse
3. Type "example" and hit enter
4. Open about:telemetry and look at the Events tab, at the bottom
5. The second-to-last entry (probably -- the position isn't important) at the bottom should have "urlbar" in the category column, "engagement" in the method column, "enter" in the object column, "typed" in the value column, and this should be in the extra column: {"elapsed": "2676", "numChars": "7", "selIndex": "0", "selType": "search"} (except "elapsed" will be different, it's not important)
6. The last entry (probably -- the position isn't important) should correspond to your typing about:telemetry and hitting enter in step 4. It should look the same as the entry in step 5, but in extra, "elapsed" will be different, "numChars" should be 11, and "selType" should be "autofill"
7. Blur the urlbar by clicking in the page. Make sure the urlbar is not focused.
8. Focus the urlbar by pressing Ctrl/Command-L.
9. Repeat steps 3-5, but type "another example" instead of "example". There should be a new entry in about:telemetry that looks the same as in step 5, except "elapsed" will be different and "numChars" should be 15

Comment 9

[Drew Willcoxon :adw]

Attachment: Beta/69 patch

Beta/Release Uplift Approval Request

• User impact if declined: This fixes an oversight in telemetry we'd like to collect while running quantumbar experiments 69.
• 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 8
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I'm sorry for requesting this so late in the game, but this (1) slightly refactors some telemetry-related code, and (2) fixes a hole in our telemetry collection. This telemetry is preffed off by default, and on 69 it will only be turned on while running quantumbar experiments. The risk is low but not zero: the patch's purpose is to refactor/fix some telemetry as I just described, but the code in question is part of the quantumbar hot path and touches more than just telemetry. I ran the relevant test locally. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a562cacec553034839e61679921f870471456b
• String changes made/needed: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9087489 [details] [diff] [review]
Beta/69 patch

Approved for 69.0b16, thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91c666707c219c5215117da97cbbec3bf07f42e1

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/8b89247dda00

Comment 13

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #0)

I'm not sure why. It looks to me like a deliberate choice since bug 1559136 instrumented mousedown specifically when it could have just as easily instrumented all focus events. I looked through the relevant bugs and doc but didn't see anything. Teon and Marco are both away.

I'll try to answer your question; yes, it was deliberate choice to force each consumer/event to invoke engagementEvent.start() explicitly, instead of making every "queryStart" or "focus" do it, mostly to protect us from false positives. It is trivial to add new features opening the panel or starting a query, or focusing the urlbar, that won't be a direct consequence of a user action, things like uitour are an example of features developed by other teams (unaware of this measurement), that may expose us to possible measurement regressions.
Overall, I would have preferred the explicit call to stay under each event, and a new start() call added for the newly measured cases, because imho it was more explicit and cleaner than filtering on an event argument.

Not handling openViewOnFocus from openLocation() was obviously my mistake, I probably searched openViewOnFocus only in the "urlbar" folder and missed that call.

Comment 14

[Mike de Boer [:mikedeboer]]

FWIW, we landed the change as a stop-gap measure without having the context you provided - so it's very reasonable to ask Drew to re-implement the change with your suggestion in mind.

Comment 15

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is work as expected, I followed the steps from comment 8, I verified using Firefox 70.0a1 and 69.0b16 on Win 10 x64, macOS 10.14 and Ubuntu 18.04 x64.

Comment 16

[Drew Willcoxon :adw]

I don't really agree with that. The caller still has to take care to pass in an event, so there's not much danger of accidentally recording telemetry when we didn't want to. Is there slightly more danger since it's just another param and not a separate call? Maybe, although the param's comment says it should be a user-generated event.

The usual case by far is that the popup is opened due to user action. I don't think it's nice to force every caller to have to call telemetry. Then we end up with bugs in the other direction, like this one -- not recording telemetry when we wanted to. Either way there's a danger of not recording when we want to record or recording when we don't. We should accommodate the common case.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #16)

I don't really agree with that. The caller still has to take care to pass in an event, so there's not much danger of accidentally recording telemetry when we didn't want to. Is there slightly more danger since it's just another param and not a separate call? Maybe, although the param's comment says it should be a user-generated event.

My concern is that we replaced an explicit "I am registering an egagement event" with "I am passing an event argument", that completely hides to the caller a potential risk of an unwanted registration. Even just renaming the argument would clarify things, at least someone looking at startQuery may argue about that argument use. The javadoc may also be a bit more clear in cases where the argument should NOT be given.