Closed Bug 1577035 Opened 1 year ago Closed 1 year ago

[Protection Report] Cache monitor card data

Categories

(Firefox :: Protections UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 71
Tracking Status
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: ewright, Assigned: ewright)

References

(Regressed 1 open bug)

Details

Attachments

(1 file)

When we start promoting the protection report after it lands on release there will be an overload on the Monitor API. Currently, we are querying Monitor for information to find if the user has a Monitor account, and the data associated with that account. We make this query on every page load of about:protections.

This data does not change often, and we should find a way to cache it, this is also beneficial because the data takes a while to fetch.

  • We cannot use Local Storage because it is an about: page.
  • Perhaps we can use a JS object that gets reset per session.
    • This may cause more oveload (will there be more users starting new sessions, or more users visiting about:protections?)
  • Perhaps we can use IndexedDB

(In reply to Erica Wright [:ewright] from comment #0)

  • Perhaps we can use a JS object that gets reset per session.
    • This may cause more oveload (will there be more users starting new sessions, or more users visiting about:protections?)

Personally per-session caching sounds easiest to me right now, and I don't really understand the load concern. We would only be caching after the user visits about:protections for the first time in that session.

We might also need to invalidate the cache daily, or so.

(In reply to Johann Hofmann [:johannh] from comment #1)

We would only be caching after the user visits about:protections for the first time in that session.

Oh, I didn't understand that part.

Oh, yeah that sounds good to me too. When the user first visits about:protections, make the query to monitor.firefox.com, and cache the results for the rest of the session.

Do we have any early indicators from Nightly about how many times the about:protections page is viewed by users? I'd like to hammer our breach-stats endpoint with some load-testing to simulate it if we can.

We landed telemetry not too long ago, Leif may be able to provide more insight to preliminary results. Though since we haven't been marketing it I imagine they won't be representative.

Flags: needinfo?(loines)
Assignee: nobody → ewright
Status: NEW → ASSIGNED

What Erica said, I don't think this data is representative right now
https://sql.telemetry.mozilla.org/queries/64370/source#164280

A few hundred clients per day viewing the protection report.

Flags: needinfo?(loines)

Thanks for checking that. It's helpful to estimate the lower-bound of sustained pings we might expect after the initial spike.

Looking at the ratio of distinct client_id to all client_id (unique opens vs. opened multiple times) shows that usually around 60% - 80% of users open the report only once per day. Not 100% sure what to make of that... Should we cache the result for the rest? Is it even worth doing that? Do we need to cache for longer than a day? I didn't explore about the behavior on whether the same users repeatedly open the report over and over again.

I get the feeling that we might want to wait on the effect and telemetry results from Beta before we move forward with this...

Leif also made this query for me, it may help in knowing how often users re-open the report.
https://sql.telemetry.mozilla.org/queries/64371/source

Pushed by ewright@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67050c8eab8a
Cache Monitor data to reduce the load on Monitor API. r=johannh
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Comment on attachment 9088907 [details]
Bug 1577035 - Cache Monitor data to reduce the load on Monitor API.

Beta/Release Uplift Approval Request

  • User impact if declined: This adds caching to the monitor card on the protection report. This should add a small performance benefit to the load time of that card, as well as lighten the load on the monitor API when 70 moves to release and we start marketing for the protection report.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • 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): simple cache using a JS object, clears every 24 hours.
  • String changes made/needed: none
Attachment #9088907 - Flags: approval-mozilla-beta?

Comment on attachment 9088907 [details]
Bug 1577035 - Cache Monitor data to reduce the load on Monitor API.

Perf improvement for Skyline feature, let's uplift for beta 6.
Is there any particular verification you would like to see here (in beta?)

Flags: needinfo?(ewright)
Attachment #9088907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(In reply to Liz Henry (:lizzard) from comment #13)

Comment on attachment 9088907 [details]
Bug 1577035 - Cache Monitor data to reduce the load on Monitor API.

Perf improvement for Skyline feature, let's uplift for beta 6.
Is there any particular verification you would like to see here (in beta?)

This can be verified as working by two things.
One

  • open a new Firefox with a new profile.
  • go to about:protections
  • click the "sign up for breach alerts" button and subscribe
  • log in to the browser with your firefox account
  • go back to about:protections, refresh the page, the monitor card should now have your breach alert stats. (there was no caching)

Two

  • go back to the monitor page (not through clicking the "View full report on Firefox Monitor" link)
  • unsubscribe from breach alerts
  • go back to about:protections and refresh the page
  • you should still see the monitor data for another 24 hours due to caching.
  • clicking on the "View full report on Firefox Monitor" link will invalidate the cache.

As for performance, we are unsure of what sort of load to expect on the Monitor API so we are taking this precaution.

(is that the sort of information you mean by "verification"?)

Flags: needinfo?(ewright)
Regressions: 1582755
You need to log in before you can comment on or make changes to this bug.