Adjust start dates when fetching notification stats #2061
Conversation
- No longer subracting 1 day from the retention period limit when calculating the start_date - Changed the stats_for_today query to start pulling notifications starting from midnight UTC of the current day instead of midnight today - retention period days
| @@ -260,7 +260,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ | |||
| *([func.count().label("count")]), | |||
| ) | |||
| .filter( | |||
| Notification.created_at >= start_date, | |||
| Notification.created_at >= get_midnight(datetime.utcnow()), | |||
There was a problem hiding this comment.
This line was the core of the issue that inflated the notification counts. We weren't pulling just today's notifications in this query, but all notifications from starting from today - days of retention to today, doubling notification counts for those already entered into ft_notification_status.
This line was also why it seemed reasonable to start the investigation with jobs as it wasn't a 1:1 duplication but a duplication of all notifications - today's notifications making it seem like one off's weren't affected.
In retrospect, investigating significantly higher volume services could have helped steer us away from that sooner as it's less likely a service would be sending large volumes of one offs. Certainly possible, but less likely to make sense to do so.
| @@ -239,7 +239,7 @@ def fetch_notification_status_for_service_for_day(bst_day, service_id): | |||
|
|
|||
|
|
|||
| def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, by_template=False, limit_days=7): | |||
| start_date = utc_midnight_n_days_ago(limit_days - 1) | |||
| start_date = utc_midnight_n_days_ago(limit_days) | |||
There was a problem hiding this comment.
I was hesitant to remove the - 1 here as we added it in this PR for an edge case.
Given this date is only compared to the bst_date in ft_notification_counts, which has no timezone data, we may not need the call to utc_midnight_n_days_ago anymore.
| @@ -239,7 +239,7 @@ def fetch_notification_status_for_service_for_day(bst_day, service_id): | |||
|
|
|||
|
|
|||
| def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, by_template=False, limit_days=7): | |||
| start_date = utc_midnight_n_days_ago(limit_days - 1) | |||
| start_date = utc_midnight_n_days_ago(limit_days) | |||
|
|
|||
| stats_for_7_days = db.session.query( | |||
There was a problem hiding this comment.
An idea that crossed my mind to make this code a bit more testable is pulling the stats_for_7_days and stats_for_today out into their own methods. We could then reuse them in other contexts in the future and get a bit more granular and focused with our testing, considering this area of the code continues to be a pain-point.
Had this been the case from the beginning we would have caught on to stats_for_today pulling in more than the current day's notifications.
There was a problem hiding this comment.
I like that idea, if you want to do it in this PR feel free
| @@ -239,7 +239,7 @@ def fetch_notification_status_for_service_for_day(bst_day, service_id): | |||
|
|
|||
|
|
|||
| def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, by_template=False, limit_days=7): | |||
| start_date = utc_midnight_n_days_ago(limit_days - 1) | |||
| start_date = utc_midnight_n_days_ago(limit_days) | |||
|
|
|||
| stats_for_7_days = db.session.query( | |||
There was a problem hiding this comment.
I like that idea, if you want to do it in this PR feel free
Summary | Résumé
midnight today - retention period dayswhich was resulting in duplication of all notifications within the retention period minus the current day's notification count.Testing
Example affected services
-OR-