Skip to content

fix(coderd/database): aggregate user engagement statistics by interval #16150

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

Merged
merged 5 commits into from
Jan 16, 2025

Conversation

SasSwart
Copy link
Contributor

@SasSwart SasSwart commented Jan 15, 2025

This PR addresses the TODO comment here:
https://github.com/coder/coder/pull/16134/files#diff-1844f895bb005f036da11d800fe2a76b54bfddd481c5d8cb15f210c64679caa5R47

The backend now backfills entries for dates with no status changes.

@SasSwart SasSwart requested a review from defelmnq January 15, 2025 15:00
@BrunoQuaresma
Copy link
Collaborator

I'm sorry, but I'm not entirely sure what this PR is intended to address. Since it's labeled as a fix, I would expect the PR description to include a related issue or an explanation of what the problem is and what the expected behavior should be.

@SasSwart
Copy link
Contributor Author

@BrunoQuaresma fair point! This addresses the TODO comment here:
https://github.com/coder/coder/pull/16134/files#diff-1844f895bb005f036da11d800fe2a76b54bfddd481c5d8cb15f210c64679caa5R47

The backend now backfills dates with no changes. A previous PR also added sorting as mentioned in the same TODO.

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Thanks for updating the issue 🙏. The code looks good! I noticed there are no tests for the backfilled dates, but I'll leave that decision up to you.

@SasSwart
Copy link
Contributor Author

SasSwart commented Jan 16, 2025

Thanks @BrunoQuaresma.
All of the tests in coderd/database/querier_test.go TestGetUserStatusCounts test backfilling by asserting this:

for i, row := range userStatusChanges {
	require.True(
		t,
		row.Date.In(location).Equal(dbtime.StartOfDay(createdAt).AddDate(0, 0, i/2)),
		"expected date %s, but got %s for row %n",
		dbtime.StartOfDay(createdAt).AddDate(0, 0, i/2),
		row.Date.In(location).String(),
		i,
	)

We assert that we have an entry for every single date in the requested range.

@SasSwart SasSwart merged commit 50bf5ca into main Jan 16, 2025
37 checks passed
@SasSwart SasSwart deleted the jjs/insights branch January 16, 2025 15:34
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants