Skip to content

Add cumulative registrations report (LG-8679)#7674

Merged
zachmargolis merged 2 commits intomainfrom
margolis-daily-registrations-report
Jan 23, 2023
Merged

Add cumulative registrations report (LG-8679)#7674
zachmargolis merged 2 commits intomainfrom
margolis-daily-registrations-report

Conversation

@zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Jan 20, 2023

🎫 Ticket

LG-8679

🛠 Summary of changes

Adds a new daily reporting job that loads all-time user registrations (both plain User.count and fully registered users)

In "do not merge" while I still confirm we're OK to launch it Update: It's ready to go! 🚀

changelog: Internal, Reporting, Add new cumulative registrations reporting
Comment on lines +53 to +62
sql = format(<<-SQL, params)
SELECT
COUNT(*) AS total_users
, created_at::date AS date
FROM users
WHERE
created_at <= %{finish}
GROUP BY
created_at::date
SQL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a table scan, ~100,000ms in prod. We could likely speed it up if we added an index on users.created_at, unsure how others feel about the cost/benefit of that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't feel worthwhile if it's only for a report

Comment on lines +74 to +83
sql = format(<<-SQL, params)
SELECT
COUNT(*) AS fully_registered_users
, registered_at::date AS date
FROM registration_logs
WHERE
registered_at <= %{finish}
GROUP BY
registered_at::date
SQL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is indexed already, relatively fast query

Comment on lines +42 to +45
{
finish: finish,
results: results.as_json,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one note: other reports collect a single day at a time (to keep calculations relatively cheap), and the frontend will make one request per day it's querying.

However, for this report, we always want to know all of time. So rather than have clients make requests going back each day to 2017 (1000+ requests), I opted to save client time so the clients only need to query one "day" to get all time.

I think it's a worthwhile tradeoff, but open to feedback!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to write over the same file in S3 then? It could get quite large.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this will make a new file every day, just like the current reports. A report from last week is about 152kb, so not that big IMO

@zachmargolis
Copy link
Contributor Author

Got the OK to ship this! It's ready for review!

@zachmargolis zachmargolis merged commit ebff677 into main Jan 23, 2023
@zachmargolis zachmargolis deleted the margolis-daily-registrations-report branch January 23, 2023 21:35
@mdiarra3 mdiarra3 mentioned this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants