[Protection Report] Cache monitor card data
Categories
(Firefox :: Protections UI, enhancement, P1)
Tracking
()
People
(Reporter: ewright, Assigned: ewright)
References
(Regressed 1 open bug)
Details
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
|
Details | Review |
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
Comment 1•5 years ago
|
||
(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.
Assignee | ||
Comment 2•5 years ago
|
||
(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.
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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.
Comment 6•5 years ago
|
||
Thanks for checking that. It's helpful to estimate the lower-bound of sustained pings we might expect after the initial spike.
Assignee | ||
Comment 7•5 years ago
|
||
Comment 8•5 years ago
|
||
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...
Assignee | ||
Comment 9•5 years ago
|
||
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
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
bugherder |
Assignee | ||
Comment 12•5 years ago
|
||
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
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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?)
Assignee | ||
Comment 14•5 years ago
•
|
||
(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"?)
Comment 15•5 years ago
|
||
bugherder uplift |
Description
•