You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR tweaks _get_daily_stats to improve performance. Instead of a round trip to the DB to fetch the daily stats, we first attempt to fetch them from Redis using the RedisAnnualLimit client with the DB as a fallback.
Perf timings before (measured on staging directly):
The reason will be displayed to describe this comment to others. Learn more.
The performance is way better for that service (4s load time for the large service vs 6s before), however I am seeing different number for the daily/annual limits.
The performance is way better for that service (4s load time for the large service vs 6s before), however I am seeing different number for the daily/annual limits.
Oh yea they are a little bit off... I tried deleting and re-seeding the data in Redis but it seems the counting may be off. I'll dig into it and see if I can find where the seeding differs from what the DAO fetches.
The performance is way better for that service (4s load time for the large service vs 6s before), however I am seeing different number for the daily/annual limits.
It seems like the difference we're seeing is because of notifications with a non-terminal statuses i.e. inflights - referred to as requested in code.
The annual_limit keys in Redis only count delivered and failed and don't count statuses like pending, pending-virus-check, created etc. Which is likely the source of, or at least contributing to, the discrepancy here.
Some thoughts on options moving forward:
Collect the stats from Redis initially to improve page load performance, then on a subsequent polls only use stats from the DB like we're doing now.
I have a version of this working currently, but I'm not sure it is a good experience for users. If you open your dashboard and scroll down immediately to the daily/annual sections, you'll notice numbers "correcting" which would be confusing if you're not currently sending.
Create a new API endpoint that collects only inflight / requested notifications. Call Redis for the bulk of the notification counts, then combine with the counts from the "inflights" endpoint.
Likely to be faster than the current DB call.
Ignore daily and annual stat loading on page load initially and only fetch them when some form of activity occurs on the page (e.g. user begins scrolling down) or the first dashboard poll occurs - whichever comes first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary | Résumé
This PR tweaks
_get_daily_statsto improve performance. Instead of a round trip to the DB to fetch the daily stats, we first attempt to fetch them from Redis using theRedisAnnualLimitclient with the DB as a fallback.Perf timings before (measured on staging directly):
{ "template_statistics_weekly": 1400.3448486328125, "aggregate_template_usage": 5.429983139038086, "get_scheduled_jobs": 45.98712921142578, "get_immediate_jobs": 32.1347713470459, "add_rate_to_job": 0.000476837158203125, "_get_daily_stats": 1246.3033199310303, <--- "weekly_stats_aggregation": 3.3087730407714844, "get_bounce_rate_data": 13.918161392211914, "get_annual_data": 3.88336181640625 }Perf timings after (measured with this branch hooked up to staging):
{ "template_statistics_weekly": 3350.5444526672363, "aggregate_template_usage": 4.066944122314453, "get_scheduled_jobs": 147.20726013183594, "get_immediate_jobs": 182.31868743896484, "add_rate_to_job": 0.000476837158203125, "_get_daily_stats": 62.27922439575195, <-- "weekly_stats_aggregation": 2.112865447998047, "get_bounce_rate_data": 346.47130966186523, "get_annual_data": 93.42670440673828 }Test instructions | Instructions pour tester la modification
Basic tests
/services/8dad0c16-6952-4424-be2a-d98b8bfc3c2din bothPerf via logs
/services/8dad0c16-6952-4424-be2a-d98b8bfc3c2d