1601662 - Querying `fxa` using UITour takes a long time to respond

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P2)

Description

[Alex Gibson [:agibson]]

"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:73.0) Gecko/20100101 Firefox/73.0"

I'm working on implementing bug 1548404 on mozorg, and one noticable regression I'm seeing is the time it takes for this new event to return.

Querying the sync configuration returns data in around 50-60ms, but querying fxa returns data in 600-700ms, which is quite a considerable difference. I think this is an important factor to consider, seeing as things like web page UI's displaying conditional content may be reliant upon it.

Is this increase in response time expected considering the new data that's being queried and returned, or should this be considered a bug? Could anything be optimized here?

Comment 1

[Alex Gibson [:agibson]]

Would you be able to help debug at all here, Mark?

Comment 2

[Alex Gibson [:agibson]]

Attachment: test-case.js

Minimal test case

Comment 3

[Mark Hammond [:markh] [:mhammond]]

We are hitting the server to discover the attached services. These aren't cached locally because we don't need them locally. I guess we could consider splitting this into 2 different configuration calls - one to fetch what's known locally (eg, account and sync state) and what is going to have higher latency (attached services and devices)?

Comment 4

[Alex Gibson [:agibson]]

I would be fine to keep the current configuration call as-is, but if we had the option of using a second, faster call to return only account and signed in state, then I think this may give us a better amount of flexibility to pick and choose depending on how & where it's being used. Would that be simpler?

Comment 5

[Alex Gibson [:agibson]]

Alternatively, could we look at caching the results of the attached services locally? Is there a reason why we need to hit the servers on every request from a web page that uses UITour?

Comment 6

[Vijay Budhram]

I guess we could consider splitting this into 2 different configuration calls - one to fetch what's known locally (eg, account and sync state) and what is going to have higher latency (attached services and devices)?

This seems reasonable to me. I suspect the only reason to hit the servers more frequently is because a user's attached services and devices probably change more often than their account and sync state.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

I guess I'm a bit confused about where this is going to end up.

The most common use-case here is going to be immediately after the browser starts - so caching locally probably isn't going to help much (unless we persist to disk, and I don't think we can make a good case for doing that just for UITour).

So, if UITour ends up wanting to know whether (say) Lockwise is being used, it's going to need to accept it's a relatively high-latency request. Presumably, if it wants to know that, it's good a good reason for wanting to know - eg, it's probably going to promote lockwise if it is not used.

So what's the UI going to show while it's waiting for that high-latency operation? Presumably it's not going to default to promoting lockwise, just to then hide that promotion once it determines it is already in use.

In other words, ISTM that having UITour change what is shown after the high-latency operation completes is going to look jarring to the user, so ultimately is going to end up waiting for the high-latency operation anyway. That possibly not true today because it doesn't currently care about the attached services (ie, it doesn't currently care whether lockwise is used or not) - but what happens once it does?

But - if we want to charge ahead with this plan, would we prefer a separate configuration option, or a param to the existing one?

Comment 8

[Alex Gibson [:agibson]]

The most common use-case here is going to be immediately after the browser starts - so caching locally probably isn't going to help much (unless we persist to disk, and I don't think we can make a good case for doing that just for UITour).

This will be true for things like /whatsnew pages that open up after a Firefox update. But it's also used in many other areas of the website. To help give an idea, there's a "Get a Firefox Account" button in the main navigation at the top of nearly every mozorg page. So potentially, this combined with /whatsnew could end up generating millions of requests (I'm not sure if that is an issue, just calling it out). It could be that some level of caching might help with server load.

So what's the UI going to show while it's waiting for that high-latency operation? Presumably it's not going to default to promoting lockwise, just to then hide that promotion once it determines it is already in use.

If mozorg needs to handle a high latency request when making use of these new features, then I think we can design for that appropriately (maybe a loading spinner, or some form of UI indicator). We would need to work out a suitable timeout for really slow networks, but we can probably work through that. It's still important to consider though, that some of these promotions will likely be the main, most critical thing on their respective pages. Making our websites as fast and performant as possible, is still an important task here.

A lot of existing content on the site today is built around the expectation of a < 100ms call to determine what content to show. All we have known so far is "is this person signed into Sync?", and that isn't something that's been a high latency call, until now. If we can keep locally known things as fast as possible, and then opt into high latency things when needed, that would likely give us the most flexibility when it comes to designing performant pages.

But - if we want to charge ahead with this plan, would we prefer a separate configuration option, or a param to the existing one?

A param to the existing call might make sense here, if that's simpler.

Comment 9

[Alex Gibson [:agibson]]

Just checking in - is there any form of consensus here as to how we might improve the current API, given some of the issues outlined above?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Sorry for the delay here. Adding a param to the fxa request isn't trivial so I went with adding a new command - the existing fxa no longer returns info about the account services nor about the devices - this is now returned in a new fxaConnections. I'm not particularly happy with the name, so feel free to bikeshed it and to request any other changes.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1601662 - split UITour's 'fxa' request into high and low latency versions

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Alex, I couldn't find you on phabricator (there's an "Alec Gibson", but I didn't want to assume that's you) - could you please take a look?

Comment 13

[Alex Gibson [:agibson]]

Thanks Mark, I appreciate the time it takes to come back and revise this. I understand this isn't ideal, but from a UITour consumer point of view I think splitting this out will help make our websites UI's more performant, as well as more resilient to network slowness / failure. I'm not the best person to review this code as I'm not all too familiar with the Firefox side things, but from a high level this looks OK to me. From a naming perspective fxaConnections seems fitting enough, or possibly fxaServices as an alternative.

Comment 14

[Alex Gibson [:agibson]]

Any updates at to when we can expect this to be available in Nightly?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d881ac5d2da
split UITour's 'fxa' request into high and low latency versions r=MattN

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6d881ac5d2da

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Alex, sorry for the delay here. Once you can confirm it work OK, let me know if you would like me to make a beta uplift request.

Comment 18

[Alex Gibson [:agibson]]

I've tested this locally in Nightly and the response time for querying fxa seems much faster (< 5ms on average). Querying fxaConnections is high latency as expected. An uplift to Beta would be much appreciated, thanks!

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 9119945 [details]
Bug 1601662 - split UITour's 'fxa' request into high and low latency versions

Beta/Release Uplift Approval Request

• User impact if declined: Onboarding pages account information will either be high-latency (if they use these facilities) or inaccurate (if they choose to not use this due to the latency)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Risk is limited to how our content pages query the state of the firefox account and sync. If things go very bad, the issues could be mitigated by these content pages.
• String changes made/needed: None

Comment 20

[(PTO May 7-12) Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119945 [details]
Bug 1601662 - split UITour's 'fxa' request into high and low latency versions

Fx73 is already in RC. Let's let this ride with Fx74.