1755401 - implement reach for pbNewtab surface

Status: NEW

(Firefox :: Messaging System, defect, P3)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Andrei, when we last talked about implementing reach telemetry for the pbNewtab private browsing surface, my understanding of your thoughts were that it would be redundant with other telemetry that we've already got, and that we shouldn't bother. Can you unpack that a bit, either here or in https://docs.google.com/document/d/1wEQEXjcSZGE8VP5qLmfS9MRmbbLY02R_4Z8-TL8OG_I/edit#
.

I'm assuming we don't need this for the VPN 98 experiment, but if there's some reason we might, we should get it in right away so we can uplift.

Also, do we need to get a data scientist's thoughts here?

Comment 1

[Punam Dahiya [:pdahiya]]

To be updated after feedback from @andrei on why PrivateBrowsing feature in messaging system doesn't need reach.

Comment 2

[Beth Rennie [:beth] (she/her)]

Redirecting ni to dmose since andrei has left.

Do we still need this?

Comment 3

[Beth Rennie [:beth] (she/her)]

Removing priority so this shows up in triage

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

I think we probably want to implement reach on all of our feature ids to reduce complexity and confusion, but that needs a bit more discussion, perhaps a brief chat when we triage this will be enough.

Assuming we do decide to go ahead with that, there is the question of whether that's appropriate to do with private browsing, since it's a fairly unique surface in terms of telemetry. I'd be interested to hear from product, and if they're interested, then I suspect there's some due diligence to do here to find previous private browsing telemetry docs, and then make a proposal and have it reviewed by a data-steward and/or legal.

Comment 5

[Shane Hughes [:aminomancer]]

Dan to figure out what to do with this bug.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

We should go ahead with this, for the reasons described in bug 1877788 comments 0 and 1.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

To clarify comment 6, the next step here is to reach out to someone in policy/legal (mfeldman?) about what makes sense to do for pbNewtab. Good (though possibly not entirely sufficient) context can be had by reading https://experimenter.info/messaging/telemetry and https://bugzilla.mozilla.org/show_bug.cgi?id=1877788#c0.

I've set this to be a defect, because it can cause issues during experiment analysis.

If the decision here is to do something other than add reach to pbNewtab, we should at the very least document this in appropriate places that Data Science will find.

Comment 8

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya [:pdahiya] from comment #1)

To be updated after feedback from @andrei on why PrivateBrowsing feature in messaging system doesn't need reach.

Historically we hadn't needed reach ping on pbNewTab surface as we never ran experiments with significant bias between branches (respective targeting used) for users opening private browsing newtab.

https://experimenter.services.mozilla.com/nimbus/?allFeatureConfigs=DESKTOP%3ApbNewtab&tab=completed

Reach is recorded for features that uses trigger. Hypothetically we can change pbNewTab messaging surface to use a trigger pb_newtab instead of current implementation of using template pb_newtab but we should do so after weighing in return of effort and need. Do we foresee branches of pbNewtab feature experiments using message targeting to be drastically different causing bias in one branch more than other. This can be resolved by using smart message targeting.

https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/browser/components/asrouter/modules/ASRouter.jsm#1990

https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/browser/components/asrouter/modules/ASRouter.jsm#1905

Comment 9

[Shane Hughes [:aminomancer]]

Good point, we also haven't run pbNewtab experiments in a long time, to my knowledge. Last PB newtab promo we ran (cookie banners?) was just landed on train, and I'm not sure if it ever was actually shipped. Makes sense to (for now) just enable reach for those feature IDs that wouldn't require refactoring to actually record reach — so, only feature IDs for messages that use triggers.

Comment 10

[Shane Hughes [:aminomancer]]

It looks like PB newtab sends both exposure and impressions. The case where these differ from reach is where the user has already exhausted the local messages and is in the control branch for an experiment. Because we only record exposure when a message is shown, not when it could have been shown if the user was in the treatment group.

This is the same situation we have with exposure events for triggered messages, in bug 1877788. This can be problematic because experimenter is set up to analyze according to exposures. When we measure impact, should it be relative to the number of users enrolled, or the number of users who could have been reached? Experimenter tries to provide both options, but for messaging experiments, the latter doesn't work because we only record exposure when a message is actually shown. Exposure is also redundant in messaging experiments because it's functionally the same as impressions.

I don't think we've yet agreed on how to deal with that issue. It seems like maybe experimenter should be set up to use reach. I think most people assume "exposure" means the user actually saw something. But in experiments for triggered messages, where the control group doesn't see anything and the treatment sees something, we need an analysis basis. So, should we change ASRouter so that it also records exposure when a message from an unenrolled branch would have shown if the user was enrolled in that branch? But then exposure would be identical to reach. And it would be confusing because the control audience wasn't truly exposed to anything at all. An alternative (and maybe better) approach would be to update experimenter so that it can use reach events as an analysis basis, rather than just exposures or enrollments.

There does seem to be a hole in our current telemetry, but it's the same hole we have in general for all messaging experiments. So I'm thinking we should seek a consensus (between us, velocity PMs, DS, anyone else?) about what exposure should mean and what basis we want to use for messaging experiments. That would inform whether we 1) start recording exposure for pbNewtab even for inactive branches, 2) start recording reach for pbNewtab the same way we do for triggered messages, or 3) just leave this as-is.

Comment 11

[Shane Hughes [:aminomancer]]

Just to unblock us on this, what I think we should do in this bug is to start recording exposure for pbNewtab even for inactive branches, as exposure is intended to be used. Over here, we only record if ASRouter gives us a message. We should change this to call the record method no matter what, as recordExposureEvent() already prevents recording if the user is not enrolled in an experiment/rollout on the pbNewtab feature. That'll be a straightforward fix, just a couple lines of code with no testing requirement.