-
Notifications
You must be signed in to change notification settings - Fork 875
feat: add total users insight #15486
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
Conversation
coderd/database/queries/insights.sql
Outdated
users | ||
WHERE | ||
created_at >= @start_time::timestamptz | ||
AND created_at < @end_time::timestamptz |
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.
More for confirmation than a request to change - is it expected to exclude the end_time from the query ?
If I say I want the insights from 01/01to 02/01- should we include 02/01 ?
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.
In-memory DB doesn't match this:
if user.CreatedAt.Before(arg.StartTime) || user.CreatedAt.After(arg.EndTime) {
Missing || user.CreatedAt.Equal(arg.EndTime)
.
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 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.
is it expected to exclude the end_time from the query ?
No. Not quite, at least. The intention is to include it. We'll generate a series of dates starting from the start date with 24 hour increments. This means that subject to some offset (<24h), the date of the endtime param will be included.
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.
CI is failing at the moment, you need to take a look at the report and address issues.
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.
Nice work! Had a few concern regarding timezones but otherwise this looks solid.
coderd/database/dbmem/dbmem.go
Outdated
if user.CreatedAt.Before(arg.StartTime) || user.CreatedAt.After(arg.EndTime) { | ||
continue | ||
} | ||
date := user.CreatedAt.Truncate(24 * time.Hour) |
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.
Will this work correctly with timezones?
coderd/database/queries/insights.sql
Outdated
users | ||
WHERE | ||
created_at >= @start_time::timestamptz | ||
AND created_at < @end_time::timestamptz |
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.
In-memory DB doesn't match this:
if user.CreatedAt.Before(arg.StartTime) || user.CreatedAt.After(arg.EndTime) {
Missing || user.CreatedAt.Equal(arg.EndTime)
.
coderd/database/queries/insights.sql
Outdated
-- in the given timeframe. It returns the accumulated number of users for each date | ||
-- within the specified timeframe, providing a running total of user sign-ups. | ||
SELECT | ||
date_trunc('day', created_at)::date AS date, |
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.
This might produce strange results if requested start time and end time are in a different timezone. Each day may be missing few/many entries and we might get ghost dates as well (+/-1 day outside start/end range).
Suggestion:
date_trunc('day', (
created_at AT TIME ZONE EXTRACT(TIMEZONE FROM @start_time::timestamptz)
)) AT TIME ZONE EXTRACT(TIMEZONE FROM @start_time::timestamptz) AS date,
(Untested), why the double at timezone? One strips timezone and the other one adds it back.
coder=# select created_at from users;
created_at
-------------------------------
2024-04-08 11:41:35.096834+00
2024-04-08 11:40:54.587231+00
(2 rows)
coder=# select created_at at time zone 'europe/helsinki' from users;
timezone
----------------------------
2024-04-08 14:41:35.096834
2024-04-08 14:40:54.587231
(2 rows)
coder=# select created_at at time zone 'europe/helsinki' at time zone 'europe/helsinki' from users;
timezone
-------------------------------
2024-04-08 11:41:35.096834+00
2024-04-08 11:40:54.587231+00
(2 rows)
Notice +00 and lack thereof. Now notice how we get a timezoneally correct truncation when we do it without timezone:
coder=# select date_trunc('day', created_at) from users;
date_trunc
------------------------
2024-04-08 00:00:00+00
2024-04-08 00:00:00+00
(2 rows)
coder=# select date_trunc('day', created_at at time zone 'europe/helsinki') at time zone 'europe/helsinki' from users;
timezone
------------------------
2024-04-07 21:00:00+00
2024-04-07 21:00:00+00
(2 rows)
You might want to use a subquery so you can count over the improved truncation without repeating all that noise.
coderd/insights_test.go
Outdated
return time.Now().UTC().AddDate(0, 0, -days).Truncate(24 * time.Hour) | ||
} | ||
|
||
func today() time.Time { |
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.
This function name is a bit confusing. I know it makes sense for the test but might be better to define it inside the parent test function instead of entire coderd_test
package.
coderd/insights_test.go
Outdated
// When: requesting the accumulated total of users by day | ||
res, err := client.TotalUsersInsight(ctx, codersdk.TotalUsersInsightRequest{ | ||
StartTime: daysAgo(2), | ||
EndTime: today(), |
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.
It would be good to add tests with other timezones too. Might need to consider the value of user created at more closely to trigger edge cases.
coderd/insights.go
Outdated
|
||
var res codersdk.TotalUsersInsightResponse | ||
currentTotal := uint64(0) | ||
for d := startTime; d.Before(endTime) || d.Equal(endTime); d = d.AddDate(0, 0, 1) { |
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.
Tip: One alternative to this method is to generate the empty dates as well via generate_series
in the query. But don't feel you need to make the change now, just suggesting it as an option for the future.
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 will take a look at that.
@mafredri About the timezone concern. I don't see us doing extra logic to handle timezones in other insights queries, why for this one should we care? |
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.
@mafredri not sure why this golden file was not on main since I didn't change it 🤔
…istered-users-endpoint
coderd/insights.go
Outdated
return | ||
} | ||
|
||
zone, _ := startTime.Zone() |
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.
You might find this in your tests too but Zone it not guaranteed to return a name. It's unfortunately safer to use the offset (integer) and turn it into a string like 2 => "2"
and `-2 => "-2".
That's a bit unfortunate however since there may be a minor edge case during DST shift and could potentially produce an unwanted extra/missing row depending on the data. I haven't tried to verify if/how much of a problem it would be though.
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.
Changed to use offset!
coderd/insights.go
Outdated
rows, err := api.Database.GetAccumulatedUsersInsights(ctx, database.GetAccumulatedUsersInsightsParams{ | ||
StartTime: startTime, | ||
EndTime: endTime, | ||
Timezone: string(offset), |
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.
This doesn't behave like intended. You need to use the strconv package instead.
…istered-users-endpoint
@mafredri I need your help here. For the When the tests run in memory, they return five as expected but when running with a PG database they return four. I think this is related to how dates are casting in the query but I have no idea how to fix it and I would love a second pair of eyes on it. |
Note: I'm removing |
Looking into how entitlements count license utilization showed that instead of |
@@ -517,6 +517,8 @@ func createWorkspace( | |||
// Update audit log's organization | |||
auditReq.UpdateOrganizationID(template.OrganizationID) | |||
|
|||
// TODO (SasSwart): api.Authorize below is already done above. This seems redundant. | |||
// Should we remove this? |
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.
Yep, that does look redundant. Let's remove it in a separate PR.
closing in favor of #15683 |
Related to #15297