Skip to content

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

Closed
wants to merge 23 commits into from

Conversation

BrunoQuaresma
Copy link
Collaborator

Related to #15297

@BrunoQuaresma BrunoQuaresma self-assigned this Nov 12, 2024
@BrunoQuaresma BrunoQuaresma changed the title feat: add registered users insights feat: add total users insight Nov 12, 2024
users
WHERE
created_at >= @start_time::timestamptz
AND created_at < @end_time::timestamptz
Copy link
Contributor

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 ?

Copy link
Member

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@defelmnq I will double-check that.

@mafredri I didn't get your last comment. Sorry 😞

Copy link
Contributor

@SasSwart SasSwart Nov 26, 2024

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.

Copy link
Member

@mtojek mtojek left a 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.

Copy link
Member

@mafredri mafredri left a 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.

if user.CreatedAt.Before(arg.StartTime) || user.CreatedAt.After(arg.EndTime) {
continue
}
date := user.CreatedAt.Truncate(24 * time.Hour)
Copy link
Member

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?

users
WHERE
created_at >= @start_time::timestamptz
AND created_at < @end_time::timestamptz
Copy link
Member

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).

-- 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,
Copy link
Member

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.

return time.Now().UTC().AddDate(0, 0, -days).Truncate(24 * time.Hour)
}

func today() time.Time {
Copy link
Member

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.

// When: requesting the accumulated total of users by day
res, err := client.TotalUsersInsight(ctx, codersdk.TotalUsersInsightRequest{
StartTime: daysAgo(2),
EndTime: today(),
Copy link
Member

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.


var res codersdk.TotalUsersInsightResponse
currentTotal := uint64(0)
for d := startTime; d.Before(endTime) || d.Equal(endTime); d = d.AddDate(0, 0, 1) {
Copy link
Member

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.

Copy link
Collaborator Author

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.

@BrunoQuaresma
Copy link
Collaborator Author

@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?

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Nov 13, 2024

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 🤔

return
}

zone, _ := startTime.Zone()
Copy link
Member

@mafredri mafredri Nov 14, 2024

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to use offset!

rows, err := api.Database.GetAccumulatedUsersInsights(ctx, database.GetAccumulatedUsersInsightsParams{
StartTime: startTime,
EndTime: endTime,
Timezone: string(offset),
Copy link
Member

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.

@github-actions github-actions bot added the stale This issue is like stale bread. label Nov 22, 2024
@BrunoQuaresma
Copy link
Collaborator Author

BrunoQuaresma commented Nov 22, 2024

@mafredri I need your help here.

For the Success test, I'm creating four users + the first user created to be used with the client so the total of users should be five.

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.

@github-actions github-actions bot removed the stale This issue is like stale bread. label Nov 23, 2024
@SasSwart SasSwart self-assigned this Nov 25, 2024
@SasSwart
Copy link
Contributor

Note:
#15297 (comment)
#15297 (comment)

I'm removing GetAccumulatedUsersInsights from this PR and reducing its scope to the frontend changes to existing graphs. The new gauge required in #15297 will follow in a subsequent PR.

@SasSwart
Copy link
Contributor

Looking into how entitlements count license utilization showed that instead of GetAccumulatedUsersInsights we'll need to use GetActiveUserCount for the gauge. This is good news, makes the PR smaller and means the number won't drift.

@@ -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?
Copy link
Member

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.

@SasSwart
Copy link
Contributor

closing in favor of #15683

@SasSwart SasSwart closed this Nov 29, 2024
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.

6 participants