1529730 - Impressions on spocs for hero regression

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4812

Comment 2

[Scott [:thecount] Downe]

[Tracking Requested - why for this release]:

Comment 3

[Scott [:thecount] Downe]

To test:

1. Set browser.newtabpage.activity-stream.discoverystream.config to {"api_key_pref":"extensions.pocket.oAuthConsumerKey","enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=$apiKey&layout_variant=dev-test-all"}
2. Scroll down until you can no longer see "Hero (with borders, 5 items, with a Spoc)"
3. Check browser.newtabpage.activity-stream.discoverystream.spoc.impressions
4. Go back to the original about:home page from step 2.
5. Refresh it a few times.
6. The value in browser.newtabpage.activity-stream.discoverystream.spoc.impressions should now have grownn with more impressions.

Comment 4

[Ed Lee :Mardak]

Attachment: Bug 1529730 - Impression capping for spocs on hero

Comment 5

[Ed Lee :Mardak]

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

Comment 6

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4817

Comment 7

[Scott [:thecount] Downe]

Attachment: Bug 1529730 - Impression capping for spocs on hero

Comment 8

[Scott [:thecount] Downe]

Comment on attachment 9046252 [details]
Bug 1529730 - Impression capping for spocs on hero

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1522832
• User impact if declined: Experiments (Would do weird things to the results because user would see spocs too often/long than the control in a way we are not testing)
  user experience (User don't expect to see spocs this often/long)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Set browser.newtabpage.activity-stream.discoverystream.config to {"api_key_pref":"extensions.pocket.oAuthConsumerKey","enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=$apiKey&layout_variant=dev-test-all"}

2. Scroll down until you can no longer see "Hero (with borders, 5 items, with a Spoc)"
3. Check browser.newtabpage.activity-stream.discoverystream.spoc.impressions
4. Go back to the original about:home page from step 2.
5. Refresh it a few times.
6. The value in browser.newtabpage.activity-stream.discoverystream.spoc.impressions should now have grownn with more impressions.

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It's 1 line, which would normally have me click low risk on this.

But given the time, and the fact that I'm getting this request out there while still waiting on QA to verify on nightly and the phabricator patch to be reviewed. Once those to things are done this is a def low risk patch, so I'm happy to wait for those but wasn't sure if waiting or sending the request now would be better.

• String changes made/needed: None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9046252 [details]
Bug 1529730 - Impression capping for spocs on hero

OK, let's uplift for beta 11.

Comment 10

[Scott [:thecount] Downe]

QA verification for nightly needed.

1. Set browser.newtabpage.activity-stream.discoverystream.config to {"api_key_pref":"extensions.pocket.oAuthConsumerKey","enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=$apiKey&layout_variant=dev-test-all"}
2. Scroll down until you can no longer see "Hero (with borders, 5 items, with a Spoc)"
3. Check browser.newtabpage.activity-stream.discoverystream.spoc.impressions
4. Go back to the original about:home page from step 2.
5. Refresh it a few times.
6. The value in browser.newtabpage.activity-stream.discoverystream.spoc.impressions should now have grownn with more impressions.

Feel free to ping me with any issues verifying it.

Comment 11

[Brahmini Nagabandi]

Observation on Windows 10 Pro (VM)
I had basic end-point initially - After I change it to dev-test-all (per bug) , there is a slight delay on loading new tab --> since the new tab still has basic’s spoc.. spoc.impressions was able to capture time stamp of this --> later after new tab loaded with dev-test-all layout --> spoc.impressions continues to capture latest timestamps in addition to the basic timestamp --> In general dev-test-all doesn't have spoc on the first visibility of page (user need to scroll in order to see first spoc)

I have a recording after initial timestamp (which is basic layout's spoc) :
https://www.dropbox.com/s/i1jrfmyimive2we/hero%20spocs%20-%20bug%201529730.mp4?dl=0

Comment 12

[Scott [:thecount] Downe]

Yup that all looks right to me. I counted along the spocs you see and the impressions.

In your video:

1. It starts at 1 impression from basic.
2. You scroll down dev-test-all and pass/view 2 more, total is 3.
3. You scroll down a bit more and see 1 more. There is another almost visible, but it's not quite 50% displayed so it shouldn't count. Total is now 4.
4. You refresh, and two are displayed. The one is fully displayed, and there is one at the top that is 50%+ in view. Total is now 6.
5. You refresh the same page again without scrolling, so another +2 making total 8. The actual breakdown at this point is 788x3, 1286x3, and 1397x2.
6. Refresh 1 more time, making the total 10 with two more view of 1397.

Step 5-6 shows an interesting edge case where you can 4 spoc impressions even though the limit is 3, because it only checks the limit when it first renders the page, not when it renders each spoc. This causes the page when it renders those 2 new spocs, they both render before you've seen them, thus they both happily render because they are both still at only 3 views. It's known unrelated issue that we've got a fix for in 67 here: https://bugzilla.mozilla.org/show_bug.cgi?id=1522899

Comment 13

[Brahmini Nagabandi]

Awesome, thanks for clarifying Scott. This is helpful.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This shall land on beta after t has been QA-verified. Is that correct?

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Actually, I'm ok with this landing for beta 12.
It might need a little extra testing in beta once it lands.

Comment 16

[Brahmini Nagabandi]

Attachment: QA Results - bug 1529730 (Mac OS).png

QA Results:

Tested on :

FF Nightly version : 67.0a1 (2019-02-25)
OS : Mac and Windows 10 Pro

Works as expected.

QA Results - bug 1529730 (Mac OS).png

For Windows there's a recording in comment# 11.

Marking as Verified

Comment 17

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/2c312025ce11

Comment 18

[Brindusa Tot, Desktop QA]

Brahmini Nagabandi, can you please verify this issue on latest Firefox 66 Beta 12(http://archive.mozilla.org/pub/firefox/candidates/66.0b12-candidates/build1/)

Comment 19

[Brahmini Nagabandi]

QA Results:

Tested on :

FF Beta version : 66.0b14 (64-bit)
OS : Mac and Windows 10 Pro

Verified on the latest Beta. Works as expected.

Marking as verified.