From 4cd66af5fbff7c29bdbefae36026305d785cbe8e Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sat, 14 Dec 2024 09:46:16 +0000 Subject: [PATCH 01/23] add user_status_changes table --- coderd/database/dump.sql | 38 ++++++++++++++++ coderd/database/foreign_key_constraint.go | 1 + .../000279_user_status_changes.down.sql | 12 ++++++ .../000279_user_status_changes.up.sql | 43 +++++++++++++++++++ coderd/database/models.go | 8 ++++ coderd/database/unique_constraint.go | 1 + 6 files changed, 103 insertions(+) create mode 100644 coderd/database/migrations/000279_user_status_changes.down.sql create mode 100644 coderd/database/migrations/000279_user_status_changes.up.sql diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index f91a5371f06f6..46d64568c46b5 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -417,6 +417,23 @@ $$; COMMENT ON FUNCTION provisioner_tagset_contains(provisioner_tags tagset, job_tags tagset) IS 'Returns true if the provisioner_tags contains the job_tags, or if the job_tags represents an untagged provisioner and the superset is exactly equal to the subset.'; +CREATE FUNCTION record_user_status_change() RETURNS trigger + LANGUAGE plpgsql + AS $$ +BEGIN + IF OLD.status IS DISTINCT FROM NEW.status THEN + INSERT INTO user_status_changes ( + user_id, + new_status + ) VALUES ( + NEW.id, + NEW.status + ); + END IF; + RETURN NEW; +END; +$$; + CREATE FUNCTION remove_organization_member_role() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -1389,6 +1406,15 @@ COMMENT ON COLUMN user_links.oauth_refresh_token_key_id IS 'The ID of the key us COMMENT ON COLUMN user_links.claims IS 'Claims from the IDP for the linked user. Includes both id_token and userinfo claims. '; +CREATE TABLE user_status_changes ( + id uuid DEFAULT gen_random_uuid() NOT NULL, + user_id uuid NOT NULL, + new_status user_status NOT NULL, + changed_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL +); + +COMMENT ON TABLE user_status_changes IS 'Tracks the history of user status changes'; + CREATE TABLE workspace_agent_log_sources ( workspace_agent_id uuid NOT NULL, id uuid NOT NULL, @@ -1976,6 +2002,9 @@ ALTER TABLE ONLY templates ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_pkey PRIMARY KEY (user_id, login_type); +ALTER TABLE ONLY user_status_changes + ADD CONSTRAINT user_status_changes_pkey PRIMARY KEY (id); + ALTER TABLE ONLY users ADD CONSTRAINT users_pkey PRIMARY KEY (id); @@ -2086,6 +2115,10 @@ CREATE INDEX idx_tailnet_tunnels_dst_id ON tailnet_tunnels USING hash (dst_id); CREATE INDEX idx_tailnet_tunnels_src_id ON tailnet_tunnels USING hash (src_id); +CREATE INDEX idx_user_status_changes_changed_at ON user_status_changes USING btree (changed_at); + +CREATE INDEX idx_user_status_changes_user_id ON user_status_changes USING btree (user_id); + CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted = false); CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); @@ -2228,6 +2261,8 @@ CREATE TRIGGER trigger_upsert_user_links BEFORE INSERT OR UPDATE ON user_links F CREATE TRIGGER update_notification_message_dedupe_hash BEFORE INSERT OR UPDATE ON notification_messages FOR EACH ROW EXECUTE FUNCTION compute_notification_message_dedupe_hash(); +CREATE TRIGGER user_status_change_trigger BEFORE UPDATE ON users FOR EACH ROW EXECUTE FUNCTION record_user_status_change(); + ALTER TABLE ONLY api_keys ADD CONSTRAINT api_keys_user_id_uuid_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; @@ -2360,6 +2395,9 @@ ALTER TABLE ONLY user_links ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; +ALTER TABLE ONLY user_status_changes + ADD CONSTRAINT user_status_changes_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id); + ALTER TABLE ONLY workspace_agent_log_sources ADD CONSTRAINT workspace_agent_log_sources_workspace_agent_id_fkey FOREIGN KEY (workspace_agent_id) REFERENCES workspace_agents(id) ON DELETE CASCADE; diff --git a/coderd/database/foreign_key_constraint.go b/coderd/database/foreign_key_constraint.go index 669ab85f945bd..18c82b83750fa 100644 --- a/coderd/database/foreign_key_constraint.go +++ b/coderd/database/foreign_key_constraint.go @@ -50,6 +50,7 @@ const ( ForeignKeyUserLinksOauthAccessTokenKeyID ForeignKeyConstraint = "user_links_oauth_access_token_key_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_oauth_access_token_key_id_fkey FOREIGN KEY (oauth_access_token_key_id) REFERENCES dbcrypt_keys(active_key_digest); ForeignKeyUserLinksOauthRefreshTokenKeyID ForeignKeyConstraint = "user_links_oauth_refresh_token_key_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_oauth_refresh_token_key_id_fkey FOREIGN KEY (oauth_refresh_token_key_id) REFERENCES dbcrypt_keys(active_key_digest); ForeignKeyUserLinksUserID ForeignKeyConstraint = "user_links_user_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; + ForeignKeyUserStatusChangesUserID ForeignKeyConstraint = "user_status_changes_user_id_fkey" // ALTER TABLE ONLY user_status_changes ADD CONSTRAINT user_status_changes_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id); ForeignKeyWorkspaceAgentLogSourcesWorkspaceAgentID ForeignKeyConstraint = "workspace_agent_log_sources_workspace_agent_id_fkey" // ALTER TABLE ONLY workspace_agent_log_sources ADD CONSTRAINT workspace_agent_log_sources_workspace_agent_id_fkey FOREIGN KEY (workspace_agent_id) REFERENCES workspace_agents(id) ON DELETE CASCADE; ForeignKeyWorkspaceAgentMetadataWorkspaceAgentID ForeignKeyConstraint = "workspace_agent_metadata_workspace_agent_id_fkey" // ALTER TABLE ONLY workspace_agent_metadata ADD CONSTRAINT workspace_agent_metadata_workspace_agent_id_fkey FOREIGN KEY (workspace_agent_id) REFERENCES workspace_agents(id) ON DELETE CASCADE; ForeignKeyWorkspaceAgentPortShareWorkspaceID ForeignKeyConstraint = "workspace_agent_port_share_workspace_id_fkey" // ALTER TABLE ONLY workspace_agent_port_share ADD CONSTRAINT workspace_agent_port_share_workspace_id_fkey FOREIGN KEY (workspace_id) REFERENCES workspaces(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000279_user_status_changes.down.sql b/coderd/database/migrations/000279_user_status_changes.down.sql new file mode 100644 index 0000000000000..b75acbcafca46 --- /dev/null +++ b/coderd/database/migrations/000279_user_status_changes.down.sql @@ -0,0 +1,12 @@ +-- Drop the trigger first +DROP TRIGGER IF EXISTS user_status_change_trigger ON users; + +-- Drop the trigger function +DROP FUNCTION IF EXISTS record_user_status_change(); + +-- Drop the indexes +DROP INDEX IF EXISTS idx_user_status_changes_changed_at; +DROP INDEX IF EXISTS idx_user_status_changes_user_id; + +-- Drop the table +DROP TABLE IF EXISTS user_status_changes; diff --git a/coderd/database/migrations/000279_user_status_changes.up.sql b/coderd/database/migrations/000279_user_status_changes.up.sql new file mode 100644 index 0000000000000..545705aba50eb --- /dev/null +++ b/coderd/database/migrations/000279_user_status_changes.up.sql @@ -0,0 +1,43 @@ +CREATE TABLE user_status_changes ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + user_id uuid NOT NULL REFERENCES users(id), + new_status user_status NOT NULL, + changed_at timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +COMMENT ON TABLE user_status_changes IS 'Tracks the history of user status changes'; + +CREATE INDEX idx_user_status_changes_user_id ON user_status_changes(user_id); +CREATE INDEX idx_user_status_changes_changed_at ON user_status_changes(changed_at); + +INSERT INTO user_status_changes ( + user_id, + new_status, + changed_at +) +SELECT + id, + status, + created_at +FROM users +WHERE NOT deleted; + +CREATE FUNCTION record_user_status_change() RETURNS trigger AS $$ +BEGIN + IF OLD.status IS DISTINCT FROM NEW.status THEN + INSERT INTO user_status_changes ( + user_id, + new_status + ) VALUES ( + NEW.id, + NEW.status + ); + END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER user_status_change_trigger + BEFORE UPDATE ON users + FOR EACH ROW + EXECUTE FUNCTION record_user_status_change(); diff --git a/coderd/database/models.go b/coderd/database/models.go index e9a5f93051ba5..81b56370ad2ea 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2907,6 +2907,14 @@ type UserLink struct { Claims UserLinkClaims `db:"claims" json:"claims"` } +// Tracks the history of user status changes +type UserStatusChange struct { + ID uuid.UUID `db:"id" json:"id"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + NewStatus UserStatus `db:"new_status" json:"new_status"` + ChangedAt time.Time `db:"changed_at" json:"changed_at"` +} + // Visible fields of users are allowed to be joined with other tables for including context of other resources. type VisibleUser struct { ID uuid.UUID `db:"id" json:"id"` diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index f4470c6546698..b59cb0cbc8091 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -63,6 +63,7 @@ const ( UniqueTemplateVersionsTemplateIDNameKey UniqueConstraint = "template_versions_template_id_name_key" // ALTER TABLE ONLY template_versions ADD CONSTRAINT template_versions_template_id_name_key UNIQUE (template_id, name); UniqueTemplatesPkey UniqueConstraint = "templates_pkey" // ALTER TABLE ONLY templates ADD CONSTRAINT templates_pkey PRIMARY KEY (id); UniqueUserLinksPkey UniqueConstraint = "user_links_pkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_pkey PRIMARY KEY (user_id, login_type); + UniqueUserStatusChangesPkey UniqueConstraint = "user_status_changes_pkey" // ALTER TABLE ONLY user_status_changes ADD CONSTRAINT user_status_changes_pkey PRIMARY KEY (id); UniqueUsersPkey UniqueConstraint = "users_pkey" // ALTER TABLE ONLY users ADD CONSTRAINT users_pkey PRIMARY KEY (id); UniqueWorkspaceAgentLogSourcesPkey UniqueConstraint = "workspace_agent_log_sources_pkey" // ALTER TABLE ONLY workspace_agent_log_sources ADD CONSTRAINT workspace_agent_log_sources_pkey PRIMARY KEY (workspace_agent_id, id); UniqueWorkspaceAgentMetadataPkey UniqueConstraint = "workspace_agent_metadata_pkey" // ALTER TABLE ONLY workspace_agent_metadata ADD CONSTRAINT workspace_agent_metadata_pkey PRIMARY KEY (workspace_agent_id, key); From 61b27bfac353113954ec1888d3ed239df252e4b5 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sat, 14 Dec 2024 10:37:43 +0000 Subject: [PATCH 02/23] add GetUserStatusCountsByDay --- coderd/database/dbauthz/dbauthz.go | 7 ++ coderd/database/dbmem/dbmem.go | 9 ++ coderd/database/dbmetrics/querymetrics.go | 7 ++ coderd/database/dbmock/dbmock.go | 15 ++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 103 ++++++++++++++++++++++ coderd/database/queries/insights.sql | 68 ++++++++++++++ 7 files changed, 210 insertions(+) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index f64dbcc166591..ffc1eb63d8dd0 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2413,6 +2413,13 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui return q.db.GetUserNotificationPreferences(ctx, userID) } +func (q *querier) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { + return nil, err + } + return q.db.GetUserStatusCountsByDay(ctx, arg) +} + func (q *querier) GetUserWorkspaceBuildParameters(ctx context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { u, err := q.db.GetUserByID(ctx, params.OwnerID) if err != nil { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 7b19790a6d8ea..306e9596b631f 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5664,6 +5664,15 @@ func (q *FakeQuerier) GetUserNotificationPreferences(_ context.Context, userID u return out, nil } +func (q *FakeQuerier) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { + err := validateDatabaseType(arg) + if err != nil { + return nil, err + } + + panic("not implemented") +} + func (q *FakeQuerier) GetUserWorkspaceBuildParameters(_ context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 645357d6f095e..9e15081fa9c36 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1337,6 +1337,13 @@ func (m queryMetricsStore) GetUserNotificationPreferences(ctx context.Context, u return r0, r1 } +func (m queryMetricsStore) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { + start := time.Now() + r0, r1 := m.s.GetUserStatusCountsByDay(ctx, arg) + m.queryLatencies.WithLabelValues("GetUserStatusCountsByDay").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m queryMetricsStore) GetUserWorkspaceBuildParameters(ctx context.Context, ownerID database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { start := time.Now() r0, r1 := m.s.GetUserWorkspaceBuildParameters(ctx, ownerID) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 73a0e6d60af55..ca59e5c50b0bd 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2811,6 +2811,21 @@ func (mr *MockStoreMockRecorder) GetUserNotificationPreferences(arg0, arg1 any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserNotificationPreferences", reflect.TypeOf((*MockStore)(nil).GetUserNotificationPreferences), arg0, arg1) } +// GetUserStatusCountsByDay mocks base method. +func (m *MockStore) GetUserStatusCountsByDay(arg0 context.Context, arg1 database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserStatusCountsByDay", arg0, arg1) + ret0, _ := ret[0].([]database.GetUserStatusCountsByDayRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserStatusCountsByDay indicates an expected call of GetUserStatusCountsByDay. +func (mr *MockStoreMockRecorder) GetUserStatusCountsByDay(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserStatusCountsByDay", reflect.TypeOf((*MockStore)(nil).GetUserStatusCountsByDay), arg0, arg1) +} + // GetUserWorkspaceBuildParameters mocks base method. func (m *MockStore) GetUserWorkspaceBuildParameters(arg0 context.Context, arg1 database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 2128315ce6dad..cf98d56ea5cfb 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -285,6 +285,7 @@ type sqlcQuerier interface { GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUserLinksByUserID(ctx context.Context, userID uuid.UUID) ([]UserLink, error) GetUserNotificationPreferences(ctx context.Context, userID uuid.UUID) ([]NotificationPreference, error) + GetUserStatusCountsByDay(ctx context.Context, arg GetUserStatusCountsByDayParams) ([]GetUserStatusCountsByDayRow, error) GetUserWorkspaceBuildParameters(ctx context.Context, arg GetUserWorkspaceBuildParametersParams) ([]GetUserWorkspaceBuildParametersRow, error) // This will never return deleted users. GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUsersRow, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 1a7911bc64b4d..29e50c39a01f5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3094,6 +3094,109 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate return items, nil } +const getUserStatusCountsByDay = `-- name: GetUserStatusCountsByDay :many +WITH dates AS ( + -- Generate a series of dates between start and end + SELECT + day::date + FROM + generate_series( + date_trunc('day', $1::timestamptz), + date_trunc('day', $2::timestamptz), + '1 day'::interval + ) AS day +), +initial_statuses AS ( + -- Get the status of each user right before the start date + SELECT DISTINCT ON (user_id) + user_id, + new_status as status + FROM + user_status_changes + WHERE + changed_at < $1::timestamptz + ORDER BY + user_id, + changed_at DESC +), +relevant_changes AS ( + -- Get only the status changes within our date range + SELECT + date_trunc('day', changed_at)::date AS day, + user_id, + new_status as status + FROM + user_status_changes + WHERE + changed_at >= $1::timestamptz + AND changed_at <= $2::timestamptz +), +daily_status AS ( + -- Combine initial statuses with changes + SELECT + d.day, + COALESCE(rc.status, i.status) as status, + COALESCE(rc.user_id, i.user_id) as user_id + FROM + dates d + CROSS JOIN + initial_statuses i + LEFT JOIN + relevant_changes rc + ON + rc.day = d.day + AND rc.user_id = i.user_id +) +SELECT + day, + status, + COUNT(*) AS count +FROM + daily_status +WHERE + status IS NOT NULL +GROUP BY + day, + status +ORDER BY + day ASC, + status ASC +` + +type GetUserStatusCountsByDayParams struct { + StartTime time.Time `db:"start_time" json:"start_time"` + EndTime time.Time `db:"end_time" json:"end_time"` +} + +type GetUserStatusCountsByDayRow struct { + Day time.Time `db:"day" json:"day"` + Status UserStatus `db:"status" json:"status"` + Count int64 `db:"count" json:"count"` +} + +func (q *sqlQuerier) GetUserStatusCountsByDay(ctx context.Context, arg GetUserStatusCountsByDayParams) ([]GetUserStatusCountsByDayRow, error) { + rows, err := q.db.QueryContext(ctx, getUserStatusCountsByDay, arg.StartTime, arg.EndTime) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetUserStatusCountsByDayRow + for rows.Next() { + var i GetUserStatusCountsByDayRow + if err := rows.Scan(&i.Day, &i.Status, &i.Count); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const upsertTemplateUsageStats = `-- name: UpsertTemplateUsageStats :exec WITH latest_start AS ( diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index de107bc0e80c7..248edd89fac23 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -771,3 +771,71 @@ SELECT FROM unique_template_params utp JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name) GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value; + +-- name: GetUserStatusCountsByDay :many +WITH dates AS ( + -- Generate a series of dates between start and end + SELECT + day::date + FROM + generate_series( + date_trunc('day', @start_time::timestamptz), + date_trunc('day', @end_time::timestamptz), + '1 day'::interval + ) AS day +), +initial_statuses AS ( + -- Get the status of each user right before the start date + SELECT DISTINCT ON (user_id) + user_id, + new_status as status + FROM + user_status_changes + WHERE + changed_at < @start_time::timestamptz + ORDER BY + user_id, + changed_at DESC +), +relevant_changes AS ( + -- Get only the status changes within our date range + SELECT + date_trunc('day', changed_at)::date AS day, + user_id, + new_status as status + FROM + user_status_changes + WHERE + changed_at >= @start_time::timestamptz + AND changed_at <= @end_time::timestamptz +), +daily_status AS ( + -- Combine initial statuses with changes + SELECT + d.day, + COALESCE(rc.status, i.status) as status, + COALESCE(rc.user_id, i.user_id) as user_id + FROM + dates d + CROSS JOIN + initial_statuses i + LEFT JOIN + relevant_changes rc + ON + rc.day = d.day + AND rc.user_id = i.user_id +) +SELECT + day, + status, + COUNT(*) AS count +FROM + daily_status +WHERE + status IS NOT NULL +GROUP BY + day, + status +ORDER BY + day ASC, + status ASC; From 7cba28cb624183bf1054897ff6290eb1b7ef7a3d Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sat, 14 Dec 2024 10:39:31 +0000 Subject: [PATCH 03/23] rename unused variable --- coderd/database/dbmem/dbmem.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 306e9596b631f..75a32538c08f0 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5664,7 +5664,10 @@ func (q *FakeQuerier) GetUserNotificationPreferences(_ context.Context, userID u return out, nil } -func (q *FakeQuerier) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { +func (q *FakeQuerier) GetUserStatusCountsByDay(_ context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + err := validateDatabaseType(arg) if err != nil { return nil, err From 2d748c96b7ca882a0bada647dc6e800a6beed7e3 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 17 Dec 2024 08:09:28 +0000 Subject: [PATCH 04/23] Test GetUserStatusCountsByDay --- coderd/database/dump.sql | 10 +- .../000279_user_status_changes.up.sql | 13 +- coderd/database/querier_test.go | 446 ++++++++++++++++++ coderd/database/queries.sql.go | 59 ++- coderd/database/queries/insights.sql | 53 +-- 5 files changed, 510 insertions(+), 71 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 46d64568c46b5..972ff3b55210c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -421,13 +421,15 @@ CREATE FUNCTION record_user_status_change() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN - IF OLD.status IS DISTINCT FROM NEW.status THEN + IF TG_OP = 'INSERT' OR OLD.status IS DISTINCT FROM NEW.status THEN INSERT INTO user_status_changes ( user_id, - new_status + new_status, + changed_at ) VALUES ( NEW.id, - NEW.status + NEW.status, + NEW.updated_at ); END IF; RETURN NEW; @@ -2261,7 +2263,7 @@ CREATE TRIGGER trigger_upsert_user_links BEFORE INSERT OR UPDATE ON user_links F CREATE TRIGGER update_notification_message_dedupe_hash BEFORE INSERT OR UPDATE ON notification_messages FOR EACH ROW EXECUTE FUNCTION compute_notification_message_dedupe_hash(); -CREATE TRIGGER user_status_change_trigger BEFORE UPDATE ON users FOR EACH ROW EXECUTE FUNCTION record_user_status_change(); +CREATE TRIGGER user_status_change_trigger AFTER INSERT OR UPDATE ON users FOR EACH ROW EXECUTE FUNCTION record_user_status_change(); ALTER TABLE ONLY api_keys ADD CONSTRAINT api_keys_user_id_uuid_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000279_user_status_changes.up.sql b/coderd/database/migrations/000279_user_status_changes.up.sql index 545705aba50eb..f16164862b3e5 100644 --- a/coderd/database/migrations/000279_user_status_changes.up.sql +++ b/coderd/database/migrations/000279_user_status_changes.up.sql @@ -7,7 +7,6 @@ CREATE TABLE user_status_changes ( COMMENT ON TABLE user_status_changes IS 'Tracks the history of user status changes'; -CREATE INDEX idx_user_status_changes_user_id ON user_status_changes(user_id); CREATE INDEX idx_user_status_changes_changed_at ON user_status_changes(changed_at); INSERT INTO user_status_changes ( @@ -22,15 +21,17 @@ SELECT FROM users WHERE NOT deleted; -CREATE FUNCTION record_user_status_change() RETURNS trigger AS $$ +CREATE OR REPLACE FUNCTION record_user_status_change() RETURNS trigger AS $$ BEGIN - IF OLD.status IS DISTINCT FROM NEW.status THEN + IF TG_OP = 'INSERT' OR OLD.status IS DISTINCT FROM NEW.status THEN INSERT INTO user_status_changes ( user_id, - new_status + new_status, + changed_at ) VALUES ( NEW.id, - NEW.status + NEW.status, + NEW.updated_at ); END IF; RETURN NEW; @@ -38,6 +39,6 @@ END; $$ LANGUAGE plpgsql; CREATE TRIGGER user_status_change_trigger - BEFORE UPDATE ON users + AFTER INSERT OR UPDATE ON users FOR EACH ROW EXECUTE FUNCTION record_user_status_change(); diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 28d7108ae31ad..3da6499f3ec82 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2255,6 +2255,452 @@ func TestGroupRemovalTrigger(t *testing.T) { }, db2sdk.List(extraUserGroups, onlyGroupIDs)) } +func TestGetUserStatusCountsByDay(t *testing.T) { + t.Parallel() + + t.Run("No Users", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitShort) + + end := dbtime.Now() + start := end.Add(-30 * 24 * time.Hour) + + counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + StartTime: start, + EndTime: end, + }) + require.NoError(t, err) + require.Empty(t, counts, "should return no results when there are no users") + }) + + t.Run("Single User/Single State", func(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + status database.UserStatus + }{ + { + name: "Active Only", + status: database.UserStatusActive, + }, + { + name: "Dormant Only", + status: database.UserStatusDormant, + }, + { + name: "Suspended Only", + status: database.UserStatusSuspended, + }, + // { + // name: "Deleted Only", + // status: database.UserStatusDeleted, + // }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.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 + now := dbtime.Now() + createdAt := now.Add(-29 * 24 * time.Hour) + dbgen.User(t, db, database.User{ + Status: tc.status, + CreatedAt: createdAt, + UpdatedAt: createdAt, + }) + + // Query for the last 30 days + counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + StartTime: createdAt, + EndTime: now, + }) + require.NoError(t, err) + require.NotEmpty(t, counts, "should return results") + + // We should have an entry for each day, all with 1 user in the specified status + require.Len(t, counts, 30, "should have 30 days of data") + for _, count := range counts { + if count.Status.Valid && count.Status.UserStatus == tc.status { + require.Equal(t, int64(1), count.Count, "should have 1 %s user", tc.status) + } else { + require.Equal(t, int64(0), count.Count, "should have 0 users for other statuses") + } + } + }) + } + }) + + t.Run("Single Transition", func(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + initialStatus database.UserStatus + targetStatus database.UserStatus + }{ + { + name: "Active to Dormant", + initialStatus: database.UserStatusActive, + targetStatus: database.UserStatusDormant, + }, + { + name: "Active to Suspended", + initialStatus: database.UserStatusActive, + targetStatus: database.UserStatusSuspended, + }, + // { + // name: "Active to Deleted", + // initialStatus: database.UserStatusActive, + // targetStatus: database.UserStatusDeleted, + // }, + { + name: "Dormant to Active", + initialStatus: database.UserStatusDormant, + targetStatus: database.UserStatusActive, + }, + { + name: "Dormant to Suspended", + initialStatus: database.UserStatusDormant, + targetStatus: database.UserStatusSuspended, + }, + // { + // name: "Dormant to Deleted", + // initialStatus: database.UserStatusDormant, + // targetStatus: database.UserStatusDeleted, + // }, + { + name: "Suspended to Active", + initialStatus: database.UserStatusSuspended, + targetStatus: database.UserStatusActive, + }, + { + name: "Suspended to Dormant", + initialStatus: database.UserStatusSuspended, + targetStatus: database.UserStatusDormant, + }, + // { + // name: "Suspended to Deleted", + // initialStatus: database.UserStatusSuspended, + // targetStatus: database.UserStatusDeleted, + // }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.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 + now := dbtime.Now() + createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago + user := dbgen.User(t, db, database.User{ + Status: tc.initialStatus, + CreatedAt: createdAt, + UpdatedAt: createdAt, + }) + + // After 2 days, change status to target status + statusChangeTime := createdAt.Add(2 * 24 * time.Hour) + user, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: user.ID, + Status: tc.targetStatus, + UpdatedAt: statusChangeTime, + }) + require.NoError(t, err) + + // Query for the last 5 days + counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + StartTime: createdAt, + EndTime: now, + }) + require.NoError(t, err) + require.NotEmpty(t, counts, "should return results") + + // We should have entries for each day + require.Len(t, counts, 6, "should have 6 days of data") + + // Helper to get count for a specific day and status + getCount := func(day time.Time, status database.UserStatus) int64 { + day = day.Truncate(24 * time.Hour) + for _, c := range counts { + if c.Day.Truncate(24*time.Hour).Equal(day) && c.Status.Valid && c.Status.UserStatus == status { + return c.Count + } + } + return 0 + } + + // First 2 days should show initial status + require.Equal(t, int64(1), getCount(createdAt, tc.initialStatus), "day 1 should be %s", tc.initialStatus) + require.Equal(t, int64(1), getCount(createdAt.Add(24*time.Hour), tc.initialStatus), "day 2 should be %s", tc.initialStatus) + + // Remaining days should show target status + for i := 2; i < 6; i++ { + dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) + require.Equal(t, int64(1), getCount(dayTime, tc.targetStatus), + "day %d should be %s", i+1, tc.targetStatus) + require.Equal(t, int64(0), getCount(dayTime, tc.initialStatus), + "day %d should have no %s users", i+1, tc.initialStatus) + } + }) + } + }) + + t.Run("Two Users Transitioning", func(t *testing.T) { + t.Parallel() + + type transition struct { + from database.UserStatus + to database.UserStatus + } + + type testCase struct { + name string + user1Transition transition + user2Transition transition + expectedCounts map[string]map[database.UserStatus]int64 + } + + testCases := []testCase{ + { + name: "Active->Dormant and Dormant->Suspended", + user1Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusDormant, + }, + user2Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusSuspended, + }, + expectedCounts: map[string]map[database.UserStatus]int64{ + "initial": { + database.UserStatusActive: 1, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + "between": { + database.UserStatusActive: 0, + database.UserStatusDormant: 2, + database.UserStatusSuspended: 0, + }, + "final": { + database.UserStatusActive: 0, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 1, + }, + }, + }, + { + name: "Suspended->Active and Active->Dormant", + user1Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusActive, + }, + user2Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusDormant, + }, + expectedCounts: map[string]map[database.UserStatus]int64{ + "initial": { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 1, + }, + "between": { + database.UserStatusActive: 2, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 0, + }, + "final": { + database.UserStatusActive: 1, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + }, + }, + { + name: "Dormant->Active and Suspended->Dormant", + user1Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusActive, + }, + user2Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusDormant, + }, + expectedCounts: map[string]map[database.UserStatus]int64{ + "initial": { + database.UserStatusActive: 0, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 1, + }, + "between": { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 1, + }, + "final": { + database.UserStatusActive: 1, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + }, + }, + { + name: "Active->Suspended and Suspended->Active", + user1Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusSuspended, + }, + user2Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusActive, + }, + expectedCounts: map[string]map[database.UserStatus]int64{ + "initial": { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 1, + }, + "between": { + database.UserStatusActive: 0, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 2, + }, + "final": { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 1, + }, + }, + }, + { + name: "Dormant->Suspended and Dormant->Active", + user1Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusSuspended, + }, + user2Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusActive, + }, + expectedCounts: map[string]map[database.UserStatus]int64{ + "initial": { + database.UserStatusActive: 0, + database.UserStatusDormant: 2, + database.UserStatusSuspended: 0, + }, + "between": { + database.UserStatusActive: 0, + database.UserStatusDormant: 1, + database.UserStatusSuspended: 1, + }, + "final": { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + database.UserStatusSuspended: 1, + }, + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitShort) + + now := dbtime.Now() + createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago + + user1 := dbgen.User(t, db, database.User{ + Status: tc.user1Transition.from, + CreatedAt: createdAt, + UpdatedAt: createdAt, + }) + user2 := dbgen.User(t, db, database.User{ + Status: tc.user2Transition.from, + CreatedAt: createdAt, + UpdatedAt: createdAt, + }) + + // First transition at 2 days + user1TransitionTime := createdAt.Add(2 * 24 * time.Hour) + user1, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: user1.ID, + Status: tc.user1Transition.to, + UpdatedAt: user1TransitionTime, + }) + require.NoError(t, err) + + // Second transition at 4 days + user2TransitionTime := createdAt.Add(4 * 24 * time.Hour) + user2, err = db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: user2.ID, + Status: tc.user2Transition.to, + UpdatedAt: user2TransitionTime, + }) + require.NoError(t, err) + + // Query for the last 5 days + counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + StartTime: createdAt, + EndTime: now, + }) + require.NoError(t, err) + require.NotEmpty(t, counts) + + // Helper to get count for a specific day and status + getCount := func(day time.Time, status database.UserStatus) int64 { + day = day.Truncate(24 * time.Hour) + for _, c := range counts { + if c.Day.Truncate(24*time.Hour).Equal(day) && c.Status.Valid && c.Status.UserStatus == status { + return c.Count + } + } + return 0 + } + + // Check initial period (days 0-1) + for i := 0; i < 2; i++ { + dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) + for status, expectedCount := range tc.expectedCounts["initial"] { + require.Equal(t, expectedCount, getCount(dayTime, status), + "day %d should have %d %s users", i+1, expectedCount, status) + } + } + + // Check between transitions (days 2-3) + for i := 2; i < 4; i++ { + dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) + for status, expectedCount := range tc.expectedCounts["between"] { + require.Equal(t, expectedCount, getCount(dayTime, status), + "day %d should have %d %s users", i+1, expectedCount, status) + } + } + + // Check final period (days 4-5) + for i := 4; i < 6; i++ { + dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) + for status, expectedCount := range tc.expectedCounts["final"] { + require.Equal(t, expectedCount, getCount(dayTime, status), + "day %d should have %d %s users", i+1, expectedCount, status) + } + } + }) + } + }) +} + func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) { t.Helper() require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 29e50c39a01f5..55cd32fe810d0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3106,46 +3106,41 @@ WITH dates AS ( '1 day'::interval ) AS day ), -initial_statuses AS ( - -- Get the status of each user right before the start date - SELECT DISTINCT ON (user_id) - user_id, - new_status as status - FROM - user_status_changes - WHERE - changed_at < $1::timestamptz - ORDER BY - user_id, - changed_at DESC +all_users AS ( + -- Get all users who have had any status changes in or before the range + SELECT DISTINCT user_id + FROM user_status_changes + WHERE changed_at <= $2::timestamptz ), -relevant_changes AS ( - -- Get only the status changes within our date range - SELECT - date_trunc('day', changed_at)::date AS day, +initial_statuses AS ( + -- Get the status of each user right before each day + SELECT DISTINCT ON (user_id, day) user_id, + day, new_status as status FROM - user_status_changes - WHERE - changed_at >= $1::timestamptz - AND changed_at <= $2::timestamptz + all_users + CROSS JOIN + dates + LEFT JOIN LATERAL ( + SELECT new_status, changed_at + FROM user_status_changes + WHERE user_status_changes.user_id = all_users.user_id + AND changed_at < day + interval '1 day' + ORDER BY changed_at DESC + LIMIT 1 + ) changes ON true + WHERE changes.new_status IS NOT NULL ), daily_status AS ( - -- Combine initial statuses with changes SELECT d.day, - COALESCE(rc.status, i.status) as status, - COALESCE(rc.user_id, i.user_id) as user_id + i.status, + i.user_id FROM dates d - CROSS JOIN - initial_statuses i LEFT JOIN - relevant_changes rc - ON - rc.day = d.day - AND rc.user_id = i.user_id + initial_statuses i ON i.day = d.day ) SELECT day, @@ -3169,9 +3164,9 @@ type GetUserStatusCountsByDayParams struct { } type GetUserStatusCountsByDayRow struct { - Day time.Time `db:"day" json:"day"` - Status UserStatus `db:"status" json:"status"` - Count int64 `db:"count" json:"count"` + Day time.Time `db:"day" json:"day"` + Status NullUserStatus `db:"status" json:"status"` + Count int64 `db:"count" json:"count"` } func (q *sqlQuerier) GetUserStatusCountsByDay(ctx context.Context, arg GetUserStatusCountsByDayParams) ([]GetUserStatusCountsByDayRow, error) { diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 248edd89fac23..1edb8666e8f1e 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -784,46 +784,41 @@ WITH dates AS ( '1 day'::interval ) AS day ), -initial_statuses AS ( - -- Get the status of each user right before the start date - SELECT DISTINCT ON (user_id) - user_id, - new_status as status - FROM - user_status_changes - WHERE - changed_at < @start_time::timestamptz - ORDER BY - user_id, - changed_at DESC +all_users AS ( + -- Get all users who have had any status changes in or before the range + SELECT DISTINCT user_id + FROM user_status_changes + WHERE changed_at <= @end_time::timestamptz ), -relevant_changes AS ( - -- Get only the status changes within our date range - SELECT - date_trunc('day', changed_at)::date AS day, +initial_statuses AS ( + -- Get the status of each user right before each day + SELECT DISTINCT ON (user_id, day) user_id, + day, new_status as status FROM - user_status_changes - WHERE - changed_at >= @start_time::timestamptz - AND changed_at <= @end_time::timestamptz + all_users + CROSS JOIN + dates + LEFT JOIN LATERAL ( + SELECT new_status, changed_at + FROM user_status_changes + WHERE user_status_changes.user_id = all_users.user_id + AND changed_at < day + interval '1 day' + ORDER BY changed_at DESC + LIMIT 1 + ) changes ON true + WHERE changes.new_status IS NOT NULL ), daily_status AS ( - -- Combine initial statuses with changes SELECT d.day, - COALESCE(rc.status, i.status) as status, - COALESCE(rc.user_id, i.user_id) as user_id + i.status, + i.user_id FROM dates d - CROSS JOIN - initial_statuses i LEFT JOIN - relevant_changes rc - ON - rc.day = d.day - AND rc.user_id = i.user_id + initial_statuses i ON i.day = d.day ) SELECT day, From 52620ce3268a22ac97a6436db59cbba5bcd09d32 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 17 Dec 2024 08:14:35 +0000 Subject: [PATCH 05/23] make gen --- coderd/database/dump.sql | 2 -- coderd/database/migrations/000279_user_status_changes.down.sql | 1 - 2 files changed, 3 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 972ff3b55210c..404c3db9d605a 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -2119,8 +2119,6 @@ CREATE INDEX idx_tailnet_tunnels_src_id ON tailnet_tunnels USING hash (src_id); CREATE INDEX idx_user_status_changes_changed_at ON user_status_changes USING btree (changed_at); -CREATE INDEX idx_user_status_changes_user_id ON user_status_changes USING btree (user_id); - CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted = false); CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); diff --git a/coderd/database/migrations/000279_user_status_changes.down.sql b/coderd/database/migrations/000279_user_status_changes.down.sql index b75acbcafca46..30514ef467ebe 100644 --- a/coderd/database/migrations/000279_user_status_changes.down.sql +++ b/coderd/database/migrations/000279_user_status_changes.down.sql @@ -6,7 +6,6 @@ DROP FUNCTION IF EXISTS record_user_status_change(); -- Drop the indexes DROP INDEX IF EXISTS idx_user_status_changes_changed_at; -DROP INDEX IF EXISTS idx_user_status_changes_user_id; -- Drop the table DROP TABLE IF EXISTS user_status_changes; From 6d15a324b4b6d15b6743b8fdaa8af7941862a3ca Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 17 Dec 2024 11:37:35 +0000 Subject: [PATCH 06/23] fix dbauthz tests --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/database/dbauthz/dbauthz_test.go | 6 +++++ coderd/database/dbmem/dbmem.go | 36 ++++++++++++++++++++++++- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index ffc1eb63d8dd0..c6853a7af969a 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2414,7 +2414,7 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui } func (q *querier) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceUser); err != nil { return nil, err } return q.db.GetUserStatusCountsByDay(ctx, arg) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 961f5d535b280..7db81a6b02ede 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1490,6 +1490,12 @@ func (s *MethodTestSuite) TestUser() { rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead, ) })) + s.Run("GetUserStatusCountsByDay", s.Subtest(func(db database.Store, check *expects) { + check.Args(database.GetUserStatusCountsByDayParams{ + StartTime: time.Now().Add(-time.Hour * 24 * 30), + EndTime: time.Now(), + }).Asserts(rbac.ResourceUser, policy.ActionRead) + })) } func (s *MethodTestSuite) TestWorkspace() { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 75a32538c08f0..7ae4b367fbc73 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -88,6 +88,7 @@ func New() database.Store { customRoles: make([]database.CustomRole, 0), locks: map[int64]struct{}{}, runtimeConfig: map[string]string{}, + userStatusChanges: make([]database.UserStatusChange, 0), }, } // Always start with a default org. Matching migration 198. @@ -256,6 +257,7 @@ type data struct { lastLicenseID int32 defaultProxyDisplayName string defaultProxyIconURL string + userStatusChanges []database.UserStatusChange } func tryPercentile(fs []float64, p float64) float64 { @@ -5673,7 +5675,21 @@ func (q *FakeQuerier) GetUserStatusCountsByDay(_ context.Context, arg database.G return nil, err } - panic("not implemented") + result := make([]database.GetUserStatusCountsByDayRow, 0) + for _, change := range q.userStatusChanges { + if change.ChangedAt.Before(arg.StartTime) || change.ChangedAt.After(arg.EndTime) { + continue + } + result = append(result, database.GetUserStatusCountsByDayRow{ + Status: database.NullUserStatus{ + UserStatus: change.NewStatus, + Valid: true, + }, + Count: 1, + }) + } + + return result, nil } func (q *FakeQuerier) GetUserWorkspaceBuildParameters(_ context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { @@ -8028,6 +8044,12 @@ func (q *FakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam sort.Slice(q.users, func(i, j int) bool { return q.users[i].CreatedAt.Before(q.users[j].CreatedAt) }) + + q.userStatusChanges = append(q.userStatusChanges, database.UserStatusChange{ + UserID: user.ID, + NewStatus: user.Status, + ChangedAt: user.UpdatedAt, + }) return user, nil } @@ -9064,12 +9086,18 @@ func (q *FakeQuerier) UpdateInactiveUsersToDormant(_ context.Context, params dat Username: user.Username, LastSeenAt: user.LastSeenAt, }) + q.userStatusChanges = append(q.userStatusChanges, database.UserStatusChange{ + UserID: user.ID, + NewStatus: database.UserStatusDormant, + ChangedAt: params.UpdatedAt, + }) } } if len(updated) == 0 { return nil, sql.ErrNoRows } + return updated, nil } @@ -9870,6 +9898,12 @@ func (q *FakeQuerier) UpdateUserStatus(_ context.Context, arg database.UpdateUse user.Status = arg.Status user.UpdatedAt = arg.UpdatedAt q.users[index] = user + + q.userStatusChanges = append(q.userStatusChanges, database.UserStatusChange{ + UserID: user.ID, + NewStatus: user.Status, + ChangedAt: user.UpdatedAt, + }) return user, nil } return database.User{}, sql.ErrNoRows From b99468ff83f27f7d0d4974792df08cc6d4d1ad1e Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 23 Dec 2024 08:57:32 +0000 Subject: [PATCH 07/23] do the plumbing to get sql, api and frontend talking to one another --- coderd/apidoc/docs.go | 61 ++++++ coderd/apidoc/swagger.json | 57 ++++++ coderd/coderd.go | 1 + coderd/database/dbauthz/dbauthz.go | 4 +- coderd/database/dbauthz/dbauthz_test.go | 4 +- coderd/database/dbmem/dbmem.go | 12 +- coderd/database/dbmetrics/querymetrics.go | 6 +- coderd/database/dbmock/dbmock.go | 14 +- coderd/database/querier.go | 2 +- coderd/database/querier_test.go | 109 +++-------- coderd/database/queries.sql.go | 104 +++-------- coderd/database/queries/insights.sql | 67 +------ coderd/insights.go | 67 +++++++ codersdk/insights.go | 31 ++++ docs/reference/api/insights.md | 50 +++++ docs/reference/api/schemas.md | 44 +++++ site/src/api/api.ts | 13 ++ site/src/api/queries/insights.ts | 7 + site/src/api/queries/users.ts | 1 - site/src/api/typesGenerated.ts | 16 ++ .../ActiveUserChart.stories.tsx | 69 ++++++- .../ActiveUserChart/ActiveUserChart.tsx | 121 +++++++++--- .../GeneralSettingsPage.tsx | 7 +- .../GeneralSettingsPageView.stories.tsx | 174 +++++++++++------- .../GeneralSettingsPageView.tsx | 82 ++++----- .../TemplateInsightsPage.tsx | 12 +- 26 files changed, 740 insertions(+), 395 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 5780443a42de1..a5d33d0797887 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -1398,6 +1398,40 @@ const docTemplate = `{ } } }, + "/insights/user-status-counts-over-time": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": [ + "application/json" + ], + "tags": [ + "Insights" + ], + "summary": "Get insights about user status counts over time", + "operationId": "get-insights-about-user-status-counts-over-time", + "parameters": [ + { + "type": "integer", + "description": "Time-zone offset (e.g. -2)", + "name": "tz_offset", + "in": "query", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/codersdk.GetUserStatusChangesResponse" + } + } + } + } + }, "/integrations/jfrog/xray-scan": { "get": { "security": [ @@ -11115,6 +11149,20 @@ const docTemplate = `{ } } }, + "codersdk.GetUserStatusChangesResponse": { + "type": "object", + "properties": { + "status_counts": { + "type": "object", + "additionalProperties": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.UserStatusChangeCount" + } + } + } + } + }, "codersdk.GetUsersResponse": { "type": "object", "properties": { @@ -14478,6 +14526,19 @@ const docTemplate = `{ "UserStatusSuspended" ] }, + "codersdk.UserStatusChangeCount": { + "type": "object", + "properties": { + "count": { + "type": "integer", + "example": 10 + }, + "date": { + "type": "string", + "format": "date-time" + } + } + }, "codersdk.ValidateUserPasswordRequest": { "type": "object", "required": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 1ecb6d185e03c..bc65cf556e7f7 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -1219,6 +1219,36 @@ } } }, + "/insights/user-status-counts-over-time": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": ["application/json"], + "tags": ["Insights"], + "summary": "Get insights about user status counts over time", + "operationId": "get-insights-about-user-status-counts-over-time", + "parameters": [ + { + "type": "integer", + "description": "Time-zone offset (e.g. -2)", + "name": "tz_offset", + "in": "query", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/codersdk.GetUserStatusChangesResponse" + } + } + } + } + }, "/integrations/jfrog/xray-scan": { "get": { "security": [ @@ -9970,6 +10000,20 @@ } } }, + "codersdk.GetUserStatusChangesResponse": { + "type": "object", + "properties": { + "status_counts": { + "type": "object", + "additionalProperties": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.UserStatusChangeCount" + } + } + } + } + }, "codersdk.GetUsersResponse": { "type": "object", "properties": { @@ -13160,6 +13204,19 @@ "UserStatusSuspended" ] }, + "codersdk.UserStatusChangeCount": { + "type": "object", + "properties": { + "count": { + "type": "integer", + "example": 10 + }, + "date": { + "type": "string", + "format": "date-time" + } + } + }, "codersdk.ValidateUserPasswordRequest": { "type": "object", "required": ["password"], diff --git a/coderd/coderd.go b/coderd/coderd.go index fd8a10a44f140..c197c08fd5cd9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1281,6 +1281,7 @@ func New(options *Options) *API { r.Use(apiKeyMiddleware) r.Get("/daus", api.deploymentDAUs) r.Get("/user-activity", api.insightsUserActivity) + r.Get("/user-status-counts-over-time", api.insightsUserStatusCountsOverTime) r.Get("/user-latency", api.insightsUserLatency) r.Get("/templates", api.insightsTemplates) }) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index c6853a7af969a..cbae48e83c8ba 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2413,11 +2413,11 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui return q.db.GetUserNotificationPreferences(ctx, userID) } -func (q *querier) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { +func (q *querier) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceUser); err != nil { return nil, err } - return q.db.GetUserStatusCountsByDay(ctx, arg) + return q.db.GetUserStatusChanges(ctx, arg) } func (q *querier) GetUserWorkspaceBuildParameters(ctx context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 7db81a6b02ede..e1258a8eed087 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1490,8 +1490,8 @@ func (s *MethodTestSuite) TestUser() { rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead, ) })) - s.Run("GetUserStatusCountsByDay", s.Subtest(func(db database.Store, check *expects) { - check.Args(database.GetUserStatusCountsByDayParams{ + s.Run("GetUserStatusChanges", s.Subtest(func(db database.Store, check *expects) { + check.Args(database.GetUserStatusChangesParams{ StartTime: time.Now().Add(-time.Hour * 24 * 30), EndTime: time.Now(), }).Asserts(rbac.ResourceUser, policy.ActionRead) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 7ae4b367fbc73..08b4d66724f41 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5666,7 +5666,7 @@ func (q *FakeQuerier) GetUserNotificationPreferences(_ context.Context, userID u return out, nil } -func (q *FakeQuerier) GetUserStatusCountsByDay(_ context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { +func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -5675,18 +5675,12 @@ func (q *FakeQuerier) GetUserStatusCountsByDay(_ context.Context, arg database.G return nil, err } - result := make([]database.GetUserStatusCountsByDayRow, 0) + result := make([]database.UserStatusChange, 0) for _, change := range q.userStatusChanges { if change.ChangedAt.Before(arg.StartTime) || change.ChangedAt.After(arg.EndTime) { continue } - result = append(result, database.GetUserStatusCountsByDayRow{ - Status: database.NullUserStatus{ - UserStatus: change.NewStatus, - Valid: true, - }, - Count: 1, - }) + result = append(result, change) } return result, nil diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 9e15081fa9c36..989a18a8856f4 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1337,10 +1337,10 @@ func (m queryMetricsStore) GetUserNotificationPreferences(ctx context.Context, u return r0, r1 } -func (m queryMetricsStore) GetUserStatusCountsByDay(ctx context.Context, arg database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { +func (m queryMetricsStore) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { start := time.Now() - r0, r1 := m.s.GetUserStatusCountsByDay(ctx, arg) - m.queryLatencies.WithLabelValues("GetUserStatusCountsByDay").Observe(time.Since(start).Seconds()) + r0, r1 := m.s.GetUserStatusChanges(ctx, arg) + m.queryLatencies.WithLabelValues("GetUserStatusChanges").Observe(time.Since(start).Seconds()) return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index ca59e5c50b0bd..a8cb8effc3214 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2811,19 +2811,19 @@ func (mr *MockStoreMockRecorder) GetUserNotificationPreferences(arg0, arg1 any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserNotificationPreferences", reflect.TypeOf((*MockStore)(nil).GetUserNotificationPreferences), arg0, arg1) } -// GetUserStatusCountsByDay mocks base method. -func (m *MockStore) GetUserStatusCountsByDay(arg0 context.Context, arg1 database.GetUserStatusCountsByDayParams) ([]database.GetUserStatusCountsByDayRow, error) { +// GetUserStatusChanges mocks base method. +func (m *MockStore) GetUserStatusChanges(arg0 context.Context, arg1 database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserStatusCountsByDay", arg0, arg1) - ret0, _ := ret[0].([]database.GetUserStatusCountsByDayRow) + ret := m.ctrl.Call(m, "GetUserStatusChanges", arg0, arg1) + ret0, _ := ret[0].([]database.UserStatusChange) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetUserStatusCountsByDay indicates an expected call of GetUserStatusCountsByDay. -func (mr *MockStoreMockRecorder) GetUserStatusCountsByDay(arg0, arg1 any) *gomock.Call { +// GetUserStatusChanges indicates an expected call of GetUserStatusChanges. +func (mr *MockStoreMockRecorder) GetUserStatusChanges(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserStatusCountsByDay", reflect.TypeOf((*MockStore)(nil).GetUserStatusCountsByDay), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserStatusChanges", reflect.TypeOf((*MockStore)(nil).GetUserStatusChanges), arg0, arg1) } // GetUserWorkspaceBuildParameters mocks base method. diff --git a/coderd/database/querier.go b/coderd/database/querier.go index cf98d56ea5cfb..248fd7bec39a7 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -285,7 +285,7 @@ type sqlcQuerier interface { GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUserLinksByUserID(ctx context.Context, userID uuid.UUID) ([]UserLink, error) GetUserNotificationPreferences(ctx context.Context, userID uuid.UUID) ([]NotificationPreference, error) - GetUserStatusCountsByDay(ctx context.Context, arg GetUserStatusCountsByDayParams) ([]GetUserStatusCountsByDayRow, error) + GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]UserStatusChange, error) GetUserWorkspaceBuildParameters(ctx context.Context, arg GetUserWorkspaceBuildParametersParams) ([]GetUserWorkspaceBuildParametersRow, error) // This will never return deleted users. GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUsersRow, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 3da6499f3ec82..c724789df58ac 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2255,7 +2255,7 @@ func TestGroupRemovalTrigger(t *testing.T) { }, db2sdk.List(extraUserGroups, onlyGroupIDs)) } -func TestGetUserStatusCountsByDay(t *testing.T) { +func TestGetUserStatusChanges(t *testing.T) { t.Parallel() t.Run("No Users", func(t *testing.T) { @@ -2266,7 +2266,7 @@ func TestGetUserStatusCountsByDay(t *testing.T) { end := dbtime.Now() start := end.Add(-30 * 24 * time.Hour) - counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + counts, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ StartTime: start, EndTime: end, }) @@ -2316,22 +2316,16 @@ func TestGetUserStatusCountsByDay(t *testing.T) { }) // Query for the last 30 days - counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ StartTime: createdAt, EndTime: now, }) require.NoError(t, err) - require.NotEmpty(t, counts, "should return results") - - // We should have an entry for each day, all with 1 user in the specified status - require.Len(t, counts, 30, "should have 30 days of data") - for _, count := range counts { - if count.Status.Valid && count.Status.UserStatus == tc.status { - require.Equal(t, int64(1), count.Count, "should have 1 %s user", tc.status) - } else { - require.Equal(t, int64(0), count.Count, "should have 0 users for other statuses") - } - } + require.NotEmpty(t, userStatusChanges, "should return results") + + // We should have an entry for each status change + require.Len(t, userStatusChanges, 1, "should have 1 status change") + require.Equal(t, userStatusChanges[0].NewStatus, tc.status, "should have the correct status") }) } }) @@ -2417,39 +2411,17 @@ func TestGetUserStatusCountsByDay(t *testing.T) { require.NoError(t, err) // Query for the last 5 days - counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ StartTime: createdAt, EndTime: now, }) require.NoError(t, err) - require.NotEmpty(t, counts, "should return results") - - // We should have entries for each day - require.Len(t, counts, 6, "should have 6 days of data") - - // Helper to get count for a specific day and status - getCount := func(day time.Time, status database.UserStatus) int64 { - day = day.Truncate(24 * time.Hour) - for _, c := range counts { - if c.Day.Truncate(24*time.Hour).Equal(day) && c.Status.Valid && c.Status.UserStatus == status { - return c.Count - } - } - return 0 - } + require.NotEmpty(t, userStatusChanges, "should return results") - // First 2 days should show initial status - require.Equal(t, int64(1), getCount(createdAt, tc.initialStatus), "day 1 should be %s", tc.initialStatus) - require.Equal(t, int64(1), getCount(createdAt.Add(24*time.Hour), tc.initialStatus), "day 2 should be %s", tc.initialStatus) - - // Remaining days should show target status - for i := 2; i < 6; i++ { - dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) - require.Equal(t, int64(1), getCount(dayTime, tc.targetStatus), - "day %d should be %s", i+1, tc.targetStatus) - require.Equal(t, int64(0), getCount(dayTime, tc.initialStatus), - "day %d should have no %s users", i+1, tc.initialStatus) - } + // We should have an entry for each status change, including the initial status + require.Len(t, userStatusChanges, 2, "should have 2 status changes") + require.Equal(t, userStatusChanges[0].NewStatus, tc.initialStatus, "should have the initial status") + require.Equal(t, userStatusChanges[1].NewStatus, tc.targetStatus, "should have the target status") }) } }) @@ -2652,50 +2624,23 @@ func TestGetUserStatusCountsByDay(t *testing.T) { require.NoError(t, err) // Query for the last 5 days - counts, err := db.GetUserStatusCountsByDay(ctx, database.GetUserStatusCountsByDayParams{ + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ StartTime: createdAt, EndTime: now, }) require.NoError(t, err) - require.NotEmpty(t, counts) - - // Helper to get count for a specific day and status - getCount := func(day time.Time, status database.UserStatus) int64 { - day = day.Truncate(24 * time.Hour) - for _, c := range counts { - if c.Day.Truncate(24*time.Hour).Equal(day) && c.Status.Valid && c.Status.UserStatus == status { - return c.Count - } - } - return 0 - } - - // Check initial period (days 0-1) - for i := 0; i < 2; i++ { - dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) - for status, expectedCount := range tc.expectedCounts["initial"] { - require.Equal(t, expectedCount, getCount(dayTime, status), - "day %d should have %d %s users", i+1, expectedCount, status) - } - } - - // Check between transitions (days 2-3) - for i := 2; i < 4; i++ { - dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) - for status, expectedCount := range tc.expectedCounts["between"] { - require.Equal(t, expectedCount, getCount(dayTime, status), - "day %d should have %d %s users", i+1, expectedCount, status) - } - } - - // Check final period (days 4-5) - for i := 4; i < 6; i++ { - dayTime := createdAt.Add(time.Duration(i) * 24 * time.Hour) - for status, expectedCount := range tc.expectedCounts["final"] { - require.Equal(t, expectedCount, getCount(dayTime, status), - "day %d should have %d %s users", i+1, expectedCount, status) - } - } + require.NotEmpty(t, userStatusChanges) + + // We should have an entry with the correct status changes for each user, including the initial status + require.Len(t, userStatusChanges, 4, "should have 4 status changes") + require.Equal(t, userStatusChanges[0].UserID, user1.ID, "should have the first user") + require.Equal(t, userStatusChanges[0].NewStatus, tc.user1Transition.from, "should have the first user's initial status") + require.Equal(t, userStatusChanges[1].UserID, user1.ID, "should have the first user") + require.Equal(t, userStatusChanges[1].NewStatus, tc.user1Transition.to, "should have the first user's target status") + require.Equal(t, userStatusChanges[2].UserID, user2.ID, "should have the second user") + require.Equal(t, userStatusChanges[2].NewStatus, tc.user2Transition.from, "should have the second user's initial status") + require.Equal(t, userStatusChanges[3].UserID, user2.ID, "should have the second user") + require.Equal(t, userStatusChanges[3].NewStatus, tc.user2Transition.to, "should have the second user's target status") }) } }) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 55cd32fe810d0..5d870802c27fc 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3094,91 +3094,35 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate return items, nil } -const getUserStatusCountsByDay = `-- name: GetUserStatusCountsByDay :many -WITH dates AS ( - -- Generate a series of dates between start and end - SELECT - day::date - FROM - generate_series( - date_trunc('day', $1::timestamptz), - date_trunc('day', $2::timestamptz), - '1 day'::interval - ) AS day -), -all_users AS ( - -- Get all users who have had any status changes in or before the range - SELECT DISTINCT user_id - FROM user_status_changes - WHERE changed_at <= $2::timestamptz -), -initial_statuses AS ( - -- Get the status of each user right before each day - SELECT DISTINCT ON (user_id, day) - user_id, - day, - new_status as status - FROM - all_users - CROSS JOIN - dates - LEFT JOIN LATERAL ( - SELECT new_status, changed_at - FROM user_status_changes - WHERE user_status_changes.user_id = all_users.user_id - AND changed_at < day + interval '1 day' - ORDER BY changed_at DESC - LIMIT 1 - ) changes ON true - WHERE changes.new_status IS NOT NULL -), -daily_status AS ( - SELECT - d.day, - i.status, - i.user_id - FROM - dates d - LEFT JOIN - initial_statuses i ON i.day = d.day -) +const GetUserStatusChanges = `-- name: GetUserStatusChanges :many SELECT - day, - status, - COUNT(*) AS count -FROM - daily_status -WHERE - status IS NOT NULL -GROUP BY - day, - status -ORDER BY - day ASC, - status ASC + id, user_id, new_status, changed_at +FROM user_status_changes +WHERE changed_at >= $1::timestamptz + AND changed_at < $2::timestamptz +ORDER BY changed_at ` -type GetUserStatusCountsByDayParams struct { +type GetUserStatusChangesParams struct { StartTime time.Time `db:"start_time" json:"start_time"` EndTime time.Time `db:"end_time" json:"end_time"` } -type GetUserStatusCountsByDayRow struct { - Day time.Time `db:"day" json:"day"` - Status NullUserStatus `db:"status" json:"status"` - Count int64 `db:"count" json:"count"` -} - -func (q *sqlQuerier) GetUserStatusCountsByDay(ctx context.Context, arg GetUserStatusCountsByDayParams) ([]GetUserStatusCountsByDayRow, error) { - rows, err := q.db.QueryContext(ctx, getUserStatusCountsByDay, arg.StartTime, arg.EndTime) +func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]UserStatusChange, error) { + rows, err := q.db.QueryContext(ctx, GetUserStatusChanges, arg.StartTime, arg.EndTime) if err != nil { return nil, err } defer rows.Close() - var items []GetUserStatusCountsByDayRow + var items []UserStatusChange for rows.Next() { - var i GetUserStatusCountsByDayRow - if err := rows.Scan(&i.Day, &i.Status, &i.Count); err != nil { + var i UserStatusChange + if err := rows.Scan( + &i.ID, + &i.UserID, + &i.NewStatus, + &i.ChangedAt, + ); err != nil { return nil, err } items = append(items, i) @@ -3488,7 +3432,7 @@ func (q *sqlQuerier) GetJFrogXrayScanByWorkspaceAndAgentID(ctx context.Context, } const upsertJFrogXrayScanByWorkspaceAndAgentID = `-- name: UpsertJFrogXrayScanByWorkspaceAndAgentID :exec -INSERT INTO +INSERT INTO jfrog_xray_scans ( agent_id, workspace_id, @@ -3497,7 +3441,7 @@ INSERT INTO medium, results_url ) -VALUES +VALUES ($1, $2, $3, $4, $5, $6) ON CONFLICT (agent_id, workspace_id) DO UPDATE SET critical = $3, high = $4, medium = $5, results_url = $6 @@ -6372,7 +6316,7 @@ FROM provisioner_keys WHERE organization_id = $1 -AND +AND lower(name) = lower($2) ` @@ -6488,10 +6432,10 @@ WHERE AND -- exclude reserved built-in key id != '00000000-0000-0000-0000-000000000001'::uuid -AND +AND -- exclude reserved user-auth key id != '00000000-0000-0000-0000-000000000002'::uuid -AND +AND -- exclude reserved psk key id != '00000000-0000-0000-0000-000000000003'::uuid ` @@ -8177,7 +8121,7 @@ func (q *sqlQuerier) GetTailnetTunnelPeerIDs(ctx context.Context, srcID uuid.UUI } const updateTailnetPeerStatusByCoordinator = `-- name: UpdateTailnetPeerStatusByCoordinator :exec -UPDATE +UPDATE tailnet_peers SET status = $2 @@ -12763,7 +12707,7 @@ WITH agent_stats AS ( coalesce((PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY connection_median_latency_ms)), -1)::FLOAT AS workspace_connection_latency_95 FROM workspace_agent_stats -- The greater than 0 is to support legacy agents that don't report connection_median_latency_ms. - WHERE workspace_agent_stats.created_at > $1 AND connection_median_latency_ms > 0 + WHERE workspace_agent_stats.created_at > $1 AND connection_median_latency_ms > 0 GROUP BY user_id, agent_id, workspace_id, template_id ), latest_agent_stats AS ( SELECT diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 1edb8666e8f1e..dfa236dfbd6d4 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -772,65 +772,10 @@ FROM unique_template_params utp JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name) GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value; --- name: GetUserStatusCountsByDay :many -WITH dates AS ( - -- Generate a series of dates between start and end - SELECT - day::date - FROM - generate_series( - date_trunc('day', @start_time::timestamptz), - date_trunc('day', @end_time::timestamptz), - '1 day'::interval - ) AS day -), -all_users AS ( - -- Get all users who have had any status changes in or before the range - SELECT DISTINCT user_id - FROM user_status_changes - WHERE changed_at <= @end_time::timestamptz -), -initial_statuses AS ( - -- Get the status of each user right before each day - SELECT DISTINCT ON (user_id, day) - user_id, - day, - new_status as status - FROM - all_users - CROSS JOIN - dates - LEFT JOIN LATERAL ( - SELECT new_status, changed_at - FROM user_status_changes - WHERE user_status_changes.user_id = all_users.user_id - AND changed_at < day + interval '1 day' - ORDER BY changed_at DESC - LIMIT 1 - ) changes ON true - WHERE changes.new_status IS NOT NULL -), -daily_status AS ( - SELECT - d.day, - i.status, - i.user_id - FROM - dates d - LEFT JOIN - initial_statuses i ON i.day = d.day -) +-- name: GetUserStatusChanges :many SELECT - day, - status, - COUNT(*) AS count -FROM - daily_status -WHERE - status IS NOT NULL -GROUP BY - day, - status -ORDER BY - day ASC, - status ASC; + * +FROM user_status_changes +WHERE changed_at >= @start_time::timestamptz + AND changed_at < @end_time::timestamptz +ORDER BY changed_at; diff --git a/coderd/insights.go b/coderd/insights.go index d5faacee90bd5..b7b2122b4feb7 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -292,6 +292,73 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp) } +// @Summary Get insights about user status counts over time +// @ID get-insights-about-user-status-counts-over-time +// @Security CoderSessionToken +// @Produce json +// @Tags Insights +// @Param tz_offset query int true "Time-zone offset (e.g. -2)" +// @Success 200 {object} codersdk.GetUserStatusChangesResponse +// @Router /insights/user-status-counts-over-time [get] +func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + p := httpapi.NewQueryParamParser() + vals := r.URL.Query() + tzOffset := p.Int(vals, 0, "tz_offset") + p.ErrorExcessParams(vals) + + if len(p.Errors) > 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Query parameters have invalid values.", + Validations: p.Errors, + }) + return + } + + loc := time.FixedZone("", tzOffset*3600) + // If the time is 14:01 or 14:31, we still want to include all the + // data between 14:00 and 15:00. Our rollups buckets are 30 minutes + // so this works nicely. It works just as well for 23:59 as well. + nextHourInLoc := time.Now().In(loc).Truncate(time.Hour).Add(time.Hour) + // Always return 60 days of data (2 months). + sixtyDaysAgo := nextHourInLoc.In(loc).Truncate(24*time.Hour).AddDate(0, 0, -60) + + rows, err := api.Database.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: sixtyDaysAgo, + EndTime: nextHourInLoc, + }) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching user status counts over time.", + Detail: err.Error(), + }) + return + } + + resp := codersdk.GetUserStatusChangesResponse{ + StatusCounts: make(map[codersdk.UserStatus][]codersdk.UserStatusChangeCount), + } + + slices.SortFunc(rows, func(a, b database.UserStatusChange) int { + return a.ChangedAt.Compare(b.ChangedAt) + }) + + for _, row := range rows { + date := row.ChangedAt.Truncate(24 * time.Hour) + status := codersdk.UserStatus(row.NewStatus) + if _, ok := resp.StatusCounts[status]; !ok { + resp.StatusCounts[status] = make([]codersdk.UserStatusChangeCount, 0) + } + resp.StatusCounts[status] = append(resp.StatusCounts[status], codersdk.UserStatusChangeCount{ + Date: date, + Count: 1, + }) + } + + httpapi.Write(ctx, rw, http.StatusOK, resp) +} + // @Summary Get insights about templates // @ID get-insights-about-templates // @Security CoderSessionToken diff --git a/codersdk/insights.go b/codersdk/insights.go index c9e708de8f34a..b3a3cc728378a 100644 --- a/codersdk/insights.go +++ b/codersdk/insights.go @@ -282,3 +282,34 @@ func (c *Client) TemplateInsights(ctx context.Context, req TemplateInsightsReque var result TemplateInsightsResponse return result, json.NewDecoder(resp.Body).Decode(&result) } + +type GetUserStatusChangesResponse struct { + StatusCounts map[UserStatus][]UserStatusChangeCount `json:"status_counts"` +} + +type UserStatusChangeCount struct { + Date time.Time `json:"date" format:"date-time"` + Count int64 `json:"count" example:"10"` +} + +type GetUserStatusChangesRequest struct { + Offset time.Time `json:"offset" format:"date-time"` +} + +func (c *Client) GetUserStatusChanges(ctx context.Context, req GetUserStatusChangesRequest) (GetUserStatusChangesResponse, error) { + qp := url.Values{} + qp.Add("offset", req.Offset.Format(insightsTimeLayout)) + + reqURL := fmt.Sprintf("/api/v2/insights/user-status-counts-over-time?%s", qp.Encode()) + resp, err := c.Request(ctx, http.MethodGet, reqURL, nil) + if err != nil { + return GetUserStatusChangesResponse{}, xerrors.Errorf("make request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return GetUserStatusChangesResponse{}, ReadBodyAsError(resp) + } + var result GetUserStatusChangesResponse + return result, json.NewDecoder(resp.Body).Decode(&result) +} diff --git a/docs/reference/api/insights.md b/docs/reference/api/insights.md index d9bb2327a9517..8d6f5640f9e60 100644 --- a/docs/reference/api/insights.md +++ b/docs/reference/api/insights.md @@ -244,3 +244,53 @@ curl -X GET http://coder-server:8080/api/v2/insights/user-latency?start_time=201 | 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.UserLatencyInsightsResponse](schemas.md#codersdkuserlatencyinsightsresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). + +## Get insights about user status counts over time + +### Code samples + +```shell +# Example request using curl +curl -X GET http://coder-server:8080/api/v2/insights/user-status-counts-over-time?tz_offset=0 \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +`GET /insights/user-status-counts-over-time` + +### Parameters + +| Name | In | Type | Required | Description | +| ----------- | ----- | ------- | -------- | -------------------------- | +| `tz_offset` | query | integer | true | Time-zone offset (e.g. -2) | + +### Example responses + +> 200 Response + +```json +{ + "status_counts": { + "property1": [ + { + "count": 10, + "date": "2019-08-24T14:15:22Z" + } + ], + "property2": [ + { + "count": 10, + "date": "2019-08-24T14:15:22Z" + } + ] + } +} +``` + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ---------------------------------------------------------------------------------------- | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.GetUserStatusChangesResponse](schemas.md#codersdkgetuserstatuschangesresponse) | + +To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 6b91a64d02789..8abd3ae7162ba 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2882,6 +2882,34 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | ----- | ------ | -------- | ------------ | ----------- | | `key` | string | false | | | +## codersdk.GetUserStatusChangesResponse + +```json +{ + "status_counts": { + "property1": [ + { + "count": 10, + "date": "2019-08-24T14:15:22Z" + } + ], + "property2": [ + { + "count": 10, + "date": "2019-08-24T14:15:22Z" + } + ] + } +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------------------ | ------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `status_counts` | object | false | | | +| » `[any property]` | array of [codersdk.UserStatusChangeCount](#codersdkuserstatuschangecount) | false | | | + ## codersdk.GetUsersResponse ```json @@ -6487,6 +6515,22 @@ If the schedule is empty, the user will be updated to use the default schedule.| | `dormant` | | `suspended` | +## codersdk.UserStatusChangeCount + +```json +{ + "count": 10, + "date": "2019-08-24T14:15:22Z" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------- | ------- | -------- | ------------ | ----------- | +| `count` | integer | false | | | +| `date` | string | false | | | + ## codersdk.ValidateUserPasswordRequest ```json diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 6b0e685b177eb..16c0506774295 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2097,6 +2097,19 @@ class ApiMethods { return response.data; }; + getInsightsUserStatusCountsOverTime = async ( + offset = Math.trunc(new Date().getTimezoneOffset() / 60), + ): Promise => { + const searchParams = new URLSearchParams({ + offset: offset.toString(), + }); + const response = await this.axios.get( + `/api/v2/insights/user-status-counts-over-time?${searchParams}`, + ); + + return response.data; + }; + getHealth = async (force = false) => { const params = new URLSearchParams({ force: force.toString() }); const response = await this.axios.get( diff --git a/site/src/api/queries/insights.ts b/site/src/api/queries/insights.ts index a7044a2f2469f..8f56b5982cd84 100644 --- a/site/src/api/queries/insights.ts +++ b/site/src/api/queries/insights.ts @@ -20,3 +20,10 @@ export const insightsUserActivity = (params: InsightsParams) => { queryFn: () => API.getInsightsUserActivity(params), }; }; + +export const userStatusCountsOverTime = () => { + return { + queryKey: ["userStatusCountsOverTime"], + queryFn: () => API.getInsightsUserStatusCountsOverTime(), + }; +}; diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 77d879abe3258..833d88e6baeef 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -9,7 +9,6 @@ import type { UpdateUserProfileRequest, User, UsersRequest, - ValidateUserPasswordRequest, } from "api/typesGenerated"; import { type MetadataState, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index c605268c9d920..c6dd1f152b66c 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -775,6 +775,16 @@ export interface GenerateAPIKeyResponse { readonly key: string; } +// From codersdk/insights.go +export interface GetUserStatusChangesRequest { + readonly offset: string; +} + +// From codersdk/insights.go +export interface GetUserStatusChangesResponse { + readonly status_counts: Record; +} + // From codersdk/users.go export interface GetUsersResponse { readonly users: readonly User[]; @@ -2316,6 +2326,12 @@ export interface UserRoles { // From codersdk/users.go export type UserStatus = "active" | "dormant" | "suspended"; +// From codersdk/insights.go +export interface UserStatusChangeCount { + readonly date: string; + readonly count: number; +} + export const UserStatuses: UserStatus[] = ["active", "dormant", "suspended"]; // From codersdk/users.go diff --git a/site/src/components/ActiveUserChart/ActiveUserChart.stories.tsx b/site/src/components/ActiveUserChart/ActiveUserChart.stories.tsx index 4f28d7243a0bf..b77886b63fd2a 100644 --- a/site/src/components/ActiveUserChart/ActiveUserChart.stories.tsx +++ b/site/src/components/ActiveUserChart/ActiveUserChart.stories.tsx @@ -5,14 +5,19 @@ const meta: Meta = { title: "components/ActiveUserChart", component: ActiveUserChart, args: { - data: [ - { date: "1/1/2024", amount: 5 }, - { date: "1/2/2024", amount: 6 }, - { date: "1/3/2024", amount: 7 }, - { date: "1/4/2024", amount: 8 }, - { date: "1/5/2024", amount: 9 }, - { date: "1/6/2024", amount: 10 }, - { date: "1/7/2024", amount: 11 }, + series: [ + { + label: "Daily", + data: [ + { date: "1/1/2024", amount: 5 }, + { date: "1/2/2024", amount: 6 }, + { date: "1/3/2024", amount: 7 }, + { date: "1/4/2024", amount: 8 }, + { date: "1/5/2024", amount: 9 }, + { date: "1/6/2024", amount: 10 }, + { date: "1/7/2024", amount: 11 }, + ], + }, ], interval: "day", }, @@ -22,3 +27,51 @@ export default meta; type Story = StoryObj; export const Example: Story = {}; + +export const MultipleSeries: Story = { + args: { + series: [ + { + label: "Active", + data: [ + { date: "1/1/2024", amount: 150 }, + { date: "1/2/2024", amount: 165 }, + { date: "1/3/2024", amount: 180 }, + { date: "1/4/2024", amount: 155 }, + { date: "1/5/2024", amount: 190 }, + { date: "1/6/2024", amount: 200 }, + { date: "1/7/2024", amount: 210 }, + ], + color: "green", + }, + { + label: "Dormant", + data: [ + { date: "1/1/2024", amount: 80 }, + { date: "1/2/2024", amount: 82 }, + { date: "1/3/2024", amount: 85 }, + { date: "1/4/2024", amount: 88 }, + { date: "1/5/2024", amount: 90 }, + { date: "1/6/2024", amount: 92 }, + { date: "1/7/2024", amount: 95 }, + ], + color: "grey", + }, + { + label: "Suspended", + data: [ + { date: "1/1/2024", amount: 20 }, + { date: "1/2/2024", amount: 22 }, + { date: "1/3/2024", amount: 25 }, + { date: "1/4/2024", amount: 23 }, + { date: "1/5/2024", amount: 28 }, + { date: "1/6/2024", amount: 30 }, + { date: "1/7/2024", amount: 32 }, + ], + color: "red", + }, + ], + interval: "day", + userLimit: 100, + }, +}; diff --git a/site/src/components/ActiveUserChart/ActiveUserChart.tsx b/site/src/components/ActiveUserChart/ActiveUserChart.tsx index 41345ea8f03f8..10acb6ec9fc90 100644 --- a/site/src/components/ActiveUserChart/ActiveUserChart.tsx +++ b/site/src/components/ActiveUserChart/ActiveUserChart.tsx @@ -1,5 +1,7 @@ import "chartjs-adapter-date-fns"; import { useTheme } from "@emotion/react"; +import LaunchOutlined from "@mui/icons-material/LaunchOutlined"; +import Button from "@mui/material/Button"; import { CategoryScale, Chart as ChartJS, @@ -14,6 +16,7 @@ import { Tooltip, defaults, } from "chart.js"; +import annotationPlugin from "chartjs-plugin-annotation"; import { HelpTooltip, HelpTooltipContent, @@ -35,32 +38,51 @@ ChartJS.register( Title, Tooltip, Legend, + annotationPlugin, ); -export interface ActiveUserChartProps { +export interface DataSeries { + label?: string; data: readonly { date: string; amount: number }[]; + color?: string; // Optional custom color +} + +export interface ActiveUserChartProps { + series: DataSeries[]; + userLimit?: number; interval: "day" | "week"; } export const ActiveUserChart: FC = ({ - data, + series, + userLimit, interval, }) => { const theme = useTheme(); - const labels = data.map((val) => dayjs(val.date).format("YYYY-MM-DD")); - const chartData = data.map((val) => val.amount); - defaults.font.family = theme.typography.fontFamily as string; defaults.color = theme.palette.text.secondary; const options: ChartOptions<"line"> = { responsive: true, animation: false, + interaction: { + mode: "index", + }, plugins: { - legend: { - display: false, - }, + legend: + series.length > 1 + ? { + display: false, + position: "top" as const, + labels: { + usePointStyle: true, + pointStyle: "line", + }, + } + : { + display: false, + }, tooltip: { displayColors: false, callbacks: { @@ -70,6 +92,24 @@ export const ActiveUserChart: FC = ({ }, }, }, + annotation: { + annotations: [ + { + type: "line", + scaleID: "y", + value: userLimit, + borderColor: "white", + borderWidth: 2, + label: { + content: "Active User limit", + color: theme.palette.primary.contrastText, + display: true, + textStrokeWidth: 2, + textStrokeColor: theme.palette.background.paper, + }, + }, + ], + }, }, scales: { y: { @@ -78,11 +118,12 @@ export const ActiveUserChart: FC = ({ ticks: { precision: 0, }, + stacked: true, }, x: { grid: { color: theme.palette.divider }, ticks: { - stepSize: data.length > 10 ? 2 : undefined, + stepSize: series[0].data.length > 10 ? 2 : undefined, }, type: "time", time: { @@ -97,16 +138,16 @@ export const ActiveUserChart: FC = ({ + dayjs(val.date).format("YYYY-MM-DD"), + ), + datasets: series.map((s) => ({ + label: s.label, + data: s.data.map((val) => val.amount), + pointBackgroundColor: s.color || theme.roles.active.outline, + pointBorderColor: s.color || theme.roles.active.outline, + borderColor: s.color || theme.roles.active.outline, + })), }} options={options} /> @@ -120,11 +161,13 @@ type ActiveUsersTitleProps = { export const ActiveUsersTitle: FC = ({ interval }) => { return (
- {interval === "day" ? "Daily" : "Weekly"} Active Users + {interval === "day" ? "Daily" : "Weekly"} User Activity - How do we calculate active users? + + How do we calculate user activity? + When a connection is initiated to a user's workspace they are considered an active user. e.g. apps, web terminal, SSH. This is for @@ -136,3 +179,39 @@ export const ActiveUsersTitle: FC = ({ interval }) => {
); }; + +export type UserStatusTitleProps = { + interval: "day" | "week"; +}; + +export const UserStatusTitle: FC = ({ interval }) => { + return ( +
+ {interval === "day" ? "Daily" : "Weekly"} User Status + + + + What are user statuses? + + + Active users count towards your license consumption. Dormant or + suspended users do not. Any user who has logged into the coder + platform within the last 90 days is considered active. + + + + + +
+ ); +}; diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx index 2b094cbf89b26..3de614a42ac39 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx @@ -1,6 +1,6 @@ import { deploymentDAUs } from "api/queries/deployment"; -import { entitlements } from "api/queries/entitlements"; import { availableExperiments, experiments } from "api/queries/experiments"; +import { userStatusCountsOverTime } from "api/queries/insights"; import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { useDeploymentSettings } from "modules/management/DeploymentSettingsProvider"; import type { FC } from "react"; @@ -15,9 +15,8 @@ const GeneralSettingsPage: FC = () => { const safeExperimentsQuery = useQuery(availableExperiments()); const { metadata } = useEmbeddedMetadata(); - const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); const enabledExperimentsQuery = useQuery(experiments(metadata.experiments)); - + const userStatusCountsOverTimeQuery = useQuery(userStatusCountsOverTime()); const safeExperiments = safeExperimentsQuery.data?.safe ?? []; const invalidExperiments = enabledExperimentsQuery.data?.filter((exp) => { @@ -33,9 +32,9 @@ const GeneralSettingsPage: FC = () => { deploymentOptions={deploymentConfig.options} deploymentDAUs={deploymentDAUsQuery.data} deploymentDAUsError={deploymentDAUsQuery.error} - entitlements={entitlementsQuery.data} invalidExperiments={invalidExperiments} safeExperiments={safeExperiments} + userStatusCountsOverTime={userStatusCountsOverTimeQuery.data} /> ); diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.stories.tsx index 05ed426d5dcc9..78291ee03b4d8 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.stories.tsx @@ -1,9 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react"; -import { - MockDeploymentDAUResponse, - MockEntitlementsWithUserLimit, - mockApiError, -} from "testHelpers/entities"; +import { MockDeploymentDAUResponse, mockApiError } from "testHelpers/entities"; import { GeneralSettingsPageView } from "./GeneralSettingsPageView"; const meta: Meta = { @@ -42,7 +38,100 @@ const meta: Meta = { deploymentDAUs: MockDeploymentDAUResponse, invalidExperiments: [], safeExperiments: [], - entitlements: undefined, + userStatusCountsOverTime: { + status_counts: { + active: [ + { + date: "1/1/2024", + count: 1, + }, + { + date: "1/2/2024", + count: 8, + }, + { + date: "1/3/2024", + count: 8, + }, + { + date: "1/4/2024", + count: 6, + }, + { + date: "1/5/2024", + count: 6, + }, + { + date: "1/6/2024", + count: 6, + }, + { + date: "1/7/2024", + count: 6, + }, + ], + dormant: [ + { + date: "1/1/2024", + count: 0, + }, + { + date: "1/2/2024", + count: 3, + }, + { + date: "1/3/2024", + count: 3, + }, + { + date: "1/4/2024", + count: 3, + }, + { + date: "1/5/2024", + count: 3, + }, + { + date: "1/6/2024", + count: 3, + }, + { + date: "1/7/2024", + count: 3, + }, + ], + suspended: [ + { + date: "1/1/2024", + count: 0, + }, + { + date: "1/2/2024", + count: 0, + }, + { + date: "1/3/2024", + count: 0, + }, + { + date: "1/4/2024", + count: 2, + }, + { + date: "1/5/2024", + count: 2, + }, + { + date: "1/6/2024", + count: 2, + }, + { + date: "1/7/2024", + count: 2, + }, + ], + }, + }, }, }; @@ -138,73 +227,26 @@ export const invalidExperimentsEnabled: Story = { }, }; -export const WithLicenseUtilization: Story = { - args: { - entitlements: { - ...MockEntitlementsWithUserLimit, - features: { - ...MockEntitlementsWithUserLimit.features, - user_limit: { - ...MockEntitlementsWithUserLimit.features.user_limit, - enabled: true, - actual: 75, - limit: 100, - entitlement: "entitled", - }, - }, - }, - }, +export const UnlicensedInstallation: Story = { + args: {}, }; -export const HighLicenseUtilization: Story = { - args: { - entitlements: { - ...MockEntitlementsWithUserLimit, - features: { - ...MockEntitlementsWithUserLimit.features, - user_limit: { - ...MockEntitlementsWithUserLimit.features.user_limit, - enabled: true, - actual: 95, - limit: 100, - entitlement: "entitled", - }, - }, - }, - }, +export const LicensedWithNoUserLimit: Story = { + args: {}, }; -export const ExceedsLicenseUtilization: Story = { +export const LicensedWithPlentyOfSpareLicenses: Story = { args: { - entitlements: { - ...MockEntitlementsWithUserLimit, - features: { - ...MockEntitlementsWithUserLimit.features, - user_limit: { - ...MockEntitlementsWithUserLimit.features.user_limit, - enabled: true, - actual: 100, - limit: 95, - entitlement: "entitled", - }, - }, - }, + activeUserLimit: 100, }, }; -export const NoLicenseLimit: Story = { + +export const TotalUsersExceedsLicenseButNotActiveUsers: Story = { args: { - entitlements: { - ...MockEntitlementsWithUserLimit, - features: { - ...MockEntitlementsWithUserLimit.features, - user_limit: { - ...MockEntitlementsWithUserLimit.features.user_limit, - enabled: false, - actual: 0, - limit: 0, - entitlement: "entitled", - }, - }, - }, + activeUserLimit: 8, }, }; + +export const ManyUsers: Story = { + args: {}, +}; diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx index df5550d70e965..bf663fecaa945 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx @@ -1,14 +1,15 @@ import AlertTitle from "@mui/material/AlertTitle"; -import LinearProgress from "@mui/material/LinearProgress"; import type { DAUsResponse, - Entitlements, Experiments, + GetUserStatusChangesResponse, SerpentOption, } from "api/typesGenerated"; import { ActiveUserChart, ActiveUsersTitle, + type DataSeries, + UserStatusTitle, } from "components/ActiveUserChart/ActiveUserChart"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { SettingsHeader } from "components/SettingsHeader/SettingsHeader"; @@ -24,7 +25,8 @@ export type GeneralSettingsPageViewProps = { deploymentOptions: SerpentOption[]; deploymentDAUs?: DAUsResponse; deploymentDAUsError: unknown; - entitlements: Entitlements | undefined; + userStatusCountsOverTime?: GetUserStatusChangesResponse; + activeUserLimit?: number; readonly invalidExperiments: Experiments | string[]; readonly safeExperiments: Experiments | string[]; }; @@ -33,16 +35,29 @@ export const GeneralSettingsPageView: FC = ({ deploymentOptions, deploymentDAUs, deploymentDAUsError, - entitlements, + userStatusCountsOverTime, + activeUserLimit, safeExperiments, invalidExperiments, }) => { - const licenseUtilizationPercentage = - entitlements?.features?.user_limit?.actual && - entitlements?.features?.user_limit?.limit - ? entitlements.features.user_limit.actual / - entitlements.features.user_limit.limit - : undefined; + const colors: Record = { + active: "green", + dormant: "grey", + deleted: "red", + }; + let series: DataSeries[] = []; + if (userStatusCountsOverTime?.status_counts) { + series = Object.entries(userStatusCountsOverTime.status_counts).map( + ([status, counts]) => ({ + label: status, + data: counts.map((count) => ({ + date: count.date.toString(), + amount: count.count, + })), + color: colors[status], + }), + ); + } return ( <> = ({ {Boolean(deploymentDAUsError) && ( )} - {deploymentDAUs && ( -
- }> - - -
+ {series.length && ( + }> + + )} - {licenseUtilizationPercentage && ( - - }> + - - {Math.round(licenseUtilizationPercentage * 100)}% used ( - {entitlements!.features.user_limit.actual}/ - {entitlements!.features.user_limit.limit} users) - )} {invalidExperiments.length > 0 && ( diff --git a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx index 097b8fce513e7..2b873c325e274 100644 --- a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx +++ b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx @@ -258,10 +258,14 @@ const ActiveUsersPanel: FC = ({ {data && data.length > 0 && ( ({ - amount: d.active_users, - date: d.start_time, - }))} + series={[ + { + data: data.map((d) => ({ + amount: d.active_users, + date: d.start_time, + })), + }, + ]} /> )} From b2047830237208816f24c6a0e9640538e6ccfeb6 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 23 Dec 2024 09:57:56 +0000 Subject: [PATCH 08/23] rename migration --- ...tatus_changes.down.sql => 000280_user_status_changes.down.sql} | 0 ...er_status_changes.up.sql => 000280_user_status_changes.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000279_user_status_changes.down.sql => 000280_user_status_changes.down.sql} (100%) rename coderd/database/migrations/{000279_user_status_changes.up.sql => 000280_user_status_changes.up.sql} (100%) diff --git a/coderd/database/migrations/000279_user_status_changes.down.sql b/coderd/database/migrations/000280_user_status_changes.down.sql similarity index 100% rename from coderd/database/migrations/000279_user_status_changes.down.sql rename to coderd/database/migrations/000280_user_status_changes.down.sql diff --git a/coderd/database/migrations/000279_user_status_changes.up.sql b/coderd/database/migrations/000280_user_status_changes.up.sql similarity index 100% rename from coderd/database/migrations/000279_user_status_changes.up.sql rename to coderd/database/migrations/000280_user_status_changes.up.sql From ce1bda5f8b8ab3f3a362f51a1e5eec396cf83ecd Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 08:49:05 +0000 Subject: [PATCH 09/23] move aggregation logic for GetUserStatusChanges into the SQL --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/database/dbmem/dbmem.go | 21 ++++++- coderd/database/dbmetrics/querymetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 4 +- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 74 ++++++++++++++++------- coderd/database/queries/insights.sql | 37 ++++++++++-- coderd/insights.go | 9 +-- 8 files changed, 108 insertions(+), 43 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index cbae48e83c8ba..78d620bd82020 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2413,7 +2413,7 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui return q.db.GetUserNotificationPreferences(ctx, userID) } -func (q *querier) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { +func (q *querier) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceUser); err != nil { return nil, err } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 08b4d66724f41..ca6a41e27266d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5666,7 +5666,7 @@ func (q *FakeQuerier) GetUserNotificationPreferences(_ context.Context, userID u return out, nil } -func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { +func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -5675,12 +5675,27 @@ func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUs return nil, err } - result := make([]database.UserStatusChange, 0) + result := make([]database.GetUserStatusChangesRow, 0) for _, change := range q.userStatusChanges { if change.ChangedAt.Before(arg.StartTime) || change.ChangedAt.After(arg.EndTime) { continue } - result = append(result, change) + if !slices.ContainsFunc(result, func(r database.GetUserStatusChangesRow) bool { + return r.ChangedAt.Equal(change.ChangedAt) && r.NewStatus == change.NewStatus + }) { + result = append(result, database.GetUserStatusChangesRow{ + NewStatus: change.NewStatus, + ChangedAt: change.ChangedAt, + Count: 1, + }) + } else { + for i, r := range result { + if r.ChangedAt.Equal(change.ChangedAt) && r.NewStatus == change.NewStatus { + result[i].Count++ + break + } + } + } } return result, nil diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 989a18a8856f4..c1662346adf63 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1337,7 +1337,7 @@ func (m queryMetricsStore) GetUserNotificationPreferences(ctx context.Context, u return r0, r1 } -func (m queryMetricsStore) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { +func (m queryMetricsStore) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { start := time.Now() r0, r1 := m.s.GetUserStatusChanges(ctx, arg) m.queryLatencies.WithLabelValues("GetUserStatusChanges").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a8cb8effc3214..eedac4ced468f 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2812,10 +2812,10 @@ func (mr *MockStoreMockRecorder) GetUserNotificationPreferences(arg0, arg1 any) } // GetUserStatusChanges mocks base method. -func (m *MockStore) GetUserStatusChanges(arg0 context.Context, arg1 database.GetUserStatusChangesParams) ([]database.UserStatusChange, error) { +func (m *MockStore) GetUserStatusChanges(arg0 context.Context, arg1 database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetUserStatusChanges", arg0, arg1) - ret0, _ := ret[0].([]database.UserStatusChange) + ret0, _ := ret[0].([]database.GetUserStatusChangesRow) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 248fd7bec39a7..373a5f28b66df 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -285,7 +285,7 @@ type sqlcQuerier interface { GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUserLinksByUserID(ctx context.Context, userID uuid.UUID) ([]UserLink, error) GetUserNotificationPreferences(ctx context.Context, userID uuid.UUID) ([]NotificationPreference, error) - GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]UserStatusChange, error) + GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]GetUserStatusChangesRow, error) GetUserWorkspaceBuildParameters(ctx context.Context, arg GetUserWorkspaceBuildParametersParams) ([]GetUserWorkspaceBuildParametersRow, error) // This will never return deleted users. GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUsersRow, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 5d870802c27fc..aec0939abc3cf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3094,13 +3094,40 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate return items, nil } -const GetUserStatusChanges = `-- name: GetUserStatusChanges :many +const getUserStatusChanges = `-- name: GetUserStatusChanges :many +WITH last_status_per_day AS ( + -- First get the last status change for each user for each day + SELECT DISTINCT ON (date_trunc('day', changed_at), user_id) + date_trunc('day', changed_at)::timestamptz AS date, + new_status, + user_id + FROM user_status_changes + WHERE changed_at >= $1::timestamptz + AND changed_at < $2::timestamptz + ORDER BY + date_trunc('day', changed_at), + user_id, + changed_at DESC -- This ensures we get the last status for each day +), +daily_counts AS ( + -- Then count unique users per status per day + SELECT + date, + new_status, + COUNT(*) AS count + FROM last_status_per_day + GROUP BY + date, + new_status +) SELECT - id, user_id, new_status, changed_at -FROM user_status_changes -WHERE changed_at >= $1::timestamptz - AND changed_at < $2::timestamptz -ORDER BY changed_at + date::timestamptz AS changed_at, + new_status, + count::bigint +FROM daily_counts +ORDER BY + new_status ASC, + date ASC ` type GetUserStatusChangesParams struct { @@ -3108,21 +3135,22 @@ type GetUserStatusChangesParams struct { EndTime time.Time `db:"end_time" json:"end_time"` } -func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]UserStatusChange, error) { - rows, err := q.db.QueryContext(ctx, GetUserStatusChanges, arg.StartTime, arg.EndTime) +type GetUserStatusChangesRow struct { + ChangedAt time.Time `db:"changed_at" json:"changed_at"` + NewStatus UserStatus `db:"new_status" json:"new_status"` + Count int64 `db:"count" json:"count"` +} + +func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]GetUserStatusChangesRow, error) { + rows, err := q.db.QueryContext(ctx, getUserStatusChanges, arg.StartTime, arg.EndTime) if err != nil { return nil, err } defer rows.Close() - var items []UserStatusChange + var items []GetUserStatusChangesRow for rows.Next() { - var i UserStatusChange - if err := rows.Scan( - &i.ID, - &i.UserID, - &i.NewStatus, - &i.ChangedAt, - ); err != nil { + var i GetUserStatusChangesRow + if err := rows.Scan(&i.ChangedAt, &i.NewStatus, &i.Count); err != nil { return nil, err } items = append(items, i) @@ -3432,7 +3460,7 @@ func (q *sqlQuerier) GetJFrogXrayScanByWorkspaceAndAgentID(ctx context.Context, } const upsertJFrogXrayScanByWorkspaceAndAgentID = `-- name: UpsertJFrogXrayScanByWorkspaceAndAgentID :exec -INSERT INTO +INSERT INTO jfrog_xray_scans ( agent_id, workspace_id, @@ -3441,7 +3469,7 @@ INSERT INTO medium, results_url ) -VALUES +VALUES ($1, $2, $3, $4, $5, $6) ON CONFLICT (agent_id, workspace_id) DO UPDATE SET critical = $3, high = $4, medium = $5, results_url = $6 @@ -6316,7 +6344,7 @@ FROM provisioner_keys WHERE organization_id = $1 -AND +AND lower(name) = lower($2) ` @@ -6432,10 +6460,10 @@ WHERE AND -- exclude reserved built-in key id != '00000000-0000-0000-0000-000000000001'::uuid -AND +AND -- exclude reserved user-auth key id != '00000000-0000-0000-0000-000000000002'::uuid -AND +AND -- exclude reserved psk key id != '00000000-0000-0000-0000-000000000003'::uuid ` @@ -8121,7 +8149,7 @@ func (q *sqlQuerier) GetTailnetTunnelPeerIDs(ctx context.Context, srcID uuid.UUI } const updateTailnetPeerStatusByCoordinator = `-- name: UpdateTailnetPeerStatusByCoordinator :exec -UPDATE +UPDATE tailnet_peers SET status = $2 @@ -12707,7 +12735,7 @@ WITH agent_stats AS ( coalesce((PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY connection_median_latency_ms)), -1)::FLOAT AS workspace_connection_latency_95 FROM workspace_agent_stats -- The greater than 0 is to support legacy agents that don't report connection_median_latency_ms. - WHERE workspace_agent_stats.created_at > $1 AND connection_median_latency_ms > 0 + WHERE workspace_agent_stats.created_at > $1 AND connection_median_latency_ms > 0 GROUP BY user_id, agent_id, workspace_id, template_id ), latest_agent_stats AS ( SELECT diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index dfa236dfbd6d4..513ff7eeec1ec 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -773,9 +773,36 @@ JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.wor GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value; -- name: GetUserStatusChanges :many +WITH last_status_per_day AS ( + -- First get the last status change for each user for each day + SELECT DISTINCT ON (date_trunc('day', changed_at), user_id) + date_trunc('day', changed_at)::timestamptz AS date, + new_status, + user_id + FROM user_status_changes + WHERE changed_at >= @start_time::timestamptz + AND changed_at < @end_time::timestamptz + ORDER BY + date_trunc('day', changed_at), + user_id, + changed_at DESC -- This ensures we get the last status for each day +), +daily_counts AS ( + -- Then count unique users per status per day + SELECT + date, + new_status, + COUNT(*) AS count + FROM last_status_per_day + GROUP BY + date, + new_status +) SELECT - * -FROM user_status_changes -WHERE changed_at >= @start_time::timestamptz - AND changed_at < @end_time::timestamptz -ORDER BY changed_at; + date::timestamptz AS changed_at, + new_status, + count::bigint +FROM daily_counts +ORDER BY + new_status ASC, + date ASC; diff --git a/coderd/insights.go b/coderd/insights.go index b7b2122b4feb7..dfa914425f965 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -340,19 +340,14 @@ func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http StatusCounts: make(map[codersdk.UserStatus][]codersdk.UserStatusChangeCount), } - slices.SortFunc(rows, func(a, b database.UserStatusChange) int { - return a.ChangedAt.Compare(b.ChangedAt) - }) - for _, row := range rows { - date := row.ChangedAt.Truncate(24 * time.Hour) status := codersdk.UserStatus(row.NewStatus) if _, ok := resp.StatusCounts[status]; !ok { resp.StatusCounts[status] = make([]codersdk.UserStatusChangeCount, 0) } resp.StatusCounts[status] = append(resp.StatusCounts[status], codersdk.UserStatusChangeCount{ - Date: date, - Count: 1, + Date: row.ChangedAt, + Count: row.Count, }) } From 734ff2b34ce9382c8e6205b1762b93d7c7bf0d77 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 10:20:09 +0000 Subject: [PATCH 10/23] use window functions for efficiency --- coderd/database/querier_test.go | 57 ++++++++++++----------- coderd/database/queries.sql.go | 68 ++++++++++++++++++++-------- coderd/database/queries/insights.sql | 68 ++++++++++++++++++++-------- 3 files changed, 127 insertions(+), 66 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index c724789df58ac..1c5d4778f6dcb 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2258,6 +2258,10 @@ func TestGroupRemovalTrigger(t *testing.T) { func TestGetUserStatusChanges(t *testing.T) { t.Parallel() + now := dbtime.Now() + createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago + firstTransitionTime := createdAt.Add(2 * 24 * time.Hour) // 3 days ago + secondTransitionTime := firstTransitionTime.Add(2 * 24 * time.Hour) // 1 days ago t.Run("No Users", func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) @@ -2307,8 +2311,6 @@ func TestGetUserStatusChanges(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) // Create a user that's been in the specified status for the past 30 days - now := dbtime.Now() - createdAt := now.Add(-29 * 24 * time.Hour) dbgen.User(t, db, database.User{ Status: tc.status, CreatedAt: createdAt, @@ -2324,8 +2326,10 @@ func TestGetUserStatusChanges(t *testing.T) { require.NotEmpty(t, userStatusChanges, "should return results") // We should have an entry for each status change - require.Len(t, userStatusChanges, 1, "should have 1 status change") - require.Equal(t, userStatusChanges[0].NewStatus, tc.status, "should have the correct status") + require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") + for _, row := range userStatusChanges { + require.Equal(t, row.NewStatus, tc.status, "should have the correct status") + } }) } }) @@ -2393,8 +2397,6 @@ func TestGetUserStatusChanges(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) // Create a user that starts with initial status - now := dbtime.Now() - createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago user := dbgen.User(t, db, database.User{ Status: tc.initialStatus, CreatedAt: createdAt, @@ -2402,11 +2404,10 @@ func TestGetUserStatusChanges(t *testing.T) { }) // After 2 days, change status to target status - statusChangeTime := createdAt.Add(2 * 24 * time.Hour) user, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user.ID, Status: tc.targetStatus, - UpdatedAt: statusChangeTime, + UpdatedAt: firstTransitionTime, }) require.NoError(t, err) @@ -2418,10 +2419,15 @@ func TestGetUserStatusChanges(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, userStatusChanges, "should return results") - // We should have an entry for each status change, including the initial status - require.Len(t, userStatusChanges, 2, "should have 2 status changes") - require.Equal(t, userStatusChanges[0].NewStatus, tc.initialStatus, "should have the initial status") - require.Equal(t, userStatusChanges[1].NewStatus, tc.targetStatus, "should have the target status") + // We should have an entry for each status (active, dormant, suspended) for each day + require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") + for _, row := range userStatusChanges { + if row.ChangedAt.Before(firstTransitionTime) { + require.Equal(t, row.NewStatus, tc.initialStatus, "should have the initial status") + } else { + require.Equal(t, row.NewStatus, tc.targetStatus, "should have the target status") + } + } }) } }) @@ -2606,20 +2612,18 @@ func TestGetUserStatusChanges(t *testing.T) { }) // First transition at 2 days - user1TransitionTime := createdAt.Add(2 * 24 * time.Hour) user1, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user1.ID, Status: tc.user1Transition.to, - UpdatedAt: user1TransitionTime, + UpdatedAt: firstTransitionTime, }) require.NoError(t, err) // Second transition at 4 days - user2TransitionTime := createdAt.Add(4 * 24 * time.Hour) user2, err = db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: user2.ID, Status: tc.user2Transition.to, - UpdatedAt: user2TransitionTime, + UpdatedAt: secondTransitionTime, }) require.NoError(t, err) @@ -2631,16 +2635,17 @@ func TestGetUserStatusChanges(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, userStatusChanges) - // We should have an entry with the correct status changes for each user, including the initial status - require.Len(t, userStatusChanges, 4, "should have 4 status changes") - require.Equal(t, userStatusChanges[0].UserID, user1.ID, "should have the first user") - require.Equal(t, userStatusChanges[0].NewStatus, tc.user1Transition.from, "should have the first user's initial status") - require.Equal(t, userStatusChanges[1].UserID, user1.ID, "should have the first user") - require.Equal(t, userStatusChanges[1].NewStatus, tc.user1Transition.to, "should have the first user's target status") - require.Equal(t, userStatusChanges[2].UserID, user2.ID, "should have the second user") - require.Equal(t, userStatusChanges[2].NewStatus, tc.user2Transition.from, "should have the second user's initial status") - require.Equal(t, userStatusChanges[3].UserID, user2.ID, "should have the second user") - require.Equal(t, userStatusChanges[3].NewStatus, tc.user2Transition.to, "should have the second user's target status") + // Expected counts before, between and after the transitions should match: + for _, row := range userStatusChanges { + switch { + case row.ChangedAt.Before(firstTransitionTime): + require.Equal(t, row.Count, tc.expectedCounts["initial"][row.NewStatus], "should have the correct count before the first transition") + case row.ChangedAt.Before(secondTransitionTime): + require.Equal(t, row.Count, tc.expectedCounts["between"][row.NewStatus], "should have the correct count between the transitions") + case row.ChangedAt.Before(now): + require.Equal(t, row.Count, tc.expectedCounts["final"][row.NewStatus], "should have the correct count after the second transition") + } + } }) } }) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index aec0939abc3cf..03ae1cf9bc3c9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3095,36 +3095,64 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate } const getUserStatusChanges = `-- name: GetUserStatusChanges :many -WITH last_status_per_day AS ( - -- First get the last status change for each user for each day - SELECT DISTINCT ON (date_trunc('day', changed_at), user_id) - date_trunc('day', changed_at)::timestamptz AS date, +WITH dates AS ( + SELECT generate_series( + date_trunc('day', $1::timestamptz), + date_trunc('day', $2::timestamptz), + '1 day'::interval + )::timestamptz AS date +), +latest_status_before_range AS ( + -- Get the last status change for each user before our date range + SELECT DISTINCT ON (user_id) + user_id, new_status, - user_id + changed_at FROM user_status_changes - WHERE changed_at >= $1::timestamptz - AND changed_at < $2::timestamptz - ORDER BY - date_trunc('day', changed_at), + WHERE changed_at < (SELECT MIN(date) FROM dates) + ORDER BY user_id, changed_at DESC +), +all_status_changes AS ( + -- Combine status changes before and during our range + SELECT + user_id, + new_status, + changed_at + FROM latest_status_before_range + + UNION ALL + + SELECT user_id, - changed_at DESC -- This ensures we get the last status for each day + new_status, + changed_at + FROM user_status_changes + WHERE changed_at < $2::timestamptz ), daily_counts AS ( - -- Then count unique users per status per day SELECT - date, - new_status, - COUNT(*) AS count - FROM last_status_per_day - GROUP BY - date, - new_status + d.date, + asc1.new_status, + -- For each date and status, count users whose most recent status change + -- (up to that date) matches this status + COUNT(*) FILTER ( + WHERE asc1.changed_at = ( + SELECT MAX(changed_at) + FROM all_status_changes asc2 + WHERE asc2.user_id = asc1.user_id + AND asc2.changed_at <= d.date + ) + )::bigint AS count + FROM dates d + CROSS JOIN all_status_changes asc1 + GROUP BY d.date, asc1.new_status ) SELECT - date::timestamptz AS changed_at, + date AS changed_at, new_status, - count::bigint + count FROM daily_counts +WHERE count > 0 ORDER BY new_status ASC, date ASC diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 513ff7eeec1ec..25f46617efeba 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -773,36 +773,64 @@ JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.wor GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value; -- name: GetUserStatusChanges :many -WITH last_status_per_day AS ( - -- First get the last status change for each user for each day - SELECT DISTINCT ON (date_trunc('day', changed_at), user_id) - date_trunc('day', changed_at)::timestamptz AS date, +WITH dates AS ( + SELECT generate_series( + date_trunc('day', @start_time::timestamptz), + date_trunc('day', @end_time::timestamptz), + '1 day'::interval + )::timestamptz AS date +), +latest_status_before_range AS ( + -- Get the last status change for each user before our date range + SELECT DISTINCT ON (user_id) + user_id, new_status, - user_id + changed_at FROM user_status_changes - WHERE changed_at >= @start_time::timestamptz - AND changed_at < @end_time::timestamptz - ORDER BY - date_trunc('day', changed_at), + WHERE changed_at < (SELECT MIN(date) FROM dates) + ORDER BY user_id, changed_at DESC +), +all_status_changes AS ( + -- Combine status changes before and during our range + SELECT + user_id, + new_status, + changed_at + FROM latest_status_before_range + + UNION ALL + + SELECT user_id, - changed_at DESC -- This ensures we get the last status for each day + new_status, + changed_at + FROM user_status_changes + WHERE changed_at < @end_time::timestamptz ), daily_counts AS ( - -- Then count unique users per status per day SELECT - date, - new_status, - COUNT(*) AS count - FROM last_status_per_day - GROUP BY - date, - new_status + d.date, + asc1.new_status, + -- For each date and status, count users whose most recent status change + -- (up to that date) matches this status + COUNT(*) FILTER ( + WHERE asc1.changed_at = ( + SELECT MAX(changed_at) + FROM all_status_changes asc2 + WHERE asc2.user_id = asc1.user_id + AND asc2.changed_at <= d.date + ) + )::bigint AS count + FROM dates d + CROSS JOIN all_status_changes asc1 + GROUP BY d.date, asc1.new_status ) SELECT - date::timestamptz AS changed_at, + date AS changed_at, new_status, - count::bigint + count FROM daily_counts +WHERE count > 0 ORDER BY new_status ASC, date ASC; From 737e63ef96e1f8fe8a841648161b410563fa55fd Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 10:27:40 +0000 Subject: [PATCH 11/23] ensure we use the same time zone as the start_time param --- coderd/database/queries/insights.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 25f46617efeba..18ea61de32773 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -774,11 +774,11 @@ GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.de -- name: GetUserStatusChanges :many WITH dates AS ( - SELECT generate_series( + SELECT (generate_series( date_trunc('day', @start_time::timestamptz), date_trunc('day', @end_time::timestamptz), '1 day'::interval - )::timestamptz AS date + ) AT TIME ZONE (extract(timezone FROM @start_time::timestamptz)::text || ' minutes'))::timestamptz AS date ), latest_status_before_range AS ( -- Get the last status change for each user before our date range From 045e4388caf0249168d1a2b3c4d128ec62c4b7de Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 10:37:38 +0000 Subject: [PATCH 12/23] ensure we use the same time zone as the start_time param --- coderd/database/queries.sql.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 03ae1cf9bc3c9..7ebe377b948b5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3096,11 +3096,11 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate const getUserStatusChanges = `-- name: GetUserStatusChanges :many WITH dates AS ( - SELECT generate_series( + SELECT (generate_series( date_trunc('day', $1::timestamptz), date_trunc('day', $2::timestamptz), '1 day'::interval - )::timestamptz AS date + ) AT TIME ZONE (extract(timezone FROM $1::timestamptz)::text || ' minutes'))::timestamptz AS date ), latest_status_before_range AS ( -- Get the last status change for each user before our date range From 32aeb4da541727f8212451af18419f8bf3366f06 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 14:29:32 +0000 Subject: [PATCH 13/23] make gen --- site/src/api/typesGenerated.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index c6dd1f152b66c..c8d33fb6b85e2 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -777,12 +777,12 @@ export interface GenerateAPIKeyResponse { // From codersdk/insights.go export interface GetUserStatusChangesRequest { - readonly offset: string; + readonly offset: string; } // From codersdk/insights.go export interface GetUserStatusChangesResponse { - readonly status_counts: Record; + readonly status_counts: Record; } // From codersdk/users.go @@ -2328,8 +2328,8 @@ export type UserStatus = "active" | "dormant" | "suspended"; // From codersdk/insights.go export interface UserStatusChangeCount { - readonly date: string; - readonly count: number; + readonly date: string; + readonly count: number; } export const UserStatuses: UserStatus[] = ["active", "dormant", "suspended"]; From 1a2fde82c5c00ccae74e93619f4401ae59f8cef4 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 24 Dec 2024 17:32:20 +0000 Subject: [PATCH 14/23] update field names and fix tests --- coderd/database/dbmem/dbmem.go | 11 ++++++----- coderd/database/querier_test.go | 20 ++++++++++---------- coderd/database/queries.sql.go | 18 +++++++++--------- coderd/database/queries/insights.sql | 10 +++++----- coderd/insights.go | 4 ++-- 5 files changed, 32 insertions(+), 31 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index ca6a41e27266d..fa51ce181ace5 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5680,17 +5680,18 @@ func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUs if change.ChangedAt.Before(arg.StartTime) || change.ChangedAt.After(arg.EndTime) { continue } + date := time.Date(change.ChangedAt.Year(), change.ChangedAt.Month(), change.ChangedAt.Day(), 0, 0, 0, 0, time.UTC) if !slices.ContainsFunc(result, func(r database.GetUserStatusChangesRow) bool { - return r.ChangedAt.Equal(change.ChangedAt) && r.NewStatus == change.NewStatus + return r.Status == change.NewStatus && r.Date.Equal(date) }) { result = append(result, database.GetUserStatusChangesRow{ - NewStatus: change.NewStatus, - ChangedAt: change.ChangedAt, - Count: 1, + Status: change.NewStatus, + Date: date, + Count: 1, }) } else { for i, r := range result { - if r.ChangedAt.Equal(change.ChangedAt) && r.NewStatus == change.NewStatus { + if r.Status == change.NewStatus && r.Date.Equal(date) { result[i].Count++ break } diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 1c5d4778f6dcb..36ce5401cfc86 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2328,7 +2328,7 @@ func TestGetUserStatusChanges(t *testing.T) { // We should have an entry for each status change require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") for _, row := range userStatusChanges { - require.Equal(t, row.NewStatus, tc.status, "should have the correct status") + require.Equal(t, row.Status, tc.status, "should have the correct status") } }) } @@ -2422,10 +2422,10 @@ func TestGetUserStatusChanges(t *testing.T) { // We should have an entry for each status (active, dormant, suspended) for each day require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") for _, row := range userStatusChanges { - if row.ChangedAt.Before(firstTransitionTime) { - require.Equal(t, row.NewStatus, tc.initialStatus, "should have the initial status") + if row.Date.Before(firstTransitionTime) { + require.Equal(t, row.Status, tc.initialStatus, "should have the initial status") } else { - require.Equal(t, row.NewStatus, tc.targetStatus, "should have the target status") + require.Equal(t, row.Status, tc.targetStatus, "should have the target status") } } }) @@ -2638,12 +2638,12 @@ func TestGetUserStatusChanges(t *testing.T) { // Expected counts before, between and after the transitions should match: for _, row := range userStatusChanges { switch { - case row.ChangedAt.Before(firstTransitionTime): - require.Equal(t, row.Count, tc.expectedCounts["initial"][row.NewStatus], "should have the correct count before the first transition") - case row.ChangedAt.Before(secondTransitionTime): - require.Equal(t, row.Count, tc.expectedCounts["between"][row.NewStatus], "should have the correct count between the transitions") - case row.ChangedAt.Before(now): - require.Equal(t, row.Count, tc.expectedCounts["final"][row.NewStatus], "should have the correct count after the second transition") + case row.Date.Before(firstTransitionTime): + require.Equal(t, row.Count, tc.expectedCounts["initial"][row.Status], "should have the correct count before the first transition") + case row.Date.Before(secondTransitionTime): + require.Equal(t, row.Count, tc.expectedCounts["between"][row.Status], "should have the correct count between the transitions") + case row.Date.Before(now): + require.Equal(t, row.Count, tc.expectedCounts["final"][row.Status], "should have the correct count after the second transition") } } }) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7ebe377b948b5..3efa91a78b1ff 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3096,11 +3096,11 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate const getUserStatusChanges = `-- name: GetUserStatusChanges :many WITH dates AS ( - SELECT (generate_series( + SELECT generate_series( date_trunc('day', $1::timestamptz), date_trunc('day', $2::timestamptz), '1 day'::interval - ) AT TIME ZONE (extract(timezone FROM $1::timestamptz)::text || ' minutes'))::timestamptz AS date + )::timestamptz AS date ), latest_status_before_range AS ( -- Get the last status change for each user before our date range @@ -3148,13 +3148,13 @@ daily_counts AS ( GROUP BY d.date, asc1.new_status ) SELECT - date AS changed_at, - new_status, + date::timestamptz AS date, + new_status AS status, count FROM daily_counts WHERE count > 0 ORDER BY - new_status ASC, + status ASC, date ASC ` @@ -3164,9 +3164,9 @@ type GetUserStatusChangesParams struct { } type GetUserStatusChangesRow struct { - ChangedAt time.Time `db:"changed_at" json:"changed_at"` - NewStatus UserStatus `db:"new_status" json:"new_status"` - Count int64 `db:"count" json:"count"` + Date time.Time `db:"date" json:"date"` + Status UserStatus `db:"status" json:"status"` + Count int64 `db:"count" json:"count"` } func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]GetUserStatusChangesRow, error) { @@ -3178,7 +3178,7 @@ func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatus var items []GetUserStatusChangesRow for rows.Next() { var i GetUserStatusChangesRow - if err := rows.Scan(&i.ChangedAt, &i.NewStatus, &i.Count); err != nil { + if err := rows.Scan(&i.Date, &i.Status, &i.Count); err != nil { return nil, err } items = append(items, i) diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 18ea61de32773..698acbfba6605 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -774,11 +774,11 @@ GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.de -- name: GetUserStatusChanges :many WITH dates AS ( - SELECT (generate_series( + SELECT generate_series( date_trunc('day', @start_time::timestamptz), date_trunc('day', @end_time::timestamptz), '1 day'::interval - ) AT TIME ZONE (extract(timezone FROM @start_time::timestamptz)::text || ' minutes'))::timestamptz AS date + )::timestamptz AS date ), latest_status_before_range AS ( -- Get the last status change for each user before our date range @@ -826,11 +826,11 @@ daily_counts AS ( GROUP BY d.date, asc1.new_status ) SELECT - date AS changed_at, - new_status, + date::timestamptz AS date, + new_status AS status, count FROM daily_counts WHERE count > 0 ORDER BY - new_status ASC, + status ASC, date ASC; diff --git a/coderd/insights.go b/coderd/insights.go index dfa914425f965..3cd9a364a2efe 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -341,12 +341,12 @@ func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http } for _, row := range rows { - status := codersdk.UserStatus(row.NewStatus) + status := codersdk.UserStatus(row.Status) if _, ok := resp.StatusCounts[status]; !ok { resp.StatusCounts[status] = make([]codersdk.UserStatusChangeCount, 0) } resp.StatusCounts[status] = append(resp.StatusCounts[status], codersdk.UserStatusChangeCount{ - Date: row.ChangedAt, + Date: row.Date, Count: row.Count, }) } From 5b8fcac37707b01e86bde19d6325cc640e281b19 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 27 Dec 2024 12:17:23 +0000 Subject: [PATCH 15/23] exclude deleted users from the user status graph --- coderd/database/dump.sql | 27 +++++++++++++++++++ coderd/database/foreign_key_constraint.go | 1 + .../000280_user_status_changes.down.sql | 6 ++--- .../000280_user_status_changes.up.sql | 21 +++++++++++++++ coderd/database/models.go | 7 +++++ coderd/database/queries.sql.go | 8 +++++- coderd/database/queries/insights.sql | 8 +++++- coderd/database/unique_constraint.go | 1 + 8 files changed, 73 insertions(+), 6 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 404c3db9d605a..f2a0c6b9a1c46 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -432,6 +432,17 @@ BEGIN NEW.updated_at ); END IF; + + IF OLD.deleted = FALSE AND NEW.deleted = TRUE THEN + INSERT INTO user_deleted ( + user_id, + deleted_at + ) VALUES ( + NEW.id, + NEW.updated_at + ); + END IF; + RETURN NEW; END; $$; @@ -1390,6 +1401,14 @@ CREATE VIEW template_with_names AS COMMENT ON VIEW template_with_names IS 'Joins in the display name information such as username, avatar, and organization name.'; +CREATE TABLE user_deleted ( + id uuid DEFAULT gen_random_uuid() NOT NULL, + user_id uuid NOT NULL, + deleted_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL +); + +COMMENT ON TABLE user_deleted IS 'Tracks when users were deleted'; + CREATE TABLE user_links ( user_id uuid NOT NULL, login_type login_type NOT NULL, @@ -2001,6 +2020,9 @@ ALTER TABLE ONLY template_versions ALTER TABLE ONLY templates ADD CONSTRAINT templates_pkey PRIMARY KEY (id); +ALTER TABLE ONLY user_deleted + ADD CONSTRAINT user_deleted_pkey PRIMARY KEY (id); + ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_pkey PRIMARY KEY (user_id, login_type); @@ -2117,6 +2139,8 @@ CREATE INDEX idx_tailnet_tunnels_dst_id ON tailnet_tunnels USING hash (dst_id); CREATE INDEX idx_tailnet_tunnels_src_id ON tailnet_tunnels USING hash (src_id); +CREATE INDEX idx_user_deleted_deleted_at ON user_deleted USING btree (deleted_at); + CREATE INDEX idx_user_status_changes_changed_at ON user_status_changes USING btree (changed_at); CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted = false); @@ -2386,6 +2410,9 @@ ALTER TABLE ONLY templates ALTER TABLE ONLY templates ADD CONSTRAINT templates_organization_id_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; +ALTER TABLE ONLY user_deleted + ADD CONSTRAINT user_deleted_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id); + ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_oauth_access_token_key_id_fkey FOREIGN KEY (oauth_access_token_key_id) REFERENCES dbcrypt_keys(active_key_digest); diff --git a/coderd/database/foreign_key_constraint.go b/coderd/database/foreign_key_constraint.go index 18c82b83750fa..52f98a679a71b 100644 --- a/coderd/database/foreign_key_constraint.go +++ b/coderd/database/foreign_key_constraint.go @@ -47,6 +47,7 @@ const ( ForeignKeyTemplateVersionsTemplateID ForeignKeyConstraint = "template_versions_template_id_fkey" // ALTER TABLE ONLY template_versions ADD CONSTRAINT template_versions_template_id_fkey FOREIGN KEY (template_id) REFERENCES templates(id) ON DELETE CASCADE; ForeignKeyTemplatesCreatedBy ForeignKeyConstraint = "templates_created_by_fkey" // ALTER TABLE ONLY templates ADD CONSTRAINT templates_created_by_fkey FOREIGN KEY (created_by) REFERENCES users(id) ON DELETE RESTRICT; ForeignKeyTemplatesOrganizationID ForeignKeyConstraint = "templates_organization_id_fkey" // ALTER TABLE ONLY templates ADD CONSTRAINT templates_organization_id_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; + ForeignKeyUserDeletedUserID ForeignKeyConstraint = "user_deleted_user_id_fkey" // ALTER TABLE ONLY user_deleted ADD CONSTRAINT user_deleted_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id); ForeignKeyUserLinksOauthAccessTokenKeyID ForeignKeyConstraint = "user_links_oauth_access_token_key_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_oauth_access_token_key_id_fkey FOREIGN KEY (oauth_access_token_key_id) REFERENCES dbcrypt_keys(active_key_digest); ForeignKeyUserLinksOauthRefreshTokenKeyID ForeignKeyConstraint = "user_links_oauth_refresh_token_key_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_oauth_refresh_token_key_id_fkey FOREIGN KEY (oauth_refresh_token_key_id) REFERENCES dbcrypt_keys(active_key_digest); ForeignKeyUserLinksUserID ForeignKeyConstraint = "user_links_user_id_fkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000280_user_status_changes.down.sql b/coderd/database/migrations/000280_user_status_changes.down.sql index 30514ef467ebe..fbe85a6be0fe5 100644 --- a/coderd/database/migrations/000280_user_status_changes.down.sql +++ b/coderd/database/migrations/000280_user_status_changes.down.sql @@ -1,11 +1,9 @@ --- Drop the trigger first DROP TRIGGER IF EXISTS user_status_change_trigger ON users; --- Drop the trigger function DROP FUNCTION IF EXISTS record_user_status_change(); --- Drop the indexes DROP INDEX IF EXISTS idx_user_status_changes_changed_at; +DROP INDEX IF EXISTS idx_user_deleted_deleted_at; --- Drop the table DROP TABLE IF EXISTS user_status_changes; +DROP TABLE IF EXISTS user_deleted; diff --git a/coderd/database/migrations/000280_user_status_changes.up.sql b/coderd/database/migrations/000280_user_status_changes.up.sql index f16164862b3e5..04d8472e55460 100644 --- a/coderd/database/migrations/000280_user_status_changes.up.sql +++ b/coderd/database/migrations/000280_user_status_changes.up.sql @@ -21,6 +21,16 @@ SELECT FROM users WHERE NOT deleted; +CREATE TABLE user_deleted ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + user_id uuid NOT NULL REFERENCES users(id), + deleted_at timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +COMMENT ON TABLE user_deleted IS 'Tracks when users were deleted'; + +CREATE INDEX idx_user_deleted_deleted_at ON user_deleted(deleted_at); + CREATE OR REPLACE FUNCTION record_user_status_change() RETURNS trigger AS $$ BEGIN IF TG_OP = 'INSERT' OR OLD.status IS DISTINCT FROM NEW.status THEN @@ -34,6 +44,17 @@ BEGIN NEW.updated_at ); END IF; + + IF OLD.deleted = FALSE AND NEW.deleted = TRUE THEN + INSERT INTO user_deleted ( + user_id, + deleted_at + ) VALUES ( + NEW.id, + NEW.updated_at + ); + END IF; + RETURN NEW; END; $$ LANGUAGE plpgsql; diff --git a/coderd/database/models.go b/coderd/database/models.go index 81b56370ad2ea..7d25b64318a87 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2892,6 +2892,13 @@ type User struct { OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` } +// Tracks when users were deleted +type UserDeleted struct { + ID uuid.UUID `db:"id" json:"id"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + DeletedAt time.Time `db:"deleted_at" json:"deleted_at"` +} + type UserLink struct { UserID uuid.UUID `db:"user_id" json:"user_id"` LoginType LoginType `db:"login_type" json:"login_type"` diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3efa91a78b1ff..cbcc8792e632f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3134,7 +3134,7 @@ daily_counts AS ( d.date, asc1.new_status, -- For each date and status, count users whose most recent status change - -- (up to that date) matches this status + -- (up to that date) matches this status AND who weren't deleted by that date COUNT(*) FILTER ( WHERE asc1.changed_at = ( SELECT MAX(changed_at) @@ -3142,6 +3142,12 @@ daily_counts AS ( WHERE asc2.user_id = asc1.user_id AND asc2.changed_at <= d.date ) + AND NOT EXISTS ( + SELECT 1 + FROM user_deleted ud + WHERE ud.user_id = asc1.user_id + AND ud.deleted_at <= d.date + ) )::bigint AS count FROM dates d CROSS JOIN all_status_changes asc1 diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 698acbfba6605..04e74bf7f166e 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -812,7 +812,7 @@ daily_counts AS ( d.date, asc1.new_status, -- For each date and status, count users whose most recent status change - -- (up to that date) matches this status + -- (up to that date) matches this status AND who weren't deleted by that date COUNT(*) FILTER ( WHERE asc1.changed_at = ( SELECT MAX(changed_at) @@ -820,6 +820,12 @@ daily_counts AS ( WHERE asc2.user_id = asc1.user_id AND asc2.changed_at <= d.date ) + AND NOT EXISTS ( + SELECT 1 + FROM user_deleted ud + WHERE ud.user_id = asc1.user_id + AND ud.deleted_at <= d.date + ) )::bigint AS count FROM dates d CROSS JOIN all_status_changes asc1 diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index b59cb0cbc8091..f253aa98ec266 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -62,6 +62,7 @@ const ( UniqueTemplateVersionsPkey UniqueConstraint = "template_versions_pkey" // ALTER TABLE ONLY template_versions ADD CONSTRAINT template_versions_pkey PRIMARY KEY (id); UniqueTemplateVersionsTemplateIDNameKey UniqueConstraint = "template_versions_template_id_name_key" // ALTER TABLE ONLY template_versions ADD CONSTRAINT template_versions_template_id_name_key UNIQUE (template_id, name); UniqueTemplatesPkey UniqueConstraint = "templates_pkey" // ALTER TABLE ONLY templates ADD CONSTRAINT templates_pkey PRIMARY KEY (id); + UniqueUserDeletedPkey UniqueConstraint = "user_deleted_pkey" // ALTER TABLE ONLY user_deleted ADD CONSTRAINT user_deleted_pkey PRIMARY KEY (id); UniqueUserLinksPkey UniqueConstraint = "user_links_pkey" // ALTER TABLE ONLY user_links ADD CONSTRAINT user_links_pkey PRIMARY KEY (user_id, login_type); UniqueUserStatusChangesPkey UniqueConstraint = "user_status_changes_pkey" // ALTER TABLE ONLY user_status_changes ADD CONSTRAINT user_status_changes_pkey PRIMARY KEY (id); UniqueUsersPkey UniqueConstraint = "users_pkey" // ALTER TABLE ONLY users ADD CONSTRAINT users_pkey PRIMARY KEY (id); From 4ac83aa1ee49ba5b4dafe4fc5aa803438021b6c9 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 2 Jan 2025 17:18:42 +0000 Subject: [PATCH 16/23] GetUserStatusChanges now passes all querier tests --- Makefile | 5 +- coderd/database/querier_test.go | 803 ++++++++++++++++----------- coderd/database/queries.sql.go | 113 ++-- coderd/database/queries/insights.sql | 114 ++-- 4 files changed, 601 insertions(+), 434 deletions(-) diff --git a/Makefile b/Makefile index bc109983ca695..748b4b4823092 100644 --- a/Makefile +++ b/Makefile @@ -500,6 +500,7 @@ lint/helm: # All files generated by the database should be added here, and this can be used # as a target for jobs that need to run after the database is generated. DB_GEN_FILES := \ + coderd/database/dump.sql \ coderd/database/querier.go \ coderd/database/unique_constraint.go \ coderd/database/dbmem/dbmem.go \ @@ -519,8 +520,6 @@ GEN_FILES := \ provisionersdk/proto/provisioner.pb.go \ provisionerd/proto/provisionerd.pb.go \ vpn/vpn.pb.go \ - coderd/database/dump.sql \ - $(DB_GEN_FILES) \ site/src/api/typesGenerated.ts \ coderd/rbac/object_gen.go \ codersdk/rbacresources_gen.go \ @@ -540,7 +539,7 @@ GEN_FILES := \ coderd/database/pubsub/psmock/psmock.go # all gen targets should be added here and to gen/mark-fresh -gen: $(GEN_FILES) +gen: gen/db $(GEN_FILES) .PHONY: gen gen/db: $(DB_GEN_FILES) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 36ce5401cfc86..6ace33f74a96c 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -7,6 +7,7 @@ import ( "database/sql" "encoding/json" "fmt" + "maps" "sort" "testing" "time" @@ -2258,397 +2259,525 @@ func TestGroupRemovalTrigger(t *testing.T) { func TestGetUserStatusChanges(t *testing.T) { t.Parallel() - now := dbtime.Now() - createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago - firstTransitionTime := createdAt.Add(2 * 24 * time.Hour) // 3 days ago - secondTransitionTime := firstTransitionTime.Add(2 * 24 * time.Hour) // 1 days ago - t.Run("No Users", func(t *testing.T) { - t.Parallel() - db, _ := dbtestutil.NewDB(t) - ctx := testutil.Context(t, testutil.WaitShort) - - end := dbtime.Now() - start := end.Add(-30 * 24 * time.Hour) + if !dbtestutil.WillUsePostgres() { + t.SkipNow() + } - counts, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ - StartTime: start, - EndTime: end, - }) - require.NoError(t, err) - require.Empty(t, counts, "should return no results when there are no users") - }) + timezones := []string{ + "Canada/Newfoundland", + "Africa/Johannesburg", + "America/New_York", + "Europe/London", + "Asia/Tokyo", + "Australia/Sydney", + } - t.Run("Single User/Single State", func(t *testing.T) { - t.Parallel() + for _, tz := range timezones { + tz := tz + t.Run(tz, func(t *testing.T) { + t.Parallel() - testCases := []struct { - name string - status database.UserStatus - }{ - { - name: "Active Only", - status: database.UserStatusActive, - }, - { - name: "Dormant Only", - status: database.UserStatusDormant, - }, - { - name: "Suspended Only", - status: database.UserStatusSuspended, - }, - // { - // name: "Deleted Only", - // status: database.UserStatusDeleted, - // }, - } + 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) - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { + t.Run("No Users", 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, - }) + end := dbtime.Now() + start := end.Add(-30 * 24 * time.Hour) - // Query for the last 30 days - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ - StartTime: createdAt, - EndTime: now, + counts, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: start, + EndTime: end, }) require.NoError(t, err) - require.NotEmpty(t, userStatusChanges, "should return results") - - // We should have an entry for each status change - require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") - for _, row := range userStatusChanges { - require.Equal(t, row.Status, tc.status, "should have the correct status") - } + require.Empty(t, counts, "should return no results when there are no users") }) - } - }) - - t.Run("Single Transition", func(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - initialStatus database.UserStatus - targetStatus database.UserStatus - }{ - { - name: "Active to Dormant", - initialStatus: database.UserStatusActive, - targetStatus: database.UserStatusDormant, - }, - { - name: "Active to Suspended", - initialStatus: database.UserStatusActive, - targetStatus: database.UserStatusSuspended, - }, - // { - // name: "Active to Deleted", - // initialStatus: database.UserStatusActive, - // targetStatus: database.UserStatusDeleted, - // }, - { - name: "Dormant to Active", - initialStatus: database.UserStatusDormant, - targetStatus: database.UserStatusActive, - }, - { - name: "Dormant to Suspended", - initialStatus: database.UserStatusDormant, - targetStatus: database.UserStatusSuspended, - }, - // { - // name: "Dormant to Deleted", - // initialStatus: database.UserStatusDormant, - // targetStatus: database.UserStatusDeleted, - // }, - { - name: "Suspended to Active", - initialStatus: database.UserStatusSuspended, - targetStatus: database.UserStatusActive, - }, - { - name: "Suspended to Dormant", - initialStatus: database.UserStatusSuspended, - targetStatus: database.UserStatusDormant, - }, - // { - // name: "Suspended to Deleted", - // initialStatus: database.UserStatusSuspended, - // targetStatus: database.UserStatusDeleted, - // }, - } - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { + t.Run("One User/Creation Only", 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, - }) - - // After 2 days, change status to target status - user, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: user.ID, - Status: tc.targetStatus, - UpdatedAt: firstTransitionTime, - }) - require.NoError(t, err) + testCases := []struct { + name string + status database.UserStatus + }{ + { + name: "Active Only", + status: database.UserStatusActive, + }, + { + name: "Dormant Only", + status: database.UserStatusDormant, + }, + { + name: "Suspended Only", + status: database.UserStatusSuspended, + }, + // { + // name: "Deleted Only", + // status: database.UserStatusDeleted, + // }, + } - // Query for the last 5 days - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ - StartTime: createdAt, - EndTime: now, - }) - require.NoError(t, err) - require.NotEmpty(t, userStatusChanges, "should return results") - - // We should have an entry for each status (active, dormant, suspended) for each day - require.Len(t, userStatusChanges, 5, "should have 1 user * 5 days = 5 rows") - for _, row := range userStatusChanges { - if row.Date.Before(firstTransitionTime) { - require.Equal(t, row.Status, tc.initialStatus, "should have the initial status") - } else { - require.Equal(t, row.Status, tc.targetStatus, "should have the target status") - } + for _, tc := range testCases { + tc := tc + t.Run(tc.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, + }) + + // Query for the last 30 days + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: createdAt, + EndTime: today, + }) + require.NoError(t, err) + require.NotEmpty(t, userStatusChanges, "should return results") + + require.Len(t, userStatusChanges, 2, "should have 1 entry per status change plus and 1 entry for the end of the range = 2 entries") + + require.Equal(t, userStatusChanges[0].Status, tc.status, "should have the correct status") + require.Equal(t, userStatusChanges[0].Count, int64(1), "should have 1 user") + require.True(t, userStatusChanges[0].Date.Equal(createdAt), "should have the correct date") + + require.Equal(t, userStatusChanges[1].Status, tc.status, "should have the correct status") + require.Equal(t, userStatusChanges[1].Count, int64(1), "should have 1 user") + require.True(t, userStatusChanges[1].Date.Equal(today), "should have the correct date") + }) } }) - } - }) - t.Run("Two Users Transitioning", func(t *testing.T) { - t.Parallel() - - type transition struct { - from database.UserStatus - to database.UserStatus - } - - type testCase struct { - name string - user1Transition transition - user2Transition transition - expectedCounts map[string]map[database.UserStatus]int64 - } + t.Run("One User/One Transition", func(t *testing.T) { + t.Parallel() - testCases := []testCase{ - { - name: "Active->Dormant and Dormant->Suspended", - user1Transition: transition{ - from: database.UserStatusActive, - to: database.UserStatusDormant, - }, - user2Transition: transition{ - from: database.UserStatusDormant, - to: database.UserStatusSuspended, - }, - expectedCounts: map[string]map[database.UserStatus]int64{ - "initial": { - database.UserStatusActive: 1, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 0, - }, - "between": { - database.UserStatusActive: 0, - database.UserStatusDormant: 2, - database.UserStatusSuspended: 0, - }, - "final": { - database.UserStatusActive: 0, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 1, - }, - }, - }, - { - name: "Suspended->Active and Active->Dormant", - user1Transition: transition{ - from: database.UserStatusSuspended, - to: database.UserStatusActive, - }, - user2Transition: transition{ - from: database.UserStatusActive, - to: database.UserStatusDormant, - }, - expectedCounts: map[string]map[database.UserStatus]int64{ - "initial": { - database.UserStatusActive: 1, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 1, - }, - "between": { - database.UserStatusActive: 2, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 0, + testCases := []struct { + name string + initialStatus database.UserStatus + targetStatus database.UserStatus + expectedCounts map[time.Time]map[database.UserStatus]int64 + }{ + { + name: "Active to Dormant", + initialStatus: database.UserStatusActive, + targetStatus: database.UserStatusDormant, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + }, + firstTransitionTime: { + database.UserStatusDormant: 1, + database.UserStatusActive: 0, + }, + today: { + database.UserStatusDormant: 1, + database.UserStatusActive: 0, + }, + }, }, - "final": { - database.UserStatusActive: 1, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 0, + { + name: "Active to Suspended", + initialStatus: database.UserStatusActive, + targetStatus: database.UserStatusSuspended, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusActive: 1, + database.UserStatusSuspended: 0, + }, + firstTransitionTime: { + database.UserStatusSuspended: 1, + database.UserStatusActive: 0, + }, + today: { + database.UserStatusSuspended: 1, + database.UserStatusActive: 0, + }, + }, }, - }, - }, - { - name: "Dormant->Active and Suspended->Dormant", - user1Transition: transition{ - from: database.UserStatusDormant, - to: database.UserStatusActive, - }, - user2Transition: transition{ - from: database.UserStatusSuspended, - to: database.UserStatusDormant, - }, - expectedCounts: map[string]map[database.UserStatus]int64{ - "initial": { - database.UserStatusActive: 0, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 1, + { + name: "Dormant to Active", + initialStatus: database.UserStatusDormant, + targetStatus: database.UserStatusActive, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusDormant: 1, + database.UserStatusActive: 0, + }, + firstTransitionTime: { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + }, + today: { + database.UserStatusActive: 1, + database.UserStatusDormant: 0, + }, + }, }, - "between": { - database.UserStatusActive: 1, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 1, + { + name: "Dormant to Suspended", + initialStatus: database.UserStatusDormant, + targetStatus: database.UserStatusSuspended, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + firstTransitionTime: { + database.UserStatusSuspended: 1, + database.UserStatusDormant: 0, + }, + today: { + database.UserStatusSuspended: 1, + database.UserStatusDormant: 0, + }, + }, }, - "final": { - database.UserStatusActive: 1, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 0, + { + name: "Suspended to Active", + initialStatus: database.UserStatusSuspended, + targetStatus: database.UserStatusActive, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusSuspended: 1, + database.UserStatusActive: 0, + }, + firstTransitionTime: { + database.UserStatusActive: 1, + database.UserStatusSuspended: 0, + }, + today: { + database.UserStatusActive: 1, + database.UserStatusSuspended: 0, + }, + }, }, - }, - }, - { - name: "Active->Suspended and Suspended->Active", - user1Transition: transition{ - from: database.UserStatusActive, - to: database.UserStatusSuspended, - }, - user2Transition: transition{ - from: database.UserStatusSuspended, - to: database.UserStatusActive, - }, - expectedCounts: map[string]map[database.UserStatus]int64{ - "initial": { - database.UserStatusActive: 1, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 1, + { + name: "Suspended to Dormant", + initialStatus: database.UserStatusSuspended, + targetStatus: database.UserStatusDormant, + expectedCounts: map[time.Time]map[database.UserStatus]int64{ + createdAt: { + database.UserStatusSuspended: 1, + database.UserStatusDormant: 0, + }, + firstTransitionTime: { + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + today: { + database.UserStatusDormant: 1, + database.UserStatusSuspended: 0, + }, + }, }, - "between": { - database.UserStatusActive: 0, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 2, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.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, + }) + + // After 2 days, change status to target status + user, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: user.ID, + Status: tc.targetStatus, + UpdatedAt: firstTransitionTime, + }) + require.NoError(t, err) + + // Query for the last 5 days + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: createdAt, + EndTime: today, + }) + require.NoError(t, err) + require.NotEmpty(t, userStatusChanges, "should return results") + + gotCounts := map[time.Time]map[database.UserStatus]int64{} + for _, row := range userStatusChanges { + gotDateInLocation := row.Date.In(location) + if _, ok := gotCounts[gotDateInLocation]; !ok { + gotCounts[gotDateInLocation] = map[database.UserStatus]int64{} + } + if _, ok := gotCounts[gotDateInLocation][row.Status]; !ok { + gotCounts[gotDateInLocation][row.Status] = 0 + } + gotCounts[gotDateInLocation][row.Status] += row.Count + } + require.Equal(t, tc.expectedCounts, gotCounts) + }) + } + }) + + t.Run("Two Users/One Transition", func(t *testing.T) { + t.Parallel() + + type transition struct { + from database.UserStatus + to database.UserStatus + } + + type testCase struct { + name string + user1Transition transition + user2Transition transition + } + + testCases := []testCase{ + { + name: "Active->Dormant and Dormant->Suspended", + user1Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusDormant, + }, + user2Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusSuspended, + }, }, - "final": { - database.UserStatusActive: 1, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 1, + { + name: "Suspended->Active and Active->Dormant", + user1Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusActive, + }, + user2Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusDormant, + }, }, - }, - }, - { - name: "Dormant->Suspended and Dormant->Active", - user1Transition: transition{ - from: database.UserStatusDormant, - to: database.UserStatusSuspended, - }, - user2Transition: transition{ - from: database.UserStatusDormant, - to: database.UserStatusActive, - }, - expectedCounts: map[string]map[database.UserStatus]int64{ - "initial": { - database.UserStatusActive: 0, - database.UserStatusDormant: 2, - database.UserStatusSuspended: 0, + { + name: "Dormant->Active and Suspended->Dormant", + user1Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusActive, + }, + user2Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusDormant, + }, }, - "between": { - database.UserStatusActive: 0, - database.UserStatusDormant: 1, - database.UserStatusSuspended: 1, + { + name: "Active->Suspended and Suspended->Active", + user1Transition: transition{ + from: database.UserStatusActive, + to: database.UserStatusSuspended, + }, + user2Transition: transition{ + from: database.UserStatusSuspended, + to: database.UserStatusActive, + }, }, - "final": { - database.UserStatusActive: 1, - database.UserStatusDormant: 0, - database.UserStatusSuspended: 1, + { + name: "Dormant->Suspended and Dormant->Active", + user1Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusSuspended, + }, + user2Transition: transition{ + from: database.UserStatusDormant, + to: database.UserStatusActive, + }, }, - }, - }, - } + } - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.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, + }) + user2 := dbgen.User(t, db, database.User{ + Status: tc.user2Transition.from, + CreatedAt: createdAt, + UpdatedAt: createdAt, + }) + + // First transition at 2 days + user1, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: user1.ID, + Status: tc.user1Transition.to, + UpdatedAt: firstTransitionTime, + }) + 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, + }) + require.NoError(t, err) + + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: createdAt, + EndTime: today, + }) + require.NoError(t, err) + require.NotEmpty(t, userStatusChanges) + gotCounts := map[time.Time]map[database.UserStatus]int64{ + createdAt.In(location): {}, + firstTransitionTime.In(location): {}, + secondTransitionTime.In(location): {}, + today.In(location): {}, + } + for _, row := range userStatusChanges { + dateInLocation := row.Date.In(location) + switch { + case dateInLocation.Equal(createdAt.In(location)): + gotCounts[createdAt][row.Status] = row.Count + case dateInLocation.Equal(firstTransitionTime.In(location)): + gotCounts[firstTransitionTime][row.Status] = row.Count + case dateInLocation.Equal(secondTransitionTime.In(location)): + gotCounts[secondTransitionTime][row.Status] = row.Count + case dateInLocation.Equal(today.In(location)): + gotCounts[today][row.Status] = row.Count + default: + t.Fatalf("unexpected date %s", row.Date) + } + } + + expectedCounts := map[time.Time]map[database.UserStatus]int64{} + for _, status := range []database.UserStatus{ + tc.user1Transition.from, + tc.user1Transition.to, + tc.user2Transition.from, + tc.user2Transition.to, + } { + if _, ok := expectedCounts[createdAt]; !ok { + expectedCounts[createdAt] = map[database.UserStatus]int64{} + } + expectedCounts[createdAt][status] = 0 + } + + expectedCounts[createdAt][tc.user1Transition.from]++ + expectedCounts[createdAt][tc.user2Transition.from]++ + + expectedCounts[firstTransitionTime] = map[database.UserStatus]int64{} + maps.Copy(expectedCounts[firstTransitionTime], expectedCounts[createdAt]) + expectedCounts[firstTransitionTime][tc.user1Transition.from]-- + expectedCounts[firstTransitionTime][tc.user1Transition.to]++ + + expectedCounts[secondTransitionTime] = map[database.UserStatus]int64{} + maps.Copy(expectedCounts[secondTransitionTime], expectedCounts[firstTransitionTime]) + expectedCounts[secondTransitionTime][tc.user2Transition.from]-- + expectedCounts[secondTransitionTime][tc.user2Transition.to]++ + + expectedCounts[today] = map[database.UserStatus]int64{} + maps.Copy(expectedCounts[today], expectedCounts[secondTransitionTime]) + + require.Equal(t, expectedCounts[createdAt], gotCounts[createdAt]) + require.Equal(t, expectedCounts[firstTransitionTime], gotCounts[firstTransitionTime]) + require.Equal(t, expectedCounts[secondTransitionTime], gotCounts[secondTransitionTime]) + require.Equal(t, expectedCounts[today], gotCounts[today]) + }) + } + }) + + t.Run("User precedes and survives query range", func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) ctx := testutil.Context(t, testutil.WaitShort) - now := dbtime.Now() - createdAt := now.Add(-5 * 24 * time.Hour) // 5 days ago - - user1 := dbgen.User(t, db, database.User{ - Status: tc.user1Transition.from, + _ = dbgen.User(t, db, database.User{ + Status: database.UserStatusActive, CreatedAt: createdAt, UpdatedAt: createdAt, }) - user2 := dbgen.User(t, db, database.User{ - Status: tc.user2Transition.from, + + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: createdAt.Add(time.Hour * 24), + EndTime: today, + }) + require.NoError(t, err) + + require.Len(t, userStatusChanges, 2) + require.Equal(t, userStatusChanges[0].Count, int64(1)) + require.Equal(t, userStatusChanges[0].Status, database.UserStatusActive) + require.Equal(t, userStatusChanges[1].Count, int64(1)) + require.Equal(t, userStatusChanges[1].Status, database.UserStatusActive) + }) + + t.Run("User deleted before query range", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitShort) + + user := dbgen.User(t, db, database.User{ + Status: database.UserStatusActive, CreatedAt: createdAt, UpdatedAt: createdAt, }) - // First transition at 2 days - user1, err := db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: user1.ID, - Status: tc.user1Transition.to, - UpdatedAt: firstTransitionTime, + err = db.UpdateUserDeletedByID(ctx, user.ID) + require.NoError(t, err) + + userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + StartTime: today.Add(time.Hour * 24), + EndTime: today.Add(time.Hour * 48), }) require.NoError(t, err) + require.Empty(t, userStatusChanges) + }) + + t.Run("User deleted during query range", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitShort) - // Second transition at 4 days - user2, err = db.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: user2.ID, - Status: tc.user2Transition.to, - UpdatedAt: secondTransitionTime, + user := dbgen.User(t, db, database.User{ + Status: database.UserStatusActive, + CreatedAt: createdAt, + UpdatedAt: createdAt, }) + + err := db.UpdateUserDeletedByID(ctx, user.ID) require.NoError(t, err) - // Query for the last 5 days userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ StartTime: createdAt, - EndTime: now, + EndTime: today.Add(time.Hour * 24), }) require.NoError(t, err) - require.NotEmpty(t, userStatusChanges) - - // Expected counts before, between and after the transitions should match: - for _, row := range userStatusChanges { - switch { - case row.Date.Before(firstTransitionTime): - require.Equal(t, row.Count, tc.expectedCounts["initial"][row.Status], "should have the correct count before the first transition") - case row.Date.Before(secondTransitionTime): - require.Equal(t, row.Count, tc.expectedCounts["between"][row.Status], "should have the correct count between the transitions") - case row.Date.Before(now): - require.Equal(t, row.Count, tc.expectedCounts["final"][row.Status], "should have the correct count after the second transition") - } - } + require.Equal(t, userStatusChanges[0].Count, int64(1)) + require.Equal(t, userStatusChanges[0].Status, database.UserStatusActive) + require.Equal(t, userStatusChanges[1].Count, int64(0)) + require.Equal(t, userStatusChanges[1].Status, database.UserStatusActive) + require.Equal(t, userStatusChanges[2].Count, int64(0)) + require.Equal(t, userStatusChanges[2].Status, database.UserStatusActive) }) - } - }) + }) + } } func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index cbcc8792e632f..38a87874d7f18 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3096,24 +3096,49 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate const getUserStatusChanges = `-- name: GetUserStatusChanges :many WITH dates AS ( - SELECT generate_series( - date_trunc('day', $1::timestamptz), - date_trunc('day', $2::timestamptz), - '1 day'::interval - )::timestamptz AS date + SELECT $1::timestamptz AS date + + UNION + + SELECT DISTINCT changed_at AS date + FROM user_status_changes + WHERE changed_at > $1::timestamptz + AND changed_at < $2::timestamptz + + UNION + + SELECT DISTINCT deleted_at AS date + FROM user_deleted + WHERE deleted_at > $1::timestamptz + AND deleted_at < $2::timestamptz + + UNION + + SELECT $2::timestamptz AS date ), latest_status_before_range AS ( - -- Get the last status change for each user before our date range - SELECT DISTINCT ON (user_id) - user_id, - new_status, - changed_at - FROM user_status_changes - WHERE changed_at < (SELECT MIN(date) FROM dates) - ORDER BY user_id, changed_at DESC + SELECT + DISTINCT usc.user_id, + usc.new_status, + usc.changed_at + FROM user_status_changes usc + LEFT JOIN user_deleted ud ON usc.user_id = ud.user_id + WHERE usc.changed_at < $1::timestamptz + AND (ud.user_id IS NULL OR ud.deleted_at > $1::timestamptz) + ORDER BY usc.user_id, usc.changed_at DESC +), +status_changes_during_range AS ( + SELECT + usc.user_id, + usc.new_status, + usc.changed_at + FROM user_status_changes usc + LEFT JOIN user_deleted ud ON usc.user_id = ud.user_id + WHERE usc.changed_at >= $1::timestamptz + AND usc.changed_at <= $2::timestamptz + AND (ud.user_id IS NULL OR usc.changed_at < ud.deleted_at) ), all_status_changes AS ( - -- Combine status changes before and during our range SELECT user_id, new_status, @@ -3126,42 +3151,36 @@ all_status_changes AS ( user_id, new_status, changed_at - FROM user_status_changes - WHERE changed_at < $2::timestamptz + FROM status_changes_during_range ), -daily_counts AS ( - SELECT - d.date, - asc1.new_status, - -- For each date and status, count users whose most recent status change - -- (up to that date) matches this status AND who weren't deleted by that date - COUNT(*) FILTER ( - WHERE asc1.changed_at = ( - SELECT MAX(changed_at) - FROM all_status_changes asc2 - WHERE asc2.user_id = asc1.user_id - AND asc2.changed_at <= d.date - ) - AND NOT EXISTS ( - SELECT 1 - FROM user_deleted ud - WHERE ud.user_id = asc1.user_id - AND ud.deleted_at <= d.date - ) - )::bigint AS count - FROM dates d - CROSS JOIN all_status_changes asc1 - GROUP BY d.date, asc1.new_status +ranked_status_change_per_user_per_date AS ( + SELECT + d.date, + asc1.user_id, + ROW_NUMBER() OVER (PARTITION BY d.date, asc1.user_id ORDER BY asc1.changed_at DESC) AS rn, + asc1.new_status + FROM dates d + LEFT JOIN all_status_changes asc1 ON asc1.changed_at <= d.date ) SELECT - date::timestamptz AS date, - new_status AS status, - count -FROM daily_counts -WHERE count > 0 -ORDER BY - status ASC, - date ASC + date, + statuses.new_status AS status, + COUNT(rscpupd.user_id) FILTER ( + WHERE rscpupd.rn = 1 + AND ( + rscpupd.new_status = statuses.new_status + AND ( + -- Include users who haven't been deleted + NOT EXISTS (SELECT 1 FROM user_deleted WHERE user_id = rscpupd.user_id) + OR + -- Or users whose deletion date is after the current date we're looking at + rscpupd.date < (SELECT deleted_at FROM user_deleted WHERE user_id = rscpupd.user_id) + ) + ) + ) AS count +FROM ranked_status_change_per_user_per_date rscpupd +CROSS JOIN (select distinct new_status FROM all_status_changes) statuses +GROUP BY date, statuses.new_status ` type GetUserStatusChangesParams struct { diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 04e74bf7f166e..c721ebcc79227 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -774,24 +774,49 @@ GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.de -- name: GetUserStatusChanges :many WITH dates AS ( - SELECT generate_series( - date_trunc('day', @start_time::timestamptz), - date_trunc('day', @end_time::timestamptz), - '1 day'::interval - )::timestamptz AS date + SELECT @start_time::timestamptz AS date + + UNION + + SELECT DISTINCT changed_at AS date + FROM user_status_changes + WHERE changed_at > @start_time::timestamptz + AND changed_at < @end_time::timestamptz + + UNION + + SELECT DISTINCT deleted_at AS date + FROM user_deleted + WHERE deleted_at > @start_time::timestamptz + AND deleted_at < @end_time::timestamptz + + UNION + + SELECT @end_time::timestamptz AS date ), latest_status_before_range AS ( - -- Get the last status change for each user before our date range - SELECT DISTINCT ON (user_id) - user_id, - new_status, - changed_at - FROM user_status_changes - WHERE changed_at < (SELECT MIN(date) FROM dates) - ORDER BY user_id, changed_at DESC + SELECT + DISTINCT usc.user_id, + usc.new_status, + usc.changed_at + FROM user_status_changes usc + LEFT JOIN user_deleted ud ON usc.user_id = ud.user_id + WHERE usc.changed_at < @start_time::timestamptz + AND (ud.user_id IS NULL OR ud.deleted_at > @start_time::timestamptz) + ORDER BY usc.user_id, usc.changed_at DESC +), +status_changes_during_range AS ( + SELECT + usc.user_id, + usc.new_status, + usc.changed_at + FROM user_status_changes usc + LEFT JOIN user_deleted ud ON usc.user_id = ud.user_id + WHERE usc.changed_at >= @start_time::timestamptz + AND usc.changed_at <= @end_time::timestamptz + AND (ud.user_id IS NULL OR usc.changed_at < ud.deleted_at) ), all_status_changes AS ( - -- Combine status changes before and during our range SELECT user_id, new_status, @@ -804,39 +829,34 @@ all_status_changes AS ( user_id, new_status, changed_at - FROM user_status_changes - WHERE changed_at < @end_time::timestamptz + FROM status_changes_during_range ), -daily_counts AS ( - SELECT - d.date, - asc1.new_status, - -- For each date and status, count users whose most recent status change - -- (up to that date) matches this status AND who weren't deleted by that date - COUNT(*) FILTER ( - WHERE asc1.changed_at = ( - SELECT MAX(changed_at) - FROM all_status_changes asc2 - WHERE asc2.user_id = asc1.user_id - AND asc2.changed_at <= d.date - ) - AND NOT EXISTS ( - SELECT 1 - FROM user_deleted ud - WHERE ud.user_id = asc1.user_id - AND ud.deleted_at <= d.date - ) - )::bigint AS count - FROM dates d - CROSS JOIN all_status_changes asc1 - GROUP BY d.date, asc1.new_status +ranked_status_change_per_user_per_date AS ( + SELECT + d.date, + asc1.user_id, + ROW_NUMBER() OVER (PARTITION BY d.date, asc1.user_id ORDER BY asc1.changed_at DESC) AS rn, + asc1.new_status + FROM dates d + LEFT JOIN all_status_changes asc1 ON asc1.changed_at <= d.date ) SELECT - date::timestamptz AS date, - new_status AS status, - count -FROM daily_counts -WHERE count > 0 -ORDER BY - status ASC, - date ASC; + date, + statuses.new_status AS status, + COUNT(rscpupd.user_id) FILTER ( + WHERE rscpupd.rn = 1 + AND ( + rscpupd.new_status = statuses.new_status + AND ( + -- Include users who haven't been deleted + NOT EXISTS (SELECT 1 FROM user_deleted WHERE user_id = rscpupd.user_id) + OR + -- Or users whose deletion date is after the current date we're looking at + rscpupd.date < (SELECT deleted_at FROM user_deleted WHERE user_id = rscpupd.user_id) + ) + ) + ) AS count +FROM ranked_status_change_per_user_per_date rscpupd +CROSS JOIN (select distinct new_status FROM all_status_changes) statuses +GROUP BY date, statuses.new_status; + From b23018aad54e6a1b772104f62a2d9374e8f9b582 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 2 Jan 2025 17:31:57 +0000 Subject: [PATCH 17/23] renumber migrations --- ...tatus_changes.down.sql => 000281_user_status_changes.down.sql} | 0 ...er_status_changes.up.sql => 000281_user_status_changes.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000280_user_status_changes.down.sql => 000281_user_status_changes.down.sql} (100%) rename coderd/database/migrations/{000280_user_status_changes.up.sql => 000281_user_status_changes.up.sql} (100%) diff --git a/coderd/database/migrations/000280_user_status_changes.down.sql b/coderd/database/migrations/000281_user_status_changes.down.sql similarity index 100% rename from coderd/database/migrations/000280_user_status_changes.down.sql rename to coderd/database/migrations/000281_user_status_changes.down.sql diff --git a/coderd/database/migrations/000280_user_status_changes.up.sql b/coderd/database/migrations/000281_user_status_changes.up.sql similarity index 100% rename from coderd/database/migrations/000280_user_status_changes.up.sql rename to coderd/database/migrations/000281_user_status_changes.up.sql From adbc8aa552dc324ec094977ed70bf6ac3fb67682 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 06:11:55 +0000 Subject: [PATCH 18/23] add partial fixture for CI --- .../000281_user_status_changes.up.sql | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql diff --git a/coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql b/coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql new file mode 100644 index 0000000000000..3f7bcc21b16f0 --- /dev/null +++ b/coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql @@ -0,0 +1,44 @@ +INSERT INTO + users ( + id, + email, + username, + hashed_password, + created_at, + updated_at, + status, + rbac_roles, + login_type, + avatar_url, + deleted, + last_seen_at, + quiet_hours_schedule, + theme_preference, + name, + github_com_user_id, + hashed_one_time_passcode, + one_time_passcode_expires_at + ) + VALUES ( + '5755e622-fadd-44ca-98da-5df070491844', -- uuid + 'test@example.com', + 'testuser', + 'hashed_password', + '2024-01-01 00:00:00', + '2024-01-01 00:00:00', + 'active', + '{}', + 'password', + '', + false, + '2024-01-01 00:00:00', + '', + '', + '', + 123, + NULL, + NULL + ); + +UPDATE users SET status = 'dormant', updated_at = '2024-01-01 01:00:00' WHERE id = '5755e622-fadd-44ca-98da-5df070491844'; +UPDATE users SET deleted = true, updated_at = '2024-01-01 02:00:00' WHERE id = '5755e622-fadd-44ca-98da-5df070491844'; From 8bff303fafc96aa4e9c4c658064c819f28a58aad Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 06:22:27 +0000 Subject: [PATCH 19/23] fix migration numbers --- ...tatus_changes.down.sql => 000282_user_status_changes.down.sql} | 0 ...er_status_changes.up.sql => 000282_user_status_changes.up.sql} | 0 ...er_status_changes.up.sql => 000282_user_status_changes.up.sql} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000281_user_status_changes.down.sql => 000282_user_status_changes.down.sql} (100%) rename coderd/database/migrations/{000281_user_status_changes.up.sql => 000282_user_status_changes.up.sql} (100%) rename coderd/database/migrations/testdata/fixtures/{000281_user_status_changes.up.sql => 000282_user_status_changes.up.sql} (100%) diff --git a/coderd/database/migrations/000281_user_status_changes.down.sql b/coderd/database/migrations/000282_user_status_changes.down.sql similarity index 100% rename from coderd/database/migrations/000281_user_status_changes.down.sql rename to coderd/database/migrations/000282_user_status_changes.down.sql diff --git a/coderd/database/migrations/000281_user_status_changes.up.sql b/coderd/database/migrations/000282_user_status_changes.up.sql similarity index 100% rename from coderd/database/migrations/000281_user_status_changes.up.sql rename to coderd/database/migrations/000282_user_status_changes.up.sql diff --git a/coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql b/coderd/database/migrations/testdata/fixtures/000282_user_status_changes.up.sql similarity index 100% rename from coderd/database/migrations/testdata/fixtures/000281_user_status_changes.up.sql rename to coderd/database/migrations/testdata/fixtures/000282_user_status_changes.up.sql From 1ceff066c211facfccaba8fce2c0d09771c93b51 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 06:51:17 +0000 Subject: [PATCH 20/23] rename and document sql function --- coderd/apidoc/docs.go | 4 +- coderd/apidoc/swagger.json | 4 +- coderd/database/dbauthz/dbauthz.go | 4 +- coderd/database/dbauthz/dbauthz_test.go | 4 +- coderd/database/dbmem/dbmem.go | 8 +-- coderd/database/dbmetrics/querymetrics.go | 6 +- coderd/database/dbmock/dbmock.go | 14 ++--- coderd/database/querier.go | 30 ++++++++- coderd/database/querier_test.go | 16 ++--- coderd/database/queries.sql.go | 62 ++++++++++++++----- coderd/database/queries/insights.sql | 50 ++++++++++++--- coderd/insights.go | 6 +- codersdk/insights.go | 12 ++-- docs/reference/api/insights.md | 6 +- docs/reference/api/schemas.md | 2 +- site/src/api/api.ts | 2 +- site/src/api/typesGenerated.ts | 4 +- .../GeneralSettingsPageView.tsx | 4 +- 18 files changed, 165 insertions(+), 73 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index a5d33d0797887..1af1ccbd78d7d 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -1426,7 +1426,7 @@ const docTemplate = `{ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/codersdk.GetUserStatusChangesResponse" + "$ref": "#/definitions/codersdk.GetUserStatusCountsOverTimeResponse" } } } @@ -11149,7 +11149,7 @@ const docTemplate = `{ } } }, - "codersdk.GetUserStatusChangesResponse": { + "codersdk.GetUserStatusCountsOverTimeResponse": { "type": "object", "properties": { "status_counts": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index bc65cf556e7f7..f77d2a6dba640 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -1243,7 +1243,7 @@ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/codersdk.GetUserStatusChangesResponse" + "$ref": "#/definitions/codersdk.GetUserStatusCountsOverTimeResponse" } } } @@ -10000,7 +10000,7 @@ } } }, - "codersdk.GetUserStatusChangesResponse": { + "codersdk.GetUserStatusCountsOverTimeResponse": { "type": "object", "properties": { "status_counts": { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 78d620bd82020..3ffc7779044f0 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2413,11 +2413,11 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui return q.db.GetUserNotificationPreferences(ctx, userID) } -func (q *querier) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { +func (q *querier) GetUserStatusCountsOverTime(ctx context.Context, arg database.GetUserStatusCountsOverTimeParams) ([]database.GetUserStatusCountsOverTimeRow, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceUser); err != nil { return nil, err } - return q.db.GetUserStatusChanges(ctx, arg) + return q.db.GetUserStatusCountsOverTime(ctx, arg) } func (q *querier) GetUserWorkspaceBuildParameters(ctx context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index e1258a8eed087..b0862f83aa737 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1490,8 +1490,8 @@ func (s *MethodTestSuite) TestUser() { rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead, ) })) - s.Run("GetUserStatusChanges", s.Subtest(func(db database.Store, check *expects) { - check.Args(database.GetUserStatusChangesParams{ + s.Run("GetUserStatusCountsOverTime", s.Subtest(func(db database.Store, check *expects) { + check.Args(database.GetUserStatusCountsOverTimeParams{ StartTime: time.Now().Add(-time.Hour * 24 * 30), EndTime: time.Now(), }).Asserts(rbac.ResourceUser, policy.ActionRead) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index fa51ce181ace5..8a646d21799a7 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5666,7 +5666,7 @@ func (q *FakeQuerier) GetUserNotificationPreferences(_ context.Context, userID u return out, nil } -func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { +func (q *FakeQuerier) GetUserStatusCountsOverTime(_ context.Context, arg database.GetUserStatusCountsOverTimeParams) ([]database.GetUserStatusCountsOverTimeRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -5675,16 +5675,16 @@ func (q *FakeQuerier) GetUserStatusChanges(_ context.Context, arg database.GetUs return nil, err } - result := make([]database.GetUserStatusChangesRow, 0) + result := make([]database.GetUserStatusCountsOverTimeRow, 0) for _, change := range q.userStatusChanges { if change.ChangedAt.Before(arg.StartTime) || change.ChangedAt.After(arg.EndTime) { continue } date := time.Date(change.ChangedAt.Year(), change.ChangedAt.Month(), change.ChangedAt.Day(), 0, 0, 0, 0, time.UTC) - if !slices.ContainsFunc(result, func(r database.GetUserStatusChangesRow) bool { + if !slices.ContainsFunc(result, func(r database.GetUserStatusCountsOverTimeRow) bool { return r.Status == change.NewStatus && r.Date.Equal(date) }) { - result = append(result, database.GetUserStatusChangesRow{ + result = append(result, database.GetUserStatusCountsOverTimeRow{ Status: change.NewStatus, Date: date, Count: 1, diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index c1662346adf63..78134fae00866 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1337,10 +1337,10 @@ func (m queryMetricsStore) GetUserNotificationPreferences(ctx context.Context, u return r0, r1 } -func (m queryMetricsStore) GetUserStatusChanges(ctx context.Context, arg database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { +func (m queryMetricsStore) GetUserStatusCountsOverTime(ctx context.Context, arg database.GetUserStatusCountsOverTimeParams) ([]database.GetUserStatusCountsOverTimeRow, error) { start := time.Now() - r0, r1 := m.s.GetUserStatusChanges(ctx, arg) - m.queryLatencies.WithLabelValues("GetUserStatusChanges").Observe(time.Since(start).Seconds()) + r0, r1 := m.s.GetUserStatusCountsOverTime(ctx, arg) + m.queryLatencies.WithLabelValues("GetUserStatusCountsOverTime").Observe(time.Since(start).Seconds()) return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index eedac4ced468f..ec1abc0361a88 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2811,19 +2811,19 @@ func (mr *MockStoreMockRecorder) GetUserNotificationPreferences(arg0, arg1 any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserNotificationPreferences", reflect.TypeOf((*MockStore)(nil).GetUserNotificationPreferences), arg0, arg1) } -// GetUserStatusChanges mocks base method. -func (m *MockStore) GetUserStatusChanges(arg0 context.Context, arg1 database.GetUserStatusChangesParams) ([]database.GetUserStatusChangesRow, error) { +// GetUserStatusCountsOverTime mocks base method. +func (m *MockStore) GetUserStatusCountsOverTime(arg0 context.Context, arg1 database.GetUserStatusCountsOverTimeParams) ([]database.GetUserStatusCountsOverTimeRow, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserStatusChanges", arg0, arg1) - ret0, _ := ret[0].([]database.GetUserStatusChangesRow) + ret := m.ctrl.Call(m, "GetUserStatusCountsOverTime", arg0, arg1) + ret0, _ := ret[0].([]database.GetUserStatusCountsOverTimeRow) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetUserStatusChanges indicates an expected call of GetUserStatusChanges. -func (mr *MockStoreMockRecorder) GetUserStatusChanges(arg0, arg1 any) *gomock.Call { +// GetUserStatusCountsOverTime indicates an expected call of GetUserStatusCountsOverTime. +func (mr *MockStoreMockRecorder) GetUserStatusCountsOverTime(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserStatusChanges", reflect.TypeOf((*MockStore)(nil).GetUserStatusChanges), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserStatusCountsOverTime", reflect.TypeOf((*MockStore)(nil).GetUserStatusCountsOverTime), arg0, arg1) } // GetUserWorkspaceBuildParameters mocks base method. diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 373a5f28b66df..550fe194912ca 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -285,7 +285,35 @@ type sqlcQuerier interface { GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUserLinksByUserID(ctx context.Context, userID uuid.UUID) ([]UserLink, error) GetUserNotificationPreferences(ctx context.Context, userID uuid.UUID) ([]NotificationPreference, error) - GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]GetUserStatusChangesRow, error) + // GetUserStatusCountsOverTime 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. + // 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. + // 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. + // 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 prior to the deletion. + // 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. + // 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. + // 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. + // 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. + GetUserStatusCountsOverTime(ctx context.Context, arg GetUserStatusCountsOverTimeParams) ([]GetUserStatusCountsOverTimeRow, error) GetUserWorkspaceBuildParameters(ctx context.Context, arg GetUserWorkspaceBuildParametersParams) ([]GetUserWorkspaceBuildParametersRow, error) // This will never return deleted users. GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUsersRow, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 6ace33f74a96c..8d0d7edf312f6 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2256,7 +2256,7 @@ func TestGroupRemovalTrigger(t *testing.T) { }, db2sdk.List(extraUserGroups, onlyGroupIDs)) } -func TestGetUserStatusChanges(t *testing.T) { +func TestGetUserStatusCountsOverTime(t *testing.T) { t.Parallel() if !dbtestutil.WillUsePostgres() { @@ -2294,7 +2294,7 @@ func TestGetUserStatusChanges(t *testing.T) { end := dbtime.Now() start := end.Add(-30 * 24 * time.Hour) - counts, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + counts, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: start, EndTime: end, }) @@ -2342,7 +2342,7 @@ func TestGetUserStatusChanges(t *testing.T) { }) // Query for the last 30 days - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: createdAt, EndTime: today, }) @@ -2510,7 +2510,7 @@ func TestGetUserStatusChanges(t *testing.T) { require.NoError(t, err) // Query for the last 5 days - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: createdAt, EndTime: today, }) @@ -2639,7 +2639,7 @@ func TestGetUserStatusChanges(t *testing.T) { }) require.NoError(t, err) - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: createdAt, EndTime: today, }) @@ -2715,7 +2715,7 @@ func TestGetUserStatusChanges(t *testing.T) { UpdatedAt: createdAt, }) - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: createdAt.Add(time.Hour * 24), EndTime: today, }) @@ -2742,7 +2742,7 @@ func TestGetUserStatusChanges(t *testing.T) { err = db.UpdateUserDeletedByID(ctx, user.ID) require.NoError(t, err) - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: today.Add(time.Hour * 24), EndTime: today.Add(time.Hour * 48), }) @@ -2764,7 +2764,7 @@ func TestGetUserStatusChanges(t *testing.T) { err := db.UpdateUserDeletedByID(ctx, user.ID) require.NoError(t, err) - userStatusChanges, err := db.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + userStatusChanges, err := db.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: createdAt, EndTime: today.Add(time.Hour * 24), }) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 38a87874d7f18..ffd0dd6e127c0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3094,8 +3094,9 @@ func (q *sqlQuerier) GetUserLatencyInsights(ctx context.Context, arg GetUserLate return items, nil } -const getUserStatusChanges = `-- name: GetUserStatusChanges :many -WITH dates AS ( +const getUserStatusCountsOverTime = `-- name: GetUserStatusCountsOverTime :many +WITH +dates_of_interest AS ( SELECT $1::timestamptz AS date UNION @@ -3138,7 +3139,7 @@ status_changes_during_range AS ( AND usc.changed_at <= $2::timestamptz AND (ud.user_id IS NULL OR usc.changed_at < ud.deleted_at) ), -all_status_changes AS ( +relevant_status_changes AS ( SELECT user_id, new_status, @@ -3153,14 +3154,17 @@ all_status_changes AS ( changed_at FROM status_changes_during_range ), +statuses AS ( + SELECT DISTINCT new_status FROM relevant_status_changes +), ranked_status_change_per_user_per_date AS ( SELECT d.date, - asc1.user_id, - ROW_NUMBER() OVER (PARTITION BY d.date, asc1.user_id ORDER BY asc1.changed_at DESC) AS rn, - asc1.new_status - FROM dates d - LEFT JOIN all_status_changes asc1 ON asc1.changed_at <= d.date + rsc1.user_id, + ROW_NUMBER() OVER (PARTITION BY d.date, rsc1.user_id ORDER BY rsc1.changed_at DESC) AS rn, + rsc1.new_status + FROM dates_of_interest d + LEFT JOIN relevant_status_changes rsc1 ON rsc1.changed_at <= d.date ) SELECT date, @@ -3179,30 +3183,58 @@ SELECT ) ) AS count FROM ranked_status_change_per_user_per_date rscpupd -CROSS JOIN (select distinct new_status FROM all_status_changes) statuses +CROSS JOIN statuses GROUP BY date, statuses.new_status ` -type GetUserStatusChangesParams struct { +type GetUserStatusCountsOverTimeParams struct { StartTime time.Time `db:"start_time" json:"start_time"` EndTime time.Time `db:"end_time" json:"end_time"` } -type GetUserStatusChangesRow struct { +type GetUserStatusCountsOverTimeRow struct { Date time.Time `db:"date" json:"date"` Status UserStatus `db:"status" json:"status"` Count int64 `db:"count" json:"count"` } -func (q *sqlQuerier) GetUserStatusChanges(ctx context.Context, arg GetUserStatusChangesParams) ([]GetUserStatusChangesRow, error) { - rows, err := q.db.QueryContext(ctx, getUserStatusChanges, arg.StartTime, arg.EndTime) +// GetUserStatusCountsOverTime 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. +// 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. +// 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. +// 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 prior to the deletion. +// 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. +// 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. +// 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. +// 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. +func (q *sqlQuerier) GetUserStatusCountsOverTime(ctx context.Context, arg GetUserStatusCountsOverTimeParams) ([]GetUserStatusCountsOverTimeRow, error) { + rows, err := q.db.QueryContext(ctx, getUserStatusCountsOverTime, arg.StartTime, arg.EndTime) if err != nil { return nil, err } defer rows.Close() - var items []GetUserStatusChangesRow + var items []GetUserStatusCountsOverTimeRow for rows.Next() { - var i GetUserStatusChangesRow + var i GetUserStatusCountsOverTimeRow if err := rows.Scan(&i.Date, &i.Status, &i.Count); err != nil { return nil, err } diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index c721ebcc79227..d599e2989a56e 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -772,8 +772,23 @@ FROM unique_template_params utp JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name) GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value; --- name: GetUserStatusChanges :many -WITH dates AS ( +-- name: GetUserStatusCountsOverTime :many +-- GetUserStatusCountsOverTime 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 @start_time::timestamptz AS date UNION @@ -794,6 +809,9 @@ WITH dates AS ( SELECT @end_time::timestamptz AS date ), +-- 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. latest_status_before_range AS ( SELECT DISTINCT usc.user_id, @@ -805,6 +823,10 @@ latest_status_before_range AS ( AND (ud.user_id IS NULL OR ud.deleted_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 prior to the deletion. +-- 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 AS ( SELECT usc.user_id, @@ -816,7 +838,9 @@ status_changes_during_range AS ( AND usc.changed_at <= @end_time::timestamptz AND (ud.user_id IS NULL OR usc.changed_at < ud.deleted_at) ), -all_status_changes AS ( +-- 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, @@ -831,14 +855,22 @@ all_status_changes AS ( changed_at FROM status_changes_during_range ), +-- 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 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 AS ( SELECT d.date, - asc1.user_id, - ROW_NUMBER() OVER (PARTITION BY d.date, asc1.user_id ORDER BY asc1.changed_at DESC) AS rn, - asc1.new_status - FROM dates d - LEFT JOIN all_status_changes asc1 ON asc1.changed_at <= d.date + rsc1.user_id, + ROW_NUMBER() OVER (PARTITION BY d.date, rsc1.user_id ORDER BY rsc1.changed_at DESC) AS rn, + rsc1.new_status + FROM dates_of_interest d + LEFT JOIN relevant_status_changes rsc1 ON rsc1.changed_at <= d.date ) SELECT date, @@ -857,6 +889,6 @@ SELECT ) ) AS count FROM ranked_status_change_per_user_per_date rscpupd -CROSS JOIN (select distinct new_status FROM all_status_changes) statuses +CROSS JOIN statuses GROUP BY date, statuses.new_status; diff --git a/coderd/insights.go b/coderd/insights.go index 3cd9a364a2efe..10205439e2b4a 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -298,7 +298,7 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { // @Produce json // @Tags Insights // @Param tz_offset query int true "Time-zone offset (e.g. -2)" -// @Success 200 {object} codersdk.GetUserStatusChangesResponse +// @Success 200 {object} codersdk.GetUserStatusCountsOverTimeResponse // @Router /insights/user-status-counts-over-time [get] func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -324,7 +324,7 @@ func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http // Always return 60 days of data (2 months). sixtyDaysAgo := nextHourInLoc.In(loc).Truncate(24*time.Hour).AddDate(0, 0, -60) - rows, err := api.Database.GetUserStatusChanges(ctx, database.GetUserStatusChangesParams{ + rows, err := api.Database.GetUserStatusCountsOverTime(ctx, database.GetUserStatusCountsOverTimeParams{ StartTime: sixtyDaysAgo, EndTime: nextHourInLoc, }) @@ -336,7 +336,7 @@ func (api *API) insightsUserStatusCountsOverTime(rw http.ResponseWriter, r *http return } - resp := codersdk.GetUserStatusChangesResponse{ + resp := codersdk.GetUserStatusCountsOverTimeResponse{ StatusCounts: make(map[codersdk.UserStatus][]codersdk.UserStatusChangeCount), } diff --git a/codersdk/insights.go b/codersdk/insights.go index b3a3cc728378a..3e285b5de0a58 100644 --- a/codersdk/insights.go +++ b/codersdk/insights.go @@ -283,7 +283,7 @@ func (c *Client) TemplateInsights(ctx context.Context, req TemplateInsightsReque return result, json.NewDecoder(resp.Body).Decode(&result) } -type GetUserStatusChangesResponse struct { +type GetUserStatusCountsOverTimeResponse struct { StatusCounts map[UserStatus][]UserStatusChangeCount `json:"status_counts"` } @@ -292,24 +292,24 @@ type UserStatusChangeCount struct { Count int64 `json:"count" example:"10"` } -type GetUserStatusChangesRequest struct { +type GetUserStatusCountsOverTimeRequest struct { Offset time.Time `json:"offset" format:"date-time"` } -func (c *Client) GetUserStatusChanges(ctx context.Context, req GetUserStatusChangesRequest) (GetUserStatusChangesResponse, error) { +func (c *Client) GetUserStatusCountsOverTime(ctx context.Context, req GetUserStatusCountsOverTimeRequest) (GetUserStatusCountsOverTimeResponse, error) { qp := url.Values{} qp.Add("offset", req.Offset.Format(insightsTimeLayout)) reqURL := fmt.Sprintf("/api/v2/insights/user-status-counts-over-time?%s", qp.Encode()) resp, err := c.Request(ctx, http.MethodGet, reqURL, nil) if err != nil { - return GetUserStatusChangesResponse{}, xerrors.Errorf("make request: %w", err) + return GetUserStatusCountsOverTimeResponse{}, xerrors.Errorf("make request: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return GetUserStatusChangesResponse{}, ReadBodyAsError(resp) + return GetUserStatusCountsOverTimeResponse{}, ReadBodyAsError(resp) } - var result GetUserStatusChangesResponse + var result GetUserStatusCountsOverTimeResponse return result, json.NewDecoder(resp.Body).Decode(&result) } diff --git a/docs/reference/api/insights.md b/docs/reference/api/insights.md index 8d6f5640f9e60..71ae63c2cf4a0 100644 --- a/docs/reference/api/insights.md +++ b/docs/reference/api/insights.md @@ -289,8 +289,8 @@ curl -X GET http://coder-server:8080/api/v2/insights/user-status-counts-over-tim ### Responses -| Status | Meaning | Description | Schema | -| ------ | ------------------------------------------------------- | ----------- | ---------------------------------------------------------------------------------------- | -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.GetUserStatusChangesResponse](schemas.md#codersdkgetuserstatuschangesresponse) | +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ------------------------------------------------------------------------------------------------------ | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.GetUserStatusCountsOverTimeResponse](schemas.md#codersdkgetuserstatuscountsovertimeresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 8abd3ae7162ba..f6eb7488bdd78 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2882,7 +2882,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | ----- | ------ | -------- | ------------ | ----------- | | `key` | string | false | | | -## codersdk.GetUserStatusChangesResponse +## codersdk.GetUserStatusCountsOverTimeResponse ```json { diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 16c0506774295..74ca5ce685007 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2099,7 +2099,7 @@ class ApiMethods { getInsightsUserStatusCountsOverTime = async ( offset = Math.trunc(new Date().getTimezoneOffset() / 60), - ): Promise => { + ): Promise => { const searchParams = new URLSearchParams({ offset: offset.toString(), }); diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index c8d33fb6b85e2..23e8be803f308 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -776,12 +776,12 @@ export interface GenerateAPIKeyResponse { } // From codersdk/insights.go -export interface GetUserStatusChangesRequest { +export interface GetUserStatusCountsOverTimeRequest { readonly offset: string; } // From codersdk/insights.go -export interface GetUserStatusChangesResponse { +export interface GetUserStatusCountsOverTimeResponse { readonly status_counts: Record; } diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx index bf663fecaa945..3680e823516a6 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx @@ -2,7 +2,7 @@ import AlertTitle from "@mui/material/AlertTitle"; import type { DAUsResponse, Experiments, - GetUserStatusChangesResponse, + GetUserStatusCountsOverTimeResponse, SerpentOption, } from "api/typesGenerated"; import { @@ -25,7 +25,7 @@ export type GeneralSettingsPageViewProps = { deploymentOptions: SerpentOption[]; deploymentDAUs?: DAUsResponse; deploymentDAUsError: unknown; - userStatusCountsOverTime?: GetUserStatusChangesResponse; + userStatusCountsOverTime?: GetUserStatusCountsOverTimeResponse; activeUserLimit?: number; readonly invalidExperiments: Experiments | string[]; readonly safeExperiments: Experiments | string[]; From 94bb06a1caaceee038f39eb4fcffe3b7de1a94bf Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 09:06:10 +0000 Subject: [PATCH 21/23] fix api call --- site/src/api/api.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 74ca5ce685007..34465e4ba80f9 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2101,7 +2101,7 @@ class ApiMethods { offset = Math.trunc(new Date().getTimezoneOffset() / 60), ): Promise => { const searchParams = new URLSearchParams({ - offset: offset.toString(), + tz_offset: offset.toString(), }); const response = await this.axios.get( `/api/v2/insights/user-status-counts-over-time?${searchParams}`, From d443a723a53d9efb6e4540e89bf36545e423c690 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 09:35:14 +0000 Subject: [PATCH 22/23] display the license user limit in the user status graph --- .../GeneralSettingsPage/GeneralSettingsPage.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx index 3de614a42ac39..6c02c4ea9b946 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx @@ -8,6 +8,7 @@ import { Helmet } from "react-helmet-async"; import { useQuery } from "react-query"; import { pageTitle } from "utils/page"; import { GeneralSettingsPageView } from "./GeneralSettingsPageView"; +import { entitlements } from "api/queries/entitlements"; const GeneralSettingsPage: FC = () => { const { deploymentConfig } = useDeploymentSettings(); @@ -15,6 +16,7 @@ const GeneralSettingsPage: FC = () => { const safeExperimentsQuery = useQuery(availableExperiments()); const { metadata } = useEmbeddedMetadata(); + const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); const enabledExperimentsQuery = useQuery(experiments(metadata.experiments)); const userStatusCountsOverTimeQuery = useQuery(userStatusCountsOverTime()); const safeExperiments = safeExperimentsQuery.data?.safe ?? []; @@ -34,6 +36,7 @@ const GeneralSettingsPage: FC = () => { deploymentDAUsError={deploymentDAUsQuery.error} invalidExperiments={invalidExperiments} safeExperiments={safeExperiments} + activeUserLimit={entitlementsQuery.data?.features?.user_limit?.limit} userStatusCountsOverTime={userStatusCountsOverTimeQuery.data} /> From 040086e5d8a1d2d32d2e3f3f57e611f22fbcde1f Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 3 Jan 2025 09:37:48 +0000 Subject: [PATCH 23/23] make fmt --- .../GeneralSettingsPage/GeneralSettingsPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx index 6c02c4ea9b946..e4543ea2c3197 100644 --- a/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx +++ b/site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx @@ -1,4 +1,5 @@ import { deploymentDAUs } from "api/queries/deployment"; +import { entitlements } from "api/queries/entitlements"; import { availableExperiments, experiments } from "api/queries/experiments"; import { userStatusCountsOverTime } from "api/queries/insights"; import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; @@ -8,7 +9,6 @@ import { Helmet } from "react-helmet-async"; import { useQuery } from "react-query"; import { pageTitle } from "utils/page"; import { GeneralSettingsPageView } from "./GeneralSettingsPageView"; -import { entitlements } from "api/queries/entitlements"; const GeneralSettingsPage: FC = () => { const { deploymentConfig } = useDeploymentSettings();