diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 17aef94f73..3ba9d8c92c 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -1803,12 +1803,17 @@ const docTemplate = `{ "summary": "Get insights about user status counts", "operationId": "get-insights-about-user-status-counts", "parameters": [ + { + "type": "string", + "description": "IANA timezone name (e.g. America/St_Johns)", + "name": "timezone", + "in": "query" + }, { "type": "integer", - "description": "Time-zone offset (e.g. -2)", + "description": "Deprecated: Time-zone offset (e.g. -2). Use timezone instead.", "name": "tz_offset", - "in": "query", - "required": true + "in": "query" } ], "responses": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 84239407a3..a55fbcfc49 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -1575,12 +1575,17 @@ "summary": "Get insights about user status counts", "operationId": "get-insights-about-user-status-counts", "parameters": [ + { + "type": "string", + "description": "IANA timezone name (e.g. America/St_Johns)", + "name": "timezone", + "in": "query" + }, { "type": "integer", - "description": "Time-zone offset (e.g. -2)", + "description": "Deprecated: Time-zone offset (e.g. -2). Use timezone instead.", "name": "tz_offset", - "in": "query", - "required": true + "in": "query" } ], "responses": { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 2f038b97fe..252f2075f4 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2076,7 +2076,7 @@ func (s *MethodTestSuite) TestUser() { ) })) s.Run("GetUserStatusCounts", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { - arg := database.GetUserStatusCountsParams{StartTime: time.Now().Add(-time.Hour * 24 * 30), EndTime: time.Now(), Interval: int32((time.Hour * 24).Seconds())} + arg := database.GetUserStatusCountsParams{StartTime: time.Now().Add(-time.Hour * 24 * 30), EndTime: time.Now(), Tz: "America/St_Johns"} dbm.EXPECT().GetUserStatusCounts(gomock.Any(), arg).Return([]database.GetUserStatusCountsRow{}, nil).AnyTimes() check.Args(arg).Asserts(rbac.ResourceUser, policy.ActionRead) })) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 1512608212..467c97801e 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -500,16 +500,6 @@ type sqlcQuerier interface { GetUserSecretByUserIDAndName(ctx context.Context, arg GetUserSecretByUserIDAndNameParams) (UserSecret, error) // GetUserStatusCounts returns the count of users in each status over time. // The time range is inclusively defined by the start_time and end_time parameters. - // - // Bucketing: - // Between the start_time and end_time, we include each timestamp where a user's status changed or they were deleted. - // We do not bucket these results by day or some other time unit. This is because such bucketing would hide potentially - // important patterns. If a user was active for 23 hours and 59 minutes, and then suspended, a daily bucket would hide this. - // A daily bucket would also have required us to carefully manage the timezone of the bucket based on the timezone of the user. - // - // Accumulation: - // We do not start counting from 0 at the start_time. We check the last status change before the start_time for each user. As such, - // the result shows the total number of users in each status on any particular day. GetUserStatusCounts(ctx context.Context, arg GetUserStatusCountsParams) ([]GetUserStatusCountsRow, error) GetUserTaskNotificationAlertDismissed(ctx context.Context, userID uuid.UUID) (bool, error) GetUserTerminalFont(ctx context.Context, userID uuid.UUID) (string, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 19c3c5673b..0915f1145a 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -4291,8 +4291,18 @@ func TestGroupRemovalTrigger(t *testing.T) { func TestGetUserStatusCounts(t *testing.T) { t.Parallel() - t.Skip("https://github.com/coder/internal/issues/464") + type testCase struct { + timezone string + location *time.Location + reportFrom time.Time + reportUntil time.Time + } + testCases := []testCase{} + + // GetUserStatusCounts is sensitive to DST transitions, because it generates timestamps exactly + // one day apart from one another, and specific days can have varying lengths depending on the timezone. + // Therefore, we test with a variety of timezones. timezones := []string{ "America/St_Johns", "Africa/Johannesburg", @@ -4302,18 +4312,39 @@ func TestGetUserStatusCounts(t *testing.T) { "Australia/Sydney", } + // assemble test cases for _, tz := range timezones { - t.Run(tz, func(t *testing.T) { + location, err := time.LoadLocation(tz) + if err != nil { + t.Fatalf("failed to load location: %v", err) + } + + // Testing based on the current system date will flake due to DST transitions. + // Instead, we test with a fixed range of dates that is large enough to span multiple DST transitions. + startOfTestDateRange := time.Date(2025, 1, 1, 0, 0, 0, 0, location) + endOfTestDateRange := time.Date(2026, 1, 1, 0, 0, 0, 0, location) + // To keep the number of test cases manageable given the large date range, + // we test with a suitable large interval. This interval is also the length of each report. + // this ensures we have full coverage of the date range. + testDateRangeInterval := 60 + + for reportFrom := startOfTestDateRange; !reportFrom.After(endOfTestDateRange); reportFrom = reportFrom.AddDate(0, 0, testDateRangeInterval) { + testCases = append(testCases, testCase{ + timezone: tz, + location: location, + reportFrom: dbtime.Time(reportFrom), + reportUntil: dbtime.Time(reportFrom.AddDate(0, 0, testDateRangeInterval)), + }) + } + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s/%s", tc.timezone, tc.reportUntil.Format("2006-01-02T15:04:05Z")), func(t *testing.T) { t.Parallel() - location, err := time.LoadLocation(tz) - if err != nil { - t.Fatalf("failed to load location: %v", err) - } - today := dbtime.Now().In(location) - createdAt := today.Add(-5 * 24 * time.Hour) - firstTransitionTime := createdAt.Add(2 * 24 * time.Hour) - secondTransitionTime := firstTransitionTime.Add(2 * 24 * time.Hour) + userCreatedAt := tc.reportUntil.AddDate(0, 0, -60) + firstStatusChange := userCreatedAt.AddDate(0, 0, 29) + secondStatusChange := firstStatusChange.AddDate(0, 0, 29) t.Run("No Users", func(t *testing.T) { t.Parallel() @@ -4321,8 +4352,9 @@ func TestGetUserStatusCounts(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) counts, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: createdAt, - EndTime: today, + Tz: tc.timezone, + StartTime: tc.reportFrom, + EndTime: tc.reportUntil, }) require.NoError(t, err) require.Empty(t, counts, "should return no results when there are no users") @@ -4331,7 +4363,7 @@ func TestGetUserStatusCounts(t *testing.T) { t.Run("One User/Creation Only", func(t *testing.T) { t.Parallel() - testCases := []struct { + subTestCases := []struct { name string status database.UserStatus }{ @@ -4349,42 +4381,56 @@ func TestGetUserStatusCounts(t *testing.T) { }, } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, stc := range subTestCases { + t.Run(stc.name, func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) ctx := testutil.Context(t, testutil.WaitShort) - // Create a user that's been in the specified status for the past 30 days dbgen.User(t, db, database.User{ - Status: tc.status, - CreatedAt: createdAt, - UpdatedAt: createdAt, + Status: stc.status, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) + startTime := dbtime.StartOfDay(userCreatedAt) + endTime := dbtime.StartOfDay(tc.reportUntil) userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: dbtime.StartOfDay(createdAt), - EndTime: dbtime.StartOfDay(today), + Tz: tc.timezone, + StartTime: startTime, + EndTime: endTime, }) require.NoError(t, err) - numDays := int(dbtime.StartOfDay(today).Sub(dbtime.StartOfDay(createdAt)).Hours() / 24) - require.Len(t, userStatusChanges, numDays+1, "should have 1 entry per day between the start and end time, including the end time") + numDays := 0 + for d := startTime; !d.After(endTime); d = d.AddDate(0, 0, 1) { + numDays++ + } + assert.Len( + t, + userStatusChanges, + numDays, + "should have 1 entry per day between the start and end time, including the end time", + ) for i, row := range userStatusChanges { - require.Equal(t, tc.status, row.Status, "should have the correct status") - require.True( + require.Equal(t, stc.status, row.Status, "should have the correct status") + + rowDate := row.Date.In(tc.location) + expectedDate := dbtime.StartOfDay(userCreatedAt).AddDate(0, 0, i) + assert.True( t, - row.Date.In(location).Equal(dbtime.StartOfDay(createdAt).AddDate(0, 0, i)), + rowDate.Equal(expectedDate), "expected date %s, but got %s for row %n", - dbtime.StartOfDay(createdAt).AddDate(0, 0, i), - row.Date.In(location).String(), + expectedDate.String(), + rowDate.String(), i, ) - if row.Date.Before(createdAt) { - require.Equal(t, int64(0), row.Count, "should have 0 users before creation") + + if row.Date.Before(userCreatedAt) { + assert.Equal(t, int64(0), row.Count, "should have 0 users before creation") } else { - require.Equal(t, int64(1), row.Count, "should have 1 user after creation") + assert.Equal(t, int64(1), row.Count, "should have 1 user after creation") } } }) @@ -4394,7 +4440,7 @@ func TestGetUserStatusCounts(t *testing.T) { t.Run("One User/One Transition", func(t *testing.T) { t.Parallel() - testCases := []struct { + subTestCases := []struct { name string initialStatus database.UserStatus targetStatus database.UserStatus @@ -4405,15 +4451,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusActive, targetStatus: database.UserStatusDormant, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusActive: 1, database.UserStatusDormant: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusDormant: 1, database.UserStatusActive: 0, }, - today: { + tc.reportUntil: { database.UserStatusDormant: 1, database.UserStatusActive: 0, }, @@ -4424,15 +4470,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusActive, targetStatus: database.UserStatusSuspended, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusActive: 1, database.UserStatusSuspended: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusSuspended: 1, database.UserStatusActive: 0, }, - today: { + tc.reportUntil: { database.UserStatusSuspended: 1, database.UserStatusActive: 0, }, @@ -4443,15 +4489,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusDormant, targetStatus: database.UserStatusActive, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusDormant: 1, database.UserStatusActive: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusActive: 1, database.UserStatusDormant: 0, }, - today: { + tc.reportUntil: { database.UserStatusActive: 1, database.UserStatusDormant: 0, }, @@ -4462,15 +4508,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusDormant, targetStatus: database.UserStatusSuspended, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusDormant: 1, database.UserStatusSuspended: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusSuspended: 1, database.UserStatusDormant: 0, }, - today: { + tc.reportUntil: { database.UserStatusSuspended: 1, database.UserStatusDormant: 0, }, @@ -4481,15 +4527,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusSuspended, targetStatus: database.UserStatusActive, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusSuspended: 1, database.UserStatusActive: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusActive: 1, database.UserStatusSuspended: 0, }, - today: { + tc.reportUntil: { database.UserStatusActive: 1, database.UserStatusSuspended: 0, }, @@ -4500,15 +4546,15 @@ func TestGetUserStatusCounts(t *testing.T) { initialStatus: database.UserStatusSuspended, targetStatus: database.UserStatusDormant, expectedCounts: map[time.Time]map[database.UserStatus]int64{ - createdAt: { + userCreatedAt: { database.UserStatusSuspended: 1, database.UserStatusDormant: 0, }, - firstTransitionTime: { + firstStatusChange: { database.UserStatusDormant: 1, database.UserStatusSuspended: 0, }, - today: { + tc.reportUntil: { database.UserStatusDormant: 1, database.UserStatusSuspended: 0, }, @@ -4516,60 +4562,60 @@ func TestGetUserStatusCounts(t *testing.T) { }, } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, stc := range subTestCases { + t.Run(stc.name, func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) ctx := testutil.Context(t, testutil.WaitShort) - // Create a user that starts with initial status user := dbgen.User(t, db, database.User{ - Status: tc.initialStatus, - CreatedAt: createdAt, - UpdatedAt: createdAt, + Status: stc.initialStatus, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) - // After 2 days, change status to target status user, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user.ID, - Status: tc.targetStatus, - UpdatedAt: firstTransitionTime, + Status: stc.targetStatus, + UpdatedAt: firstStatusChange, }) require.NoError(t, err) - // Query for the last 5 days userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: dbtime.StartOfDay(createdAt), - EndTime: dbtime.StartOfDay(today), + Tz: tc.timezone, + StartTime: dbtime.StartOfDay(userCreatedAt), + EndTime: dbtime.StartOfDay(tc.reportUntil), }) require.NoError(t, err) for i, row := range userStatusChanges { + rowDate := row.Date.In(tc.location) + expectedDate := dbtime.StartOfDay(userCreatedAt).AddDate(0, 0, i/2) require.True( t, - row.Date.In(location).Equal(dbtime.StartOfDay(createdAt).AddDate(0, 0, i/2)), + rowDate.Equal(expectedDate), "expected date %s, but got %s for row %n", - dbtime.StartOfDay(createdAt).AddDate(0, 0, i/2), - row.Date.In(location).String(), + expectedDate.String(), + rowDate.String(), i, ) switch { - case row.Date.Before(createdAt): + case row.Date.Before(userCreatedAt): require.Equal(t, int64(0), row.Count) - case row.Date.Before(firstTransitionTime): - if row.Status == tc.initialStatus { + case row.Date.Before(firstStatusChange): + if row.Status == stc.initialStatus { require.Equal(t, int64(1), row.Count) - } else if row.Status == tc.targetStatus { + } else if row.Status == stc.targetStatus { require.Equal(t, int64(0), row.Count) } - case !row.Date.After(today): - if row.Status == tc.initialStatus { + case !row.Date.After(tc.reportUntil): + if row.Status == stc.initialStatus { require.Equal(t, int64(0), row.Count) - } else if row.Status == tc.targetStatus { + } else if row.Status == stc.targetStatus { require.Equal(t, int64(1), row.Count) } default: - t.Errorf("date %q beyond expected range end %q", row.Date, today) + t.Errorf("date %q beyond expected range end %q", row.Date, tc.reportUntil) } } }) @@ -4590,7 +4636,7 @@ func TestGetUserStatusCounts(t *testing.T) { user2Transition transition } - testCases := []testCase{ + subTestCases := []testCase{ { name: "Active->Dormant and Dormant->Suspended", user1Transition: transition{ @@ -4648,49 +4694,48 @@ func TestGetUserStatusCounts(t *testing.T) { }, } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, stc := range subTestCases { + t.Run(stc.name, func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) ctx := testutil.Context(t, testutil.WaitShort) user1 := dbgen.User(t, db, database.User{ - Status: tc.user1Transition.from, - CreatedAt: createdAt, - UpdatedAt: createdAt, + Status: stc.user1Transition.from, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) user2 := dbgen.User(t, db, database.User{ - Status: tc.user2Transition.from, - CreatedAt: createdAt, - UpdatedAt: createdAt, + Status: stc.user2Transition.from, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) - // First transition at 2 days user1, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user1.ID, - Status: tc.user1Transition.to, - UpdatedAt: firstTransitionTime, + Status: stc.user1Transition.to, + UpdatedAt: firstStatusChange, }) require.NoError(t, err) - // Second transition at 4 days user2, err = db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user2.ID, - Status: tc.user2Transition.to, - UpdatedAt: secondTransitionTime, + Status: stc.user2Transition.to, + UpdatedAt: secondStatusChange, }) require.NoError(t, err) userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: dbtime.StartOfDay(createdAt), - EndTime: dbtime.StartOfDay(today), + Tz: tc.timezone, + StartTime: dbtime.StartOfDay(userCreatedAt), + EndTime: dbtime.StartOfDay(tc.reportUntil), }) require.NoError(t, err) require.NotEmpty(t, userStatusChanges) gotCounts := map[time.Time]map[database.UserStatus]int64{} for _, row := range userStatusChanges { - dateInLocation := row.Date.In(location) + dateInLocation := row.Date.In(tc.location) if gotCounts[dateInLocation] == nil { gotCounts[dateInLocation] = map[database.UserStatus]int64{} } @@ -4698,30 +4743,30 @@ func TestGetUserStatusCounts(t *testing.T) { } expectedCounts := map[time.Time]map[database.UserStatus]int64{} - for d := dbtime.StartOfDay(createdAt); !d.After(dbtime.StartOfDay(today)); d = d.AddDate(0, 0, 1) { + for d := dbtime.StartOfDay(userCreatedAt); !d.After(dbtime.StartOfDay(tc.reportUntil)); d = d.AddDate(0, 0, 1) { expectedCounts[d] = map[database.UserStatus]int64{} // Default values - expectedCounts[d][tc.user1Transition.from] = 0 - expectedCounts[d][tc.user1Transition.to] = 0 - expectedCounts[d][tc.user2Transition.from] = 0 - expectedCounts[d][tc.user2Transition.to] = 0 + expectedCounts[d][stc.user1Transition.from] = 0 + expectedCounts[d][stc.user1Transition.to] = 0 + expectedCounts[d][stc.user2Transition.from] = 0 + expectedCounts[d][stc.user2Transition.to] = 0 // Counted Values switch { - case d.Before(createdAt): + case d.Before(userCreatedAt): continue - case d.Before(firstTransitionTime): - expectedCounts[d][tc.user1Transition.from]++ - expectedCounts[d][tc.user2Transition.from]++ - case d.Before(secondTransitionTime): - expectedCounts[d][tc.user1Transition.to]++ - expectedCounts[d][tc.user2Transition.from]++ - case d.Before(today): - expectedCounts[d][tc.user1Transition.to]++ - expectedCounts[d][tc.user2Transition.to]++ + case d.Before(firstStatusChange): + expectedCounts[d][stc.user1Transition.from]++ + expectedCounts[d][stc.user2Transition.from]++ + case d.Before(secondStatusChange): + expectedCounts[d][stc.user1Transition.to]++ + expectedCounts[d][stc.user2Transition.from]++ + case !d.After(tc.reportUntil): + expectedCounts[d][stc.user1Transition.to]++ + expectedCounts[d][stc.user2Transition.to]++ default: - t.Fatalf("date %q beyond expected range end %q", d, today) + t.Fatalf("date %q beyond expected range end %q", d, tc.reportUntil) } } @@ -4737,23 +4782,24 @@ func TestGetUserStatusCounts(t *testing.T) { _ = dbgen.User(t, db, database.User{ Status: database.UserStatusActive, - CreatedAt: createdAt, - UpdatedAt: createdAt, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: dbtime.StartOfDay(createdAt.Add(time.Hour * 24)), - EndTime: dbtime.StartOfDay(today), + Tz: tc.timezone, + StartTime: dbtime.StartOfDay(userCreatedAt.Add(time.Hour * 24)), + EndTime: dbtime.StartOfDay(tc.reportUntil), }) require.NoError(t, err) for i, row := range userStatusChanges { require.True( t, - row.Date.In(location).Equal(dbtime.StartOfDay(createdAt).AddDate(0, 0, 1+i)), + row.Date.In(tc.location).Equal(dbtime.StartOfDay(userCreatedAt).AddDate(0, 0, 1+i)), "expected date %s, but got %s for row %n", - dbtime.StartOfDay(createdAt).AddDate(0, 0, 1+i), - row.Date.In(location).String(), + dbtime.StartOfDay(userCreatedAt).AddDate(0, 0, 1+i), + row.Date.In(tc.location).String(), i, ) require.Equal(t, database.UserStatusActive, row.Status) @@ -4763,21 +4809,25 @@ func TestGetUserStatusCounts(t *testing.T) { t.Run("User deleted before query range", func(t *testing.T) { t.Parallel() - db, _ := dbtestutil.NewDB(t) + db, _, sqlDB := dbtestutil.NewDBWithSQLDB(t) ctx := testutil.Context(t, testutil.WaitShort) user := dbgen.User(t, db, database.User{ Status: database.UserStatusActive, - CreatedAt: createdAt, - UpdatedAt: createdAt, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) - err = db.UpdateUserDeletedByID(ctx, user.ID) + err := db.UpdateUserDeletedByID(ctx, user.ID) + require.NoError(t, err) + + _, err = sqlDB.ExecContext(ctx, "UPDATE user_deleted SET deleted_at = $1 WHERE user_id = $2", tc.reportUntil, user.ID) require.NoError(t, err) userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: today.Add(time.Hour * 24), - EndTime: today.Add(time.Hour * 48), + Tz: tc.timezone, + StartTime: tc.reportUntil.Add(time.Hour * 24), + EndTime: tc.reportUntil.Add(time.Hour * 48), }) require.NoError(t, err) require.Empty(t, userStatusChanges) @@ -4786,37 +4836,45 @@ func TestGetUserStatusCounts(t *testing.T) { t.Run("User deleted during query range", func(t *testing.T) { t.Parallel() - db, _ := dbtestutil.NewDB(t) + db, _, sqlDB := dbtestutil.NewDBWithSQLDB(t) ctx := testutil.Context(t, testutil.WaitShort) user := dbgen.User(t, db, database.User{ Status: database.UserStatusActive, - CreatedAt: createdAt, - UpdatedAt: createdAt, + CreatedAt: userCreatedAt, + UpdatedAt: userCreatedAt, }) err := db.UpdateUserDeletedByID(ctx, user.ID) require.NoError(t, err) + _, err = sqlDB.ExecContext(ctx, "UPDATE user_deleted SET deleted_at = $1 WHERE user_id = $2", tc.reportUntil, user.ID) + require.NoError(t, err) + userStatusChanges, err := db.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ - StartTime: dbtime.StartOfDay(createdAt), - EndTime: dbtime.StartOfDay(today.Add(time.Hour * 24)), + Tz: tc.timezone, + StartTime: dbtime.StartOfDay(userCreatedAt), + EndTime: dbtime.StartOfDay(tc.reportUntil.Add(time.Hour * 24)), }) require.NoError(t, err) for i, row := range userStatusChanges { - require.True( + row.Date = row.Date.In(tc.location) + userStatusChanges[i] = row + target := dbtime.StartOfDay(userCreatedAt).AddDate(0, 0, i) + assert.True( t, - row.Date.In(location).Equal(dbtime.StartOfDay(createdAt).AddDate(0, 0, i)), + row.Date.Equal(target), "expected date %s, but got %s for row %n", - dbtime.StartOfDay(createdAt).AddDate(0, 0, i), - row.Date.In(location).String(), + target.String(), + row.Date.String(), i, ) require.Equal(t, database.UserStatusActive, row.Status) switch { - case row.Date.Before(createdAt): + case row.Date.Before(userCreatedAt): require.Equal(t, int64(0), row.Count) - case i == len(userStatusChanges)-1: + case !row.Date.Before(tc.reportUntil): + // On or after the deletion date, the user should not be counted. require.Equal(t, int64(0), row.Count) default: require.Equal(t, int64(1), row.Count) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f75da88cc8..088c1cdf12 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7089,79 +7089,69 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate const getUserStatusCounts = `-- name: GetUserStatusCounts :many WITH - -- dates_of_interest defines all points in time that are relevant to the query. - -- It includes the start_time, all status changes, all deletions, and the end_time. -dates_of_interest AS ( - SELECT date FROM generate_series( - $1::timestamptz, - $2::timestamptz, - (CASE WHEN $3::int <= 0 THEN 3600 * 24 ELSE $3::int END || ' seconds')::interval - ) AS date +system_users AS ( + SELECT id FROM users WHERE is_system = TRUE ), - -- latest_status_before_range defines the status of each user before the start_time. - -- We do not include users who were deleted before the start_time. We use this to ensure that - -- we correctly count users prior to the start_time for a complete graph. + -- dates_of_interest generates the dates that will represent the horizontal axis of the chart. +dates_of_interest AS ( + SELECT timezone($1::text, gs_local) AS date + FROM generate_series( + timezone($1::text, $2::timestamptz), + timezone($1::text, $3::timestamptz), + interval '1 day' + ) AS gs_local +), + -- latest_status_before_range selects the last status of each user before the start_time. + -- This represents the status of all users at the start of the time range. latest_status_before_range AS ( SELECT DISTINCT usc.user_id, usc.new_status, - usc.changed_at, - ud.deleted + usc.changed_at FROM user_status_changes usc LEFT JOIN LATERAL ( SELECT COUNT(*) > 0 AS deleted FROM user_deleted ud - WHERE ud.user_id = usc.user_id AND (ud.deleted_at < usc.changed_at OR ud.deleted_at < $1) + WHERE ud.user_id = usc.user_id AND (ud.deleted_at < usc.changed_at OR ud.deleted_at < $2) ) AS ud ON true - WHERE usc.changed_at < $1::timestamptz + WHERE usc.user_id NOT IN (SELECT id FROM system_users) + AND NOT ud.deleted + AND usc.changed_at < $2::timestamptz ORDER BY usc.user_id, usc.changed_at DESC ), - -- status_changes_during_range defines the status of each user during the start_time and end_time. - -- If a user is deleted during the time range, we count status changes between the start_time and the deletion date. - -- Theoretically, it should probably not be possible to update the status of a deleted user, but we - -- need to ensure that this is enforced, so that a change in business logic later does not break this graph. + -- status_changes_during_range selects the statuses of each user during the start_time and end_time. status_changes_during_range AS ( SELECT usc.user_id, usc.new_status, - usc.changed_at, - ud.deleted + usc.changed_at FROM user_status_changes usc LEFT JOIN LATERAL ( SELECT COUNT(*) > 0 AS deleted FROM user_deleted ud WHERE ud.user_id = usc.user_id AND ud.deleted_at < usc.changed_at ) AS ud ON true - WHERE usc.changed_at >= $1::timestamptz - AND usc.changed_at <= $2::timestamptz + WHERE usc.user_id NOT IN (SELECT id FROM system_users) + AND NOT ud.deleted + AND usc.changed_at >= $2::timestamptz + AND usc.changed_at <= $3::timestamptz ), - -- relevant_status_changes defines the status of each user at any point in time. - -- It includes the status of each user before the start_time, and the status of each user during the start_time and end_time. relevant_status_changes AS ( - SELECT - user_id, - new_status, - changed_at + SELECT user_id, new_status, changed_at FROM latest_status_before_range - WHERE NOT deleted UNION ALL - SELECT - user_id, - new_status, - changed_at + SELECT user_id, new_status, changed_at FROM status_changes_during_range - WHERE NOT deleted ), - -- statuses defines all the distinct statuses that were present just before and during the time range. - -- This is used to ensure that we have a series for every relevant status. + -- statuses selects all the distinct statuses that were present just before and during the time range. + -- Each status will have a series on the chart. statuses AS ( SELECT DISTINCT new_status FROM relevant_status_changes ), - -- We only want to count the latest status change for each user on each date and then filter them by the relevant status. - -- We use the row_number function to ensure that we only count the latest status change for each user on each date. - -- We then filter the status changes by the relevant status in the final select statement below. + -- ranked_status_change_per_user_per_date selects the latest status change for each user on each date. + -- The last status for a user on every given date will be counted. ranked_status_change_per_user_per_date AS ( SELECT d.date, @@ -7194,9 +7184,9 @@ ORDER BY rscpupd.date ` type GetUserStatusCountsParams struct { + Tz string `db:"tz" json:"tz"` StartTime time.Time `db:"start_time" json:"start_time"` EndTime time.Time `db:"end_time" json:"end_time"` - Interval int32 `db:"interval" json:"interval"` } type GetUserStatusCountsRow struct { @@ -7207,18 +7197,8 @@ type GetUserStatusCountsRow struct { // GetUserStatusCounts returns the count of users in each status over time. // The time range is inclusively defined by the start_time and end_time parameters. -// -// Bucketing: -// Between the start_time and end_time, we include each timestamp where a user's status changed or they were deleted. -// We do not bucket these results by day or some other time unit. This is because such bucketing would hide potentially -// important patterns. If a user was active for 23 hours and 59 minutes, and then suspended, a daily bucket would hide this. -// A daily bucket would also have required us to carefully manage the timezone of the bucket based on the timezone of the user. -// -// Accumulation: -// We do not start counting from 0 at the start_time. We check the last status change before the start_time for each user. As such, -// the result shows the total number of users in each status on any particular day. func (q *sqlQuerier) GetUserStatusCounts(ctx context.Context, arg GetUserStatusCountsParams) ([]GetUserStatusCountsRow, error) { - rows, err := q.db.QueryContext(ctx, getUserStatusCounts, arg.StartTime, arg.EndTime, arg.Interval) + rows, err := q.db.QueryContext(ctx, getUserStatusCounts, arg.Tz, arg.StartTime, arg.EndTime) if err != nil { return nil, err } diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 1588d68f31..b589ce4e9a 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -805,90 +805,70 @@ GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.de -- name: GetUserStatusCounts :many -- GetUserStatusCounts returns the count of users in each status over time. -- The time range is inclusively defined by the start_time and end_time parameters. --- --- Bucketing: --- Between the start_time and end_time, we include each timestamp where a user's status changed or they were deleted. --- We do not bucket these results by day or some other time unit. This is because such bucketing would hide potentially --- important patterns. If a user was active for 23 hours and 59 minutes, and then suspended, a daily bucket would hide this. --- A daily bucket would also have required us to carefully manage the timezone of the bucket based on the timezone of the user. --- --- Accumulation: --- We do not start counting from 0 at the start_time. We check the last status change before the start_time for each user. As such, --- the result shows the total number of users in each status on any particular day. WITH - -- dates_of_interest defines all points in time that are relevant to the query. - -- It includes the start_time, all status changes, all deletions, and the end_time. -dates_of_interest AS ( - SELECT date FROM generate_series( - @start_time::timestamptz, - @end_time::timestamptz, - (CASE WHEN @interval::int <= 0 THEN 3600 * 24 ELSE @interval::int END || ' seconds')::interval - ) AS date +system_users AS ( + SELECT id FROM users WHERE is_system = TRUE ), - -- latest_status_before_range defines the status of each user before the start_time. - -- We do not include users who were deleted before the start_time. We use this to ensure that - -- we correctly count users prior to the start_time for a complete graph. + -- dates_of_interest generates the dates that will represent the horizontal axis of the chart. +dates_of_interest AS ( + SELECT timezone(@tz::text, gs_local) AS date + FROM generate_series( + timezone(@tz::text, @start_time::timestamptz), + timezone(@tz::text, @end_time::timestamptz), + interval '1 day' + ) AS gs_local +), + -- latest_status_before_range selects the last status of each user before the start_time. + -- This represents the status of all users at the start of the time range. latest_status_before_range AS ( SELECT DISTINCT usc.user_id, usc.new_status, - usc.changed_at, - ud.deleted + usc.changed_at FROM user_status_changes usc LEFT JOIN LATERAL ( SELECT COUNT(*) > 0 AS deleted FROM user_deleted ud WHERE ud.user_id = usc.user_id AND (ud.deleted_at < usc.changed_at OR ud.deleted_at < @start_time) ) AS ud ON true - WHERE usc.changed_at < @start_time::timestamptz + WHERE usc.user_id NOT IN (SELECT id FROM system_users) + AND NOT ud.deleted + AND usc.changed_at < @start_time::timestamptz ORDER BY usc.user_id, usc.changed_at DESC ), - -- status_changes_during_range defines the status of each user during the start_time and end_time. - -- If a user is deleted during the time range, we count status changes between the start_time and the deletion date. - -- Theoretically, it should probably not be possible to update the status of a deleted user, but we - -- need to ensure that this is enforced, so that a change in business logic later does not break this graph. + -- status_changes_during_range selects the statuses of each user during the start_time and end_time. status_changes_during_range AS ( SELECT usc.user_id, usc.new_status, - usc.changed_at, - ud.deleted + usc.changed_at FROM user_status_changes usc LEFT JOIN LATERAL ( SELECT COUNT(*) > 0 AS deleted FROM user_deleted ud WHERE ud.user_id = usc.user_id AND ud.deleted_at < usc.changed_at ) AS ud ON true - WHERE usc.changed_at >= @start_time::timestamptz + WHERE usc.user_id NOT IN (SELECT id FROM system_users) + AND NOT ud.deleted + AND usc.changed_at >= @start_time::timestamptz AND usc.changed_at <= @end_time::timestamptz ), - -- relevant_status_changes defines the status of each user at any point in time. - -- It includes the status of each user before the start_time, and the status of each user during the start_time and end_time. relevant_status_changes AS ( - SELECT - user_id, - new_status, - changed_at + SELECT user_id, new_status, changed_at FROM latest_status_before_range - WHERE NOT deleted UNION ALL - SELECT - user_id, - new_status, - changed_at + SELECT user_id, new_status, changed_at FROM status_changes_during_range - WHERE NOT deleted ), - -- statuses defines all the distinct statuses that were present just before and during the time range. - -- This is used to ensure that we have a series for every relevant status. + -- statuses selects all the distinct statuses that were present just before and during the time range. + -- Each status will have a series on the chart. statuses AS ( SELECT DISTINCT new_status FROM relevant_status_changes ), - -- We only want to count the latest status change for each user on each date and then filter them by the relevant status. - -- We use the row_number function to ensure that we only count the latest status change for each user on each date. - -- We then filter the status changes by the relevant status in the final select statement below. + -- ranked_status_change_per_user_per_date selects the latest status change for each user on each date. + -- The last status for a user on every given date will be counted. ranked_status_change_per_user_per_date AS ( SELECT d.date, diff --git a/coderd/insights.go b/coderd/insights.go index b8ae6e6481..c477df6342 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -298,7 +298,8 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { // @Security CoderSessionToken // @Produce json // @Tags Insights -// @Param tz_offset query int true "Time-zone offset (e.g. -2)" +// @Param timezone query string false "IANA timezone name (e.g. America/St_Johns)" +// @Param tz_offset query int false "Deprecated: Time-zone offset (e.g. -2). Use timezone instead." // @Success 200 {object} codersdk.GetUserStatusCountsResponse // @Router /insights/user-status-counts [get] func (api *API) insightsUserStatusCounts(rw http.ResponseWriter, r *http.Request) { @@ -306,8 +307,9 @@ func (api *API) insightsUserStatusCounts(rw http.ResponseWriter, r *http.Request p := httpapi.NewQueryParamParser() vals := r.URL.Query() + timezone := p.String(vals, "", "timezone") tzOffset := p.Int(vals, 0, "tz_offset") - interval := p.Int(vals, int((24 * time.Hour).Seconds()), "interval") + _ = p.Int(vals, 0, "interval") // Deprecated: ignored, kept for backward compatibility. p.ErrorExcessParams(vals) if len(p.Errors) > 0 { @@ -318,16 +320,45 @@ func (api *API) insightsUserStatusCounts(rw http.ResponseWriter, r *http.Request return } - loc := time.FixedZone("", tzOffset*3600) + if timezone != "" && tzOffset != 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Provide either \"timezone\" or \"tz_offset\", not both.", + }) + return + } + + var loc *time.Location + if timezone == "" { + timezone = "UTC" + if tzOffset > 0 { + timezone = fmt.Sprintf("Etc/GMT-%d", tzOffset) + } else if tzOffset < 0 { + timezone = fmt.Sprintf("Etc/GMT+%d", -tzOffset) + } + } + + loc, err := time.LoadLocation(timezone) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid timezone.", + Detail: err.Error(), + }) + return + } + nextHourInLoc := dbtime.Now().Truncate(time.Hour).Add(time.Hour).In(loc) sixtyDaysAgo := dbtime.StartOfDay(nextHourInLoc).AddDate(0, 0, -60) - rows, err := api.Database.GetUserStatusCounts(ctx, database.GetUserStatusCountsParams{ + queryParams := database.GetUserStatusCountsParams{ StartTime: sixtyDaysAgo, EndTime: nextHourInLoc, - // #nosec G115 - Interval value is small and fits in int32 (typically days or hours) - Interval: int32(interval), - }) + // loc.String() returns an IANA timezone name (e.g. "America/New_York"). + // Both Go and PostgreSQL use the IANA Time Zone Database, so names are + // compatible. The Etc/GMT±N names used for offset fallback are also valid + // in both systems. + Tz: loc.String(), + } + rows, err := api.Database.GetUserStatusCounts(ctx, queryParams) if err != nil { if httpapi.IsUnauthorizedError(err) { httpapi.Forbidden(rw) diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 09850e4d57..b1cf6ee87b 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -2443,7 +2443,8 @@ func TestGenericInsights_Disabled(t *testing.T) { name: "UserStatusCounts", fn: func(ctx context.Context) error { _, err := client.GetUserStatusCounts(ctx, codersdk.GetUserStatusCountsRequest{ - Offset: 0, + Timezone: "America/St_Johns", + Offset: -2, }) return err }, @@ -2479,3 +2480,83 @@ func TestGenericInsights_Disabled(t *testing.T) { }) } } + +func TestGetUserStatusCounts(t *testing.T) { + t.Parallel() + + type testCase struct { + name string + request codersdk.GetUserStatusCountsRequest + checkError func(t *testing.T, err error) + checkResponse func(t *testing.T, resp codersdk.GetUserStatusCountsResponse) + } + + happyResponseCheck := func(t *testing.T, resp codersdk.GetUserStatusCountsResponse) { + require.Len(t, resp.StatusCounts, 1) + require.NotNil(t, resp.StatusCounts[codersdk.UserStatusActive]) + require.Len(t, resp.StatusCounts[codersdk.UserStatusActive], 61) + for _, count := range resp.StatusCounts[codersdk.UserStatusActive] { + require.Zero(t, count.Count) + } + } + testcases := []testCase{ + { + name: "OK when timezone and offset are provided", + request: codersdk.GetUserStatusCountsRequest{ + Timezone: "America/St_Johns", + Offset: -2, + }, + checkError: func(t *testing.T, err error) { + require.NoError(t, err) + }, + checkResponse: happyResponseCheck, + }, + { + name: "OK when timezone without offset", + request: codersdk.GetUserStatusCountsRequest{ + Timezone: "America/St_Johns", + }, + checkError: func(t *testing.T, err error) { + require.NoError(t, err) + }, + checkResponse: happyResponseCheck, + }, + { + name: "OK when offset is provided without timezone", + request: codersdk.GetUserStatusCountsRequest{ + Offset: -2, + }, + checkError: func(t *testing.T, err error) { + require.NoError(t, err) + }, + checkResponse: happyResponseCheck, + }, + { + name: "Error when timezone is invalid", + request: codersdk.GetUserStatusCountsRequest{ + Timezone: "Invalid/Timezone", + }, + checkError: func(t *testing.T, err error) { + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + assert.ErrorContains(t, cerr, "unknown time zone") + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }, + checkResponse: func(t *testing.T, resp codersdk.GetUserStatusCountsResponse) { + require.Empty(t, resp.StatusCounts) + }, + }, + } + + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + for _, tt := range testcases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + resp, err := client.GetUserStatusCounts(ctx, tt.request) + tt.checkError(t, err) + tt.checkResponse(t, resp) + }) + } +} diff --git a/codersdk/insights.go b/codersdk/insights.go index 301411d412..e757f28d18 100644 --- a/codersdk/insights.go +++ b/codersdk/insights.go @@ -294,14 +294,18 @@ type UserStatusChangeCount struct { } type GetUserStatusCountsRequest struct { - // Timezone offset in hours. Use 0 for UTC, and TimezoneOffsetHour(time.Local) - // for the local timezone. - Offset int `json:"offset"` + Timezone string `json:"timezone" example:"America/St_Johns"` + // Deprecated: Use Timezone instead. Offset is ignored when Timezone is provided. + Offset int `json:"offset,omitempty" example:"-2"` } func (c *Client) GetUserStatusCounts(ctx context.Context, req GetUserStatusCountsRequest) (GetUserStatusCountsResponse, error) { qp := url.Values{} - qp.Add("tz_offset", strconv.Itoa(req.Offset)) + if req.Timezone != "" { + qp.Add("timezone", req.Timezone) + } else { + qp.Add("tz_offset", strconv.Itoa(req.Offset)) + } reqURL := fmt.Sprintf("/api/v2/insights/user-status-counts?%s", qp.Encode()) resp, err := c.Request(ctx, http.MethodGet, reqURL, nil) diff --git a/docs/reference/api/insights.md b/docs/reference/api/insights.md index 62bdf0541a..7e45126fba 100644 --- a/docs/reference/api/insights.md +++ b/docs/reference/api/insights.md @@ -266,7 +266,7 @@ To perform this operation, you must be authenticated. [Learn more](authenticatio ```shell # Example request using curl -curl -X GET http://coder-server:8080/api/v2/insights/user-status-counts?tz_offset=0 \ +curl -X GET http://coder-server:8080/api/v2/insights/user-status-counts \ -H 'Accept: application/json' \ -H 'Coder-Session-Token: API_KEY' ``` @@ -275,9 +275,10 @@ curl -X GET http://coder-server:8080/api/v2/insights/user-status-counts?tz_offse ### Parameters -| Name | In | Type | Required | Description | -|-------------|-------|---------|----------|----------------------------| -| `tz_offset` | query | integer | true | Time-zone offset (e.g. -2) | +| Name | In | Type | Required | Description | +|-------------|-------|---------|----------|---------------------------------------------------------------| +| `timezone` | query | string | false | IANA timezone name (e.g. America/St_Johns) | +| `tz_offset` | query | integer | false | Deprecated: Time-zone offset (e.g. -2). Use timezone instead. | ### Example responses diff --git a/site/src/api/api.ts b/site/src/api/api.ts index c1742cff45..99715bbc96 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2533,11 +2533,14 @@ class ApiMethods { return response.data; }; + // Intl.DateTimeFormat().resolvedOptions().timeZone returns an IANA timezone + // name (e.g. "America/New_York") per ECMA-402. Go's time.LoadLocation and + // PostgreSQL's timezone() both accept IANA names, so these are compatible. getInsightsUserStatusCounts = async ( - offset = Math.trunc(new Date().getTimezoneOffset() / 60), + timezone = Intl.DateTimeFormat().resolvedOptions().timeZone, ): Promise => { const searchParams = new URLSearchParams({ - tz_offset: offset.toString(), + timezone, }); const response = await this.axios.get( `/api/v2/insights/user-status-counts?${searchParams}`, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 5abc003815..94402e4c92 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2838,11 +2838,11 @@ export interface GetInboxNotificationResponse { // From codersdk/insights.go export interface GetUserStatusCountsRequest { + readonly timezone: string; /** - * Timezone offset in hours. Use 0 for UTC, and TimezoneOffsetHour(time.Local) - * for the local timezone. + * Deprecated: Use Timezone instead. Offset is ignored when Timezone is provided. */ - readonly offset: number; + readonly offset?: number; } // From codersdk/insights.go