1600779 - Caching in ASRouterTargeting for attachedFxAOAuthClients

Status: VERIFIED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Punam Dahiya [:pdahiya]]

attachedFxAOAuthClients getter makes an expensive call listAttachedOAuthClients to FxAccount API https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/ASRouterTargeting.jsm#525

We should have a basic caching in place for attachedFxAOAuthClients getter
See https://bugzilla.mozilla.org/show_bug.cgi?id=1591584#c8

Comment 1

[Punam Dahiya [:pdahiya]]

NI Ryan to help arrive at best conditions to invalidate cache. Possible options
a) Time period after which ASRouter code can invalidate cache and make fxAccounts.listAttachedOAuthClients() call
b) An event that we can listen to that indicates its time to reload cache.

Thanks!

Comment 2

[Ryan Kelly [:rfkelly]]

Thanks for following up on this!

b) An event that we can listen to that indicates its time to reload cache.

There is an fxaccounts:device_connected notification that is fired on some changes to this list, so it's probably worth listening for that, but we'll need to add a time-based invalidation as well.

a) Time period after which ASRouter code can invalidate cache and make fxAccounts.listAttachedOAuthClients() call

I suggest 2 hours as a good starting point.

What are the consequences of this list being out-of-date? Is it limited to us potentially showing a mis-targeted campaign, e.g. telling the user about Monitor even though they already signed up for Monitor in the recent past?

You might be able to mitigate that somewhat by clearing the cache if/when a user interacts with one of the campaigns.

Comment 3

[Punam Dahiya [:pdahiya]]

(In reply to Ryan Kelly [:rfkelly] from comment #2)

Thanks for following up on this!

b) An event that we can listen to that indicates its time to reload cache.

There is an fxaccounts:device_connected notification that is fired on some changes to this list, so it's probably worth listening for that, but we'll need to add a time-based invalidation as well.

a) Time period after which ASRouter code can invalidate cache and make fxAccounts.listAttachedOAuthClients() call

I suggest 2 hours as a good starting point.

What are the consequences of this list being out-of-date? Is it limited to us potentially showing a mis-targeted campaign, e.g. telling the user about Monitor even though they already signed up for Monitor in the recent past?

As of now, fxAccounts.listAttachedOAuthClients() is used in 'Firefox Monitor' card shown on opening a new tab inside dynamic triplets.
If the list is out-of-date, the consequences is limited to showing 'Monitor' card to user who has already signed up for Monitor. I believe caching attachedFxAOAuthClients for 2 hours should greatly reduce this possibility.

You can test in Nightly for a new profile by setting 'trailhead.firstrun.branches' value as 'join-dynamic'.

You might be able to mitigate that somewhat by clearing the cache if/when a user interacts with one of the campaigns.

Interaction such as click on card hides the card on subsequent reload or opening of new tab ( its assumed message is communicated and safe to replace the card).

Comment 4

[Punam Dahiya [:pdahiya]]

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

(In reply to Ryan Kelly [:rfkelly] from comment #2)

Thanks for following up on this!

b) An event that we can listen to that indicates its time to reload cache.

There is an fxaccounts:device_connected notification that is fired on some changes to this list, so it's probably worth listening for that, but we'll need to add a time-based invalidation as well.

Hi Ryan, is it possible that above event is not triggered for Firefox Monitor Logins. If yes, is https://dxr.mozilla.org/mozilla-central/source/browser/components/BrowserGlue.jsm#860 right place to log to check if the event is getting fired on Firefox Montior login. Thanks!

Comment 5

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1600779 - Caching in ASRouterTargeting for attachedFxAOAuthClients

Comment 6

[Ryan Kelly [:rfkelly]]

Hi Ryan, is it possible that above event is not triggered for Firefox Monitor Logins.

Yes, I think you're right, this event is probably not getting triggered for Firefox Monitor logins. It's designed more for new sync logins at the moment. The time-based cache expiry will be more useful to you in this case.

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/integration/autoland/rev/440d0842a0aa25871aeb0b1b98c4292e7bb22e8d

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/440d0842a0aa

Comment 9

[Punam Dahiya [:pdahiya]]

Hi Ryan

The patch that caches output from listAttachedOAuthClients is in nightly. Was thinking of QA test steps and thought should check with you if there is some form of logging at FxA end that QA can use to verify that listAttachedOAuthClients is not getting called before 2 hours. Thanks!

Comment 10

[Andrei Oprea [:andreio]]

We're going to want to uplift this into Beta once QA verifies.

Comment 11

[Ryan Kelly [:rfkelly]]

Was thinking of QA test steps and thought should check with you if there is some form of logging at FxA end that QA can use
to verify that listAttachedOAuthClients is not getting called before 2 hours.

In the past we've done this by watching for the outgoing request in the browser console, see e.g. Bug 1591312 Comment 16. I think something similar should work here as well.

Comment 12

[Punam Dahiya [:pdahiya]]

(In reply to Ryan Kelly [:rfkelly] from comment #11)

Was thinking of QA test steps and thought should check with you if there is some form of logging at FxA end that QA can use
to verify that listAttachedOAuthClients is not getting called before 2 hours.

In the past we've done this by watching for the outgoing request in the browser console, see e.g. Bug 1591312 Comment 16. I think something similar should work here as well.

perfect, will add QA steps to check for '/account/attached_clients' requests in browser console. Thanks!

Comment 13

[Punam Dahiya [:pdahiya]]

QA Steps to verify:

1. Launch firefox with a new profile
2. Set trailhead.firstrun.branches to join-dynamic
3. Open the Browser Console and in the toolbar at the top of the console, toggle the "Requests" options to "on" if they are not already
4. In the Browser Console, filter the logs by the word “attached_clients”.
5. Click on Sync and complete Sync signin using email address
6. Open or reload new tab
7. Check that you see only one GET request to 'https://api.accounts.firefox.com/v1/account/attached_clients'
8. Reload new tab and verify there are no additional GET requests to /attached_clients
9. Set pref ’browser.newtabpage.activity-stream.asrouter.devtoolsEnabled’ to true and open about:newtab#devtools
10. In General -> Message Providers -> Clear onboarding impressions by toggling check box against ‘onboarding’ Message provider
11. Clear cache by clicking on Targeting -> Expire Cache
12. Open and Reload new tab and verify there is one new GET request to https://api.accounts.firefox.com/v1/account/attached_clients

Comment 14

[Valentina Virlics [:vvalentina], Ecosystem QA]

I have verified that the issue is no longer reproducible on Firefox Nightly 73.0a1 (Build ID 20191213094653) using Win 10, Mac 10.14 and Linux Debian 9.
The GET request is displayed after complete Sync (if an already created account is used) and a second GET request is displayed after clearing the onboarding impressions and cache (steps 10->12) .
The first GET request is not displayed if a new account is used. Is this intended?

Comment 15

[Punam Dahiya [:pdahiya]]

The first GET request is not displayed if a new account is used. Is this intended?

That's correct, its expected and have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1603803 to handle edge cases where fxAccounts.listAttachedOAuthClients() API call fails to trigger GET request and return with an error. Thanks!

Comment 16

[Valentina Virlics [:vvalentina], Ecosystem QA]

Based on the above comment I can mark this issue as verified.

Comment 17

[Punam Dahiya [:pdahiya]]

Comment on attachment 9114269 [details]
Bug 1600779 - Caching in ASRouterTargeting for attachedFxAOAuthClients

Beta/Release Uplift Approval Request

• User impact if declined: attachedFxAOAuthClients is an expensive call and caching it helps improve performance
• 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: https://bugzilla.mozilla.org/show_bug.cgi?id=1600779#c13
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small patch that introduces caching in targeting condition attachedFxAOAuthClients and its usage is verified in nightly.
• String changes made/needed: None

Comment 18

[Julien Cristau [:jcristau]]

Can you give a bit more details on the impact of shipping without this? Is this about reducing server load (that's what I understand from the linked comment)? How bad is it? Is this a new issue?

Comment 19

[Andrei Oprea [:andreio]]

Hey, I can take this, Punam is on PTO. The call takes at least 200+ms best case and it can happen very often. For example snippets use it and because it is not cached every time you open a new tab that call is made. If lets say 3 snippets use the same attribute (and we iterate through all to decide which to show) opening a new tab has a potential penalty of 600+ms. The attribute landed in 71 but we haven't started using it until more recently. When we did reports started coming in: see blocked bugs in bug 1603483.

We also have telemetry which shows that now in nightly processing time is at most 200ms in the long tail for the past 3 days (after we removed the messages and landed this caching patch) but if more days are included it spikes to 2s+.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 9114269 [details]
Bug 1600779 - Caching in ASRouterTargeting for attachedFxAOAuthClients

ok let's get this in 72.0b9

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecf0db1da560