1855562 - Invalidate MLS cache once per day

Status: RESOLVED FIXED

(Firefox for Android :: Search, task, P1)

Description

[Joe M [:jmahon]]

Per Bug 1854988, we've been seeing some issues with unexpectedly high volume of MLS (Mozilla Location Services) network calls. We assume this is because we're not caching the results, and are re-requesting location too frequently (e.g. every time the app opens, or every time the page appears, or something like that).

Bug 1854988 will make sure we cache the results.

But, we also want to be sure that the location response can be occasionally refreshed as the user moves around day-to-day - as a baseline, let's make sure the cache results are considered invalid after one day. We may want to enhance this to be a bit "smarter" or choose a higher frequency if this proves to be too infrequent.

Note: Please also make sure to review this bug with the Search and Pocket PMs before landing it, to make sure they don't spot any obvious flaws, since this affects their services. See Jira ticket for their contact info.

Comment 1

[Joe M [:jmahon]]

:boek, any chance you can work on this alongside bug 1854988, or would you prefer to delegate this to someone else?

Comment 2

[Mark Banner (:standard8)]

(In reply to Joe M [:jmahon] from comment #0)

But, we also want to be sure that the location response can be occasionally refreshed as the user moves around day-to-day - as a baseline, let's make sure the cache results are considered invalid after one day. We may want to enhance this to be a bit "smarter" or choose a higher frequency if this proves to be too infrequent.

On Desktop, the region helps determines a number of items, including Search Engines, Form Auto Fill, Top Sites etc. We cache the region indefinitely, but monitor for changes (iirc, we ping once a day) - if the region is different for more than 14 days, then we'll change the user's region.

The idea was that if the user is on holiday for a week or so, they don't necessarily want their search engines (or other items) changed to the new region they are in. If they are there more permanently, then we should be updating.

The logic for this is all found in Region.sys.mjs.

So assuming MLS is only driving regional information, we might want to do something similar on the mobile side. Adding NI for info.

Comment 3

[Joe M [:jmahon]]

Thanks, Mark. That seems like a reasonable enhancement to further optimize, but I just want to make sure we're aligned on the overarching situation:

Solution	Effort	Effect
no caching	(existing behavior)	Avg 10-20 MLS requests per user per day. Region changes perpetually as user moves around.
caching, no invalidation	low - bug 1854988	one MLS request per user, ever. Region never changes.
caching, daily invalidation	low - this bug	Up to one MLS request per user per day. Region changes daily.
caching, 'smart' invalidation	???	MLS requests only when user has moved significantly. Region changes only "as needed".

Is it safe to assume that the "smart" invalidation strategy you referenced would be a nice-to-have, but that we should still move forward with this quick fix to at least rate-limit the current tide of overkill MLS requests?

Comment 4

[Mark Banner (:standard8)]

(In reply to Joe M [:jmahon] from comment #3)

| caching, 'smart' invalidation | ??? | MLS requests only when user has moved significantly. Region changes only "as needed". |

I think we still request location once a day (so that we know when the region changes), but we'll only update the in-use region after 2 weeks.

Is it safe to assume that the "smart" invalidation strategy you referenced would be a nice-to-have, but that we should still move forward with this quick fix to at least rate-limit the current tide of overkill MLS requests?

Yes, that would totally be fine, I don't want to make quick fix longer, but thought it was worth mentioning as it may be an idea to align what we do.

Comment 5

[Joe M [:jmahon]]

(In reply to Mark Banner (:standard8) from comment #4)

(In reply to Joe M [:jmahon] from comment #3)

| caching, 'smart' invalidation | ??? | MLS requests only when user has moved significantly. Region changes only "as needed". |

I think we still request location once a day (so that we know when the region changes), but we'll only update the in-use region after 2 weeks.

Ah, gotcha. It might be beneficial to use a different strategy on mobile, e.g. using the on-device location provider, to prevent the extra networking calls, if possible. But I'll leave the pondering up to whoever winds up working on this in the future.

Is it safe to assume that the "smart" invalidation strategy you referenced would be a nice-to-have, but that we should still move forward with this quick fix to at least rate-limit the current tide of overkill MLS requests?

Yes, that would totally be fine, I don't want to make quick fix longer, but thought it was worth mentioning as it may be an idea to align what we do.

Awesome - I'll leave this ticket as-is, but open a new task to track that as a potential follow-up improvement later on. (Edit: task created, Bug 1862732).

Comment 6

[Jeff Boek [:boek]]

Ah, gotcha. It might be beneficial to use a different strategy on mobile, e.g. using the on-device location provider, to prevent the extra networking calls, if possible. But I'll leave the pondering up to whoever winds up working on this in the future.

Long term we probably want to integrate using the on-device location provider when / if the user grants permission and fall back to using MLS if we don't have permission. I will create a follow up bug for future improvements.

After looking into it I think the solution for now will be caching with daily invalidation.

Comment 7

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1855562 - RegionManager now uses the cached result from its `LocationService`. (#4497)

Comment 8

[BMO Github Automation]

Authored by https://github.com/boek
https://github.com/mozilla-mobile/firefox-android/commit/f303e68a76315bccc98f8940491abe79d2d5db31
[main] Bug 1855562 - Adds a cache lifetime to MozillaLocationService.

Authored by https://github.com/boek
https://github.com/mozilla-mobile/firefox-android/commit/439e2a5b9154371e7e81198850da103ad8303410
[main] Bug 1855562 - RegionManager now uses the cached result from its LocationService.

Comment 9

[Mira Lobontiu (Android QA)]

Hello!
Is there anything that the QA team can manually verify on this ticket?
Thank you!

Comment 10

[Joe M [:jmahon]]

(In reply to miralobontiu from comment #9)

Hello!
Is there anything that the QA team can manually verify on this ticket?
Thank you!

Fwding to Jeff!

Comment 11

[Jeff Boek [:boek]]

Outside of monitoring network requests to MLS, I don't think so. Our best course will be to ask MLS if the amount of traffic they are seeing from Fenix has improved :)

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Jeff Boek [:boek] from comment #11)

Outside of monitoring network requests to MLS, I don't think so. Our best course will be to ask MLS if the amount of traffic they are seeing from Fenix has improved :)

I asked Benson on the MLS team to check. He raised this Android caching issue in the first place.

If the MLS requests look better, we should consider uplifting to Beta 121. No need to rush this change from Nightly 122 directly to a 120 dot release.

Comment 13

[Benson Wong [:mostlygeek]]

Nightly is too small of a population to see any traffic impact. We'll likely only be able to see any real impact when it rolls out to Release.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I discussed this with Benson and it sounds like there's value in getting this uplifted to 121. Can you please create the backport PR and request Beta approval, Jeff? Funny enough, given the proximity to merge day of when this PR merged, the changelog entry for it actually ended up in 121's anyway 😳.

Comment 15

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1855562 - RegionManager now uses the cached result from its `LocationService`. (backport #4497) (#4714)

Comment 16

[Jeff Boek [:boek]]

Comment on attachment 9367151 [details] [review]
[mozilla-mobile/firefox-android] Bug 1855562 - RegionManager now uses the cached result from its LocationService. (backport #4497) (#4714)

Beta/Release Uplift Approval Request

• User impact if declined: No impacts to users, but we will continue to put unnecessary pressure on MLS.
• 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): Commit has baked in Nightly without any reported regressions.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9367151 [details] [review]
[mozilla-mobile/firefox-android] Bug 1855562 - RegionManager now uses the cached result from its `LocationService`. (backport #4497) (#4714)

Approved for 121.0b8.

Comment 18

[BMO Github Automation]

Authored by https://github.com/boek
https://github.com/mozilla-mobile/firefox-android/commit/e6b147de8e2801da6b9e47b990c1513a9779bb70
[releases_v121] Bug 1855562 - Adds a cache lifetime to MozillaLocationService.

Authored by https://github.com/boek
https://github.com/mozilla-mobile/firefox-android/commit/4d84bae3f5365b5a5fb215751893f531b3df2a31
[releases_v121] Bug 1855562 - RegionManager now uses the cached result from its LocationService.