1868602 - the surface displayed probe overcounts in some cases

Status: VERIFIED FIXED

(Firefox :: Shopping, defect, P1)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Summary

In certain cases, we have discovered counting errors in the probe that records events when the shopping sidebar is displayed in desktop Firefox (glean.shopping.surfaceDisplayed). These bugs go back to 119.

We are recording extra incorrect events in the following cases:

• tab switches
  • page is displayed with sidebar visible
  • user switches tabs
  • user then switches back to the page (without reloading the page)
  • an extra event is incorrectly recorded
• page loaded in background
  • page is loaded in a background tab
  • an event is incorrectly recorded immediately, before the user sees the tab
• user opts in
  • page is displayed with onboarding shown in the sidebar
  • event is recorded
  • user opts in, displaying review checker content
  • event is incorrectly recorded again
• user opts out, then back in
  • page is displayed with sidebar in opted-in state with review checker content
  • event is correctly recorded
  • user hides the sidebar by clicking "hide review checker" in sidebar settings component
  • user re-displays the sidebar by clicking the urlbar button
  • event is incorrectly recorded twice

We are correctly recording events in the following cases:

(note, all of these assume the page is loaded in a foregrounded tab)

• page is loaded by entering URL or clicking a link
• page is loaded from session restore
• page is displayed from hitting back/forward buttons
• page is reloaded by the user manually
• page was displayed with sidebar hidden, then the user clicked the urlbar button to show the sidebar
• page was displayed with sidebar visible, then the user closes the sidebar and reopens the sidebar
  • accurate if user has not opted in, so onboarding card is shown, and closes the sidebar using the X in the header, the "not now" link in the onboarding card, or the urlbar button
  • accurate if the user has opted in and closes the sidebar using the X in the header, but not in the case where the user opts out using the "hide review checker" button in the settings component
• page was displayed with sidebar hidden, then sidebar was made visible by another FF component
  • or by flipping prefs in about:config in a different window
  • or by nimbus activating the shopping feature for the first time
  • (note for future, this is not done now) or by a messaging system callout

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

[Tracking Requested - why for this release]: We've just discovered a bug in a critical Glean probe and would like to uplift a fix into 121.

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: WIP: Bug 1868602 WIP fixes r?Gijs

• Avoid counting background tab loads by checking if the browser is the selectedBrowser

• Avoid counting tab switches by passing TabSelect through and dropping those.

  • Unfortunately, this means tabs opened in the background, then foregrounded via tab switch, will not get counted.

Still todo:

• tabs loaded in background, then foregrounded later.

  • maybe we set some state on the chrome browser element, and clear it
    when the URL changes? sigh

• user opts in and we double count

• user opts out, then in, and we double count

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1868602 - Fixes to dedupe shopping surface displayed Glean counts. r?Gijs,perry.mcmanis

• Avoid counting background tab loads by setting a flag if a navigation
  involves a browser that is not the gBrowser.selectedBrowser. Set a
  flag in this case.

• Avoid counting TabSelects unless the background flag is set.

Still todo:

• user opt in/out double count

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

This can just ride the trains into 122; based on conversations with leadership, we need to iterate further on the review checker experience before it'll roll out further, and the overcounts tracked by this bug don't influence the data enough to merit late uplift into a respin/dot release.

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1868602 - Fixes to dedupe shopping surface displayed Glean counts. r?Gijs,perry.mcmanis

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dc81d782e04
Fixes to dedupe shopping surface displayed Glean counts. r=Gijs,tabbrowser-reviewers,mconley

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0dc81d782e04

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jhirsch, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 9367376 [details]
Bug 1868602 - Fixes to dedupe shopping surface displayed Glean counts. r?Gijs,perry.mcmanis

Beta/Release Uplift Approval Request

• User impact if declined: This feature is only experimentally exposed to a small percentage of en-US users. This patch corrects some telemetry overcounts so we can more accurately measure the number of pages loaded with the shopping sidebar visible.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Use the glean debugging tools to verify that the various extra telemetry counts listed in the bug description no longer occur.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very small patch that only affects users enrolled in the shopping experiments, which are limited to a very small percentage of en-US users.
• String changes made/needed: None
• Is Android affected?: No

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9367376 [details]
Bug 1868602 - Fixes to dedupe shopping surface displayed Glean counts. r?Gijs,perry.mcmanis

Approved for 122.0b9

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8085fb2889c6

Comment 13

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Nightly build 123.0a1 (2024-01-11) and Beta 122.0b9.