- Notifications
You must be signed in to change notification settings - Fork1k
chore: replace GetManagedAgentCount query with aggregate table#19636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
- Removes GetManagedAgentCount query- Adds new table `usage_events_daily` which stores aggregated usage events by the type and UTC day- Adds trigger to update the values in this table when a new row is inserted into `usage_events`- Adds a migration that adds `usage_events_daily` rows for existing data in `usage_events`Since the `usage_events` table is unreleased currently, this migrationwill do nothing on real deployments and will only affect previewdeployments such as dogfood.
| RETURNS TRIGGER AS $$ | ||
| BEGIN | ||
| -- Check for supported event types and throw error for unknown types | ||
| IF NEW.event_type NOT IN ('dc_managed_agents_v1') THEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My main worry with this is that if some bad code spams insertion of unknown usage events we could end up creating some serious DB load. IIRC you make it fairly difficult to even do that though, so I guess this is OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I imagine it would generate a lot of load anyway if something like that was happening. This upsert should be fairly quick since it's using the primary key.
I also don't know how else I'd handle this other than a cronjob, which would also generate a lot of load if there was a lot of rows, and would require a new Go package to handle doing it every once in a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reminder to check migration number before merge
39bf3ba intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
usage_events_dailywhich stores aggregated usage events by the type and UTC dayusage_eventsusage_events_dailyrows for existing data inusage_eventsSince the
usage_eventstable is unreleased currently, this migration will do nothing on real deployments and will only affect preview deployments such as dogfood.Closescoder/internal#943