Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Private browsing mode URIs are not included in total URI count metric #17089

Closed
7 tasks
tdsmith opened this issue Dec 17, 2020 · 3 comments
Closed
7 tasks

Private browsing mode URIs are not included in total URI count metric #17089

tdsmith opened this issue Dec 17, 2020 · 3 comments

Comments

@tdsmith
Copy link
Contributor

tdsmith commented Dec 17, 2020

Pageloads are excluded from the total URI count metric if they occur in a private browsing tab:

// Record UriOpened event when a non-private page finishes loading
if (tab.content.loading && !action.loading && !tab.content.private) {
metrics.track(Event.UriOpened)
}

In https://bugzilla.mozilla.org/show_bug.cgi?id=1535169 we made a case to stop excluding PBM from URI count tallies in Firefox desktop, and it would make sense to make a symmetric change in Fenix. Ehsan's governance email from 2018 explains why we've historically taken a conservative approach to instrumenting PBM and also why that approach is inconsistent. Our PBM threat model doesn't include preventing an adversary from observing that PBM activity occurred, and it's real activity that we should reflect in our metrics.

In particular, this makes it hard to use URI counts as a measure of real user activity.

@magorlick, do you have any concerns about changing the definition of the existing metric, or would you prefer to stand up a new "complete total URI count" metric that includes both PBM and non-PBM activity?

Acceptance Criteria

  • ENG files a DS JIRA request outlining their methodology.
  • DS sign off on instrumentation methodology addressing product questions.
  • Event pings can be queried via re:dash
  • Event pings can be queried via amplitude
  • We are sending telemetry events for the actions listed in the requirements
  • We have documented the telemetry
  • We have asked a data steward to review the telemetry

┆Issue is synchronized with this Jira Task

@github-actions github-actions bot added the needs:triage Issue needs triage label Dec 17, 2020
@kbrosnan kbrosnan added Feature:Telemetry and removed needs:triage Issue needs triage labels Dec 17, 2020
@rocketsroger rocketsroger self-assigned this Feb 8, 2021
@irrationalagent
Copy link

Desktop now has an additional probe, browser.engagement.total_uri_count_normal_and_private_mode that does what it says on the tin. This probe will continue to exist side-by-side with the old total_uri_count which excludes PBM.

Of course, that we now have two probes is just an accident of history - the old total_uri_count probe does not provide us with any information not covered by the new probe. We've elected to keep both of them (for now) so that time series of total_uri_count do not break. There is also a long tail of new probe uptake which means it takes several months until the new probe has comparable coverage.

I would say if you need your current time series of uri counts to not break or change suddenly, then consider adding an additional probe. Or if version uptake is slow enough that a change to the current probe will take a long time to get adopted (if this is the case, then your aggregates will contain a mix of old and new behavior for an extended period which is not ideal). If this is less of a concern (new product, etc) then it probably wouldn't be so bad to change the behavior of the current probe. If we do this, we would want to make the date of the change clear in the metrics yaml file so it shows up in the probe dictionary.

That said, the conservative thing to do would probably add a new probe, then after awhile you can let the old one expire. Downside is you've now added some clutter, two probes doing similar things.

Other facts about how the uri_count probes behave on desktop (I'm paraphrasing some notes from a conversation w/ one of the desktop engineers about this last year):

Basically any navigation of a toplevel document in any element will count, independent of whether the tab is visible, the window minimized, or whether the load was user-initiated or not.

Examples of what will cause the probe to increment:

  • website on a meta refresh timer so it reloads every N minutes (some
    news sites do this; our rolling TV stuff around release / bug tracking
    in offices might do something like this; unsure)
  • ad-based hijacking
    (where they navigate from some benign website that embeds ads, one of
    which turns out to be malicious, to a cryptominer or whatever)
  • automated scripts
  • "you have been logged out for inactivity" from banks etc.

He wasn't 100% sure about twitter and other social media, but: where the site is just reloading content, but keeping
the same DOM document loaded in the browser, then probe likely will NOT increment. So long as they are just refreshing the timeline etc without reloading the top-level document.

@rocketsroger
Copy link
Contributor

I would say if you need your current time series of uri counts to not break or change suddenly, then consider adding an additional probe.

Agreed. I'll add a new probe. Thanks!

rocketsroger added a commit to rocketsroger/fenix that referenced this issue Feb 9, 2021
rocketsroger added a commit to rocketsroger/fenix that referenced this issue Feb 9, 2021
rocketsroger added a commit to rocketsroger/fenix that referenced this issue Feb 19, 2021
@rocketsroger rocketsroger added this to the 87 milestone Feb 23, 2021
@AndiAJ
Copy link
Collaborator

AndiAJ commented Feb 24, 2021

Hi, verified as fixed on Firefox 87.0.0-beta.1 using a OnePlus A3 (Android 6.0.1)
Opened 3 tabs in Normal Browsing and 3 in Private Browsing

"counter": {
          "events.normal_and_private_uri_count": 6,
          "events.total_uri_count": 3

Logcat
Glean dashboard

@AndiAJ AndiAJ added the eng:qa:verified QA Verified label Feb 24, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this issue Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants