-
Notifications
You must be signed in to change notification settings - Fork 110
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve universe stats #396
Conversation
47cac50
to
cb9b7e2
Compare
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.
Looks good! Cool to have a migration in + DB-specific logic support.
cb9b7e2
to
cdee934
Compare
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.
There's a lot of work here, good job Oli on getting it together!
I would question whether we need to aggregate the events data at all. Or at least whether it makes sense to aggregate by day. That seems like the sort of thing we could just ask a client to do for the time being.
If we are to aggregate, I wonder if we could just keep the aggregation window as an argument.
I see that the terminal team requested this level of aggregation (5 here: #367)
In a next commit we'll want to be able to better test asset events which are always inserted into the DB with a timestamp of `time.Now()`. In order to be able to influence what time is actually written to the DB, we use the `clock.Now()` value that can be overwritten in unit tests.
cdee934
to
e39d07b
Compare
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.
🥳
Depends on #389.Fixes #367.
This PR addresses items 2, 3, 4 and 5 of #367.
@jamaljsr can you please check if this is what you need?