1805470 - msgs with multiple groups only get impressions written to one group

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

If a message has multiple entries listed in the groups array, only one of them will get an impression written. Which group gets written is deterministic, and I suspect it's the last one read in from the groups provider, but I need to verify that.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Bug 1805470 - FxMS should save impressions to all groups in a message, r?barret

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cad1670a2076
FxMS should save impressions to all groups in a message, r=barret

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cad1670a2076

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

It would be great to have some manual test coverage on this so that we don't have to wait for another train cycle to fix any issues; adding qe-verify?

Comment 5

[Simona Rosu [:srosu], Ecosystem QA]

Hi, @Dan! Thanks for flagging this issue for QA! Could you please provide us with testing steps for verifying this issue?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

[Tracking Requested - why for this release]:

We're hoping to roll out a couple of onboarding messages in 109 Release that we hope will effect CDOU/DAU in this release, and having this bug fixed will make it possible to avoid annoying users by showing them too many dialogs.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

We've already built the Fx109 RC. Is this something we need to respin for or is a dot release ride-along sufficient?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Ryan, two questions:

• besides the two possibilities you suggest, would a ride-along on an RC2-if-that-happens be a third option?
• is it correct that the new/current reality is that there will always be a dot-release two-weeks after a regular release? So far, I haven't found anything on fx-trains or wiki.m.o about this one way or the other...

Venetia, this affects both experiments and rollouts. What do you think re Ryan's question?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dan Mosedale (:dmosedale, :dmose) from comment #8)

• besides the two possibilities you suggest, would a ride-along on an RC2-if-that-happens be a third option?

By respin, I was referring to an RC2. RC1 is already out the door.

• is it correct that the new/current reality is that there will always be a dot-release two-weeks after a regular release? I don't see anything on fx-trains about this one way or the other...

Yes, since July (Fx103). It's also on fx-trains as the "Planned dot release" at the bottom of the milestones list for each release.

Comment 10

[Vtay]

dot release works!

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

@srosu:

Preparation:

• create or clone a simple messaging experiment

• adjust the targeting at the nimbus level to be simple (no advanced targeting)

• adjust the targeting at the message level on the treatment branch to be non-existent ("targeting"=true) or very simple

• set the groups at the message level on the treatment branch to include two groups ("groups": ["import-spotlights", "eco"])

• put the experiment into preview

• Clone the treatment branch of the above experiment into a rollout, and put it into preview.

Testing (do this once for the experiment and once for the rollout):

• use the appropriate URL from experimenter to force enroll yourself the branch that will show the message
• in the browser console, do

const lazy = {};
XPCOMUtils.defineLazyModuleGetters(lazy, {
  ASRouter: "resource://activity-stream/lib/ASRouter.jsm",
});
lazy.ASRouter.state.groupImpressions

There may or may not be arrays for import-spotlights and/or eco. Note this, as well as the contents of any of these arrays that do exist.

• trigger the message (based on whatever trigger is specified at the message level)
• if triggering the message involved restarting the browser, re-import the ASRouter module in the browser console the same way as before
• in the browser console, look at lazy.ASRouter.state.groupImpressions again
• Verify that there are now Arrays for both groups and that each array has one more number than at the previous check.

Comment 12

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this issue using the latest Firefox Nightly 110.0a1 (Build ID: 20230109212101) on Windows 10 x64, macOS 11.7.1, and Ubuntu 22.04 x64.

• To verify this, I created an experiment and a rollout recipe on the Stage server.

• After following the testing steps in the comment above, I was able to see Arrays for both groups only once out of 10 profiles.

• After forced enrollment in experiment/rollout, and run "XPCOMUtils.defineLazyModuleGetters(lazy, { ASRouter: "resource://activity-stream/lib/ASRouter.jsm", }); lazy.ASRouter.state.groupImpressions" syntax I didn't see any array in the object, but after triggering the message and running the "lazy.ASRouter.state.groupImpressions" syntax again, I'm seeing the Array only for the "import-spotlights" group.

  @ Dan, could you please let me know if I'm missing something? Or is that expected?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

The schedule for upcoming rollouts has just shifted so that this is no longer planned for 109 release, but 110 release instead.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

We

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

There was some discussion around trying to make eco the preferred group that gets used in an experiment which would start before this makes it to release. Ultimately, that was decided against. For the record, before this fix:

The group that gets an impression saved appears to be the last group with frequency in ASRouter.state.groups. I'm seeing a specific order for that array in the few builds I've tested: [cfr, cfr-experiments, eco, import-spotlights, moments-page]. Also interestingly, the order in that array is different than the order that remote-settings devtools shows. Which makes me suspect things are being sorted on the way, though I haven't yet tracked down where. Not pursuing further, as we have bigger fish to fry.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

[Tracking Requested - why for this release]:

Another experiment that is planned to run in the 109 release cycle is going to need this fix, so I'm renominating for tracking for 109.0.1. Will dig into the QA question around this early next week.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

This is not needed for 109.1; removing the tracking flag.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Simona, at least one thing going on was that the message groups remote settings bucket on stage did not have a "eco" group. I've added one in. If you could take another look, that'd be super helpful...

Comment 19

[Simona Rosu [:srosu], Ecosystem QA]

Hi Dan, I‘ve verified this issue using the latest Firefox Beta 110.0b8 (Build ID: 20230131190033) on Windows 10 x64, macOS 11.7.1, and Ubuntu 22.04 x64.

• After following the testing steps from comment 11, I confirm the following:
  • No arrays are returned before the experiment/rollout's message is shown;
  • After the experiment/rollout’s message is shown the “Object { eco: (1) […], "import-spotlights": (1) […] }” arrays are returned.

Considering these, I’m marking this issue as Verified.