mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: user status change chart accommodates DST (#22191)
closes https://github.com/coder/internal/issues/464 # Summary This PR resolves a flaky test that was sensitive to DST transitions in various time zones. The root of the flake was: * a bug; the query and its tests assume 24 hours per day * the tests used local system time, which resulted in failures for dates proximal to DST transitions # Changes Query: The original query assumed 24 hour intervals between each day, which is not a valid assumption. It now increments `1 day` at a time. Database tests: Database level tests for the query all assumed 24 hour days. They now increment in DST-aware days instead. Instead of using time.Now() as a base for testing, the test uses a series of dates over the course of an entire year, to ensure that DST transition dates are present in every test run. # API Endpoint The endpoint that delivers the user status chart now accepts an IANA timezone name as a parameter and passes it, keeping the existing offset as a fallback, to the database query. API level tests were added to ensure the correct response form and error behaviour. Correctness of content is tested at the database level.
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user