From 4bb2c73fb4295c88f44a07997ba33a5d9b53b40a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 13:04:59 +0000 Subject: [PATCH 1/6] lower app intervals to 1m and increase test coverage --- coderd/database/dbfake/dbfake.go | 13 ++----------- coderd/database/queries.sql.go | 6 +++--- coderd/database/queries/insights.sql | 6 +++--- coderd/insights_test.go | 14 +++++++++++++- ...s_and_workspaces_week_all_templates.json.golden | 4 ++-- ...and_workspaces_week_deployment_wide.json.golden | 4 ++-- ..._and_workspaces_week_first_template.json.golden | 4 ++-- ..._other_timezone_(S\303\243o_Paulo).json.golden" | 5 +++-- 8 files changed, 30 insertions(+), 26 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index d8afdd3d96b87..209908c86a69e 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2004,15 +2004,6 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G appUsageIntervalsByUserAgentApp := make(map[uniqueKey]map[time.Time]int64) for _, s := range q.workspaceAppStats { - // (was.session_started_at >= ts.from_ AND was.session_started_at < ts.to_) - // OR (was.session_ended_at > ts.from_ AND was.session_ended_at < ts.to_) - // OR (was.session_started_at < ts.from_ AND was.session_ended_at >= ts.to_) - if !(((s.SessionStartedAt.After(arg.StartTime) || s.SessionStartedAt.Equal(arg.StartTime)) && s.SessionStartedAt.Before(arg.EndTime)) || - (s.SessionEndedAt.After(arg.StartTime) && s.SessionEndedAt.Before(arg.EndTime)) || - (s.SessionStartedAt.Before(arg.StartTime) && (s.SessionEndedAt.After(arg.EndTime) || s.SessionEndedAt.Equal(arg.EndTime)))) { - continue - } - w, err := q.getWorkspaceByIDNoLock(ctx, s.WorkspaceID) if err != nil { return nil, err @@ -2048,8 +2039,8 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G t = arg.StartTime } for t.Before(s.SessionEndedAt) && t.Before(arg.EndTime) { - appUsageIntervalsByUserAgentApp[key][t] = 300 // 5 minutes. - t = t.Add(5 * time.Minute) + appUsageIntervalsByUserAgentApp[key][t] = 60 // 5 minutes. + t = t.Add(1 * time.Minute) } } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 49c3084c0a710..4bf44ddc1ec91 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1465,11 +1465,11 @@ const getTemplateAppInsights = `-- name: GetTemplateAppInsights :many WITH ts AS ( SELECT d::timestamptz AS from_, - (d::timestamptz + '5 minute'::interval) AS to_, - EXTRACT(epoch FROM '5 minute'::interval) AS seconds + (d::timestamptz + '1 minute'::interval) AS to_, + EXTRACT(epoch FROM '1 minute'::interval) AS seconds FROM -- Subtract 1 second from end_time to avoid including the next interval in the results. - generate_series($1::timestamptz, ($2::timestamptz) - '1 second'::interval, '5 minute'::interval) d + generate_series($1::timestamptz, ($2::timestamptz) - '1 second'::interval, '1 minute'::interval) d ), app_stats_by_user_and_agent AS ( SELECT ts.from_, diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 93e195f41eb64..bfed04fe4993b 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -64,11 +64,11 @@ FROM agent_stats_by_interval_and_user; WITH ts AS ( SELECT d::timestamptz AS from_, - (d::timestamptz + '5 minute'::interval) AS to_, - EXTRACT(epoch FROM '5 minute'::interval) AS seconds + (d::timestamptz + '1 minute'::interval) AS to_, + EXTRACT(epoch FROM '1 minute'::interval) AS seconds FROM -- Subtract 1 second from end_time to avoid including the next interval in the results. - generate_series(@start_time::timestamptz, (@end_time::timestamptz) - '1 second'::interval, '5 minute'::interval) d + generate_series(@start_time::timestamptz, (@end_time::timestamptz) - '1 second'::interval, '1 minute'::interval) d ), app_stats_by_user_and_agent AS ( SELECT ts.from_, diff --git a/coderd/insights_test.go b/coderd/insights_test.go index b6bc1ab424e31..351905e9b698e 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -778,7 +778,19 @@ func TestTemplateInsights_Golden(t *testing.T) { endedAt: frozenWeekAgo.Add(time.Hour), requests: 1, }, - { // used an app on the last day, counts as active user, 12m -> 15m rounded. + { // 30s of app usage -> 1m rounded. + app: users[0].workspaces[0].apps[0], + startedAt: frozenWeekAgo.Add(2*time.Hour + 10*time.Second), + endedAt: frozenWeekAgo.Add(2*time.Hour + 40*time.Second), + requests: 1, + }, + { // 1m30s of app usage -> 2m rounded (included in São Paulo). + app: users[0].workspaces[0].apps[0], + startedAt: frozenWeekAgo.Add(3*time.Hour + 30*time.Second), + endedAt: frozenWeekAgo.Add(3*time.Hour + 90*time.Second), + requests: 1, + }, + { // used an app on the last day, counts as active user, 12m. app: users[0].workspaces[0].apps[2], startedAt: frozenWeekAgo.AddDate(0, 0, 6), endedAt: frozenWeekAgo.AddDate(0, 0, 6).Add(12 * time.Minute), diff --git a/coderd/testdata/insights/multiple_users_and_workspaces_week_all_templates.json.golden b/coderd/testdata/insights/multiple_users_and_workspaces_week_all_templates.json.golden index c4164fe1248ce..664e2fed8f250 100644 --- a/coderd/testdata/insights/multiple_users_and_workspaces_week_all_templates.json.golden +++ b/coderd/testdata/insights/multiple_users_and_workspaces_week_all_templates.json.golden @@ -66,7 +66,7 @@ "display_name": "app1", "slug": "app1", "icon": "/icon1.png", - "seconds": 25200 + "seconds": 25380 }, { "template_ids": [ @@ -76,7 +76,7 @@ "display_name": "app3", "slug": "app3", "icon": "/icon2.png", - "seconds": 900 + "seconds": 720 }, { "template_ids": [ diff --git a/coderd/testdata/insights/multiple_users_and_workspaces_week_deployment_wide.json.golden b/coderd/testdata/insights/multiple_users_and_workspaces_week_deployment_wide.json.golden index c4164fe1248ce..664e2fed8f250 100644 --- a/coderd/testdata/insights/multiple_users_and_workspaces_week_deployment_wide.json.golden +++ b/coderd/testdata/insights/multiple_users_and_workspaces_week_deployment_wide.json.golden @@ -66,7 +66,7 @@ "display_name": "app1", "slug": "app1", "icon": "/icon1.png", - "seconds": 25200 + "seconds": 25380 }, { "template_ids": [ @@ -76,7 +76,7 @@ "display_name": "app3", "slug": "app3", "icon": "/icon2.png", - "seconds": 900 + "seconds": 720 }, { "template_ids": [ diff --git a/coderd/testdata/insights/multiple_users_and_workspaces_week_first_template.json.golden b/coderd/testdata/insights/multiple_users_and_workspaces_week_first_template.json.golden index c7132bf9f3340..d96469dc5c724 100644 --- a/coderd/testdata/insights/multiple_users_and_workspaces_week_first_template.json.golden +++ b/coderd/testdata/insights/multiple_users_and_workspaces_week_first_template.json.golden @@ -55,7 +55,7 @@ "display_name": "app1", "slug": "app1", "icon": "/icon1.png", - "seconds": 3600 + "seconds": 3780 }, { "template_ids": [ @@ -65,7 +65,7 @@ "display_name": "app3", "slug": "app3", "icon": "/icon2.png", - "seconds": 900 + "seconds": 720 } ], "parameters_usage": [] diff --git "a/coderd/testdata/insights/multiple_users_and_workspaces_week_other_timezone_(S\303\243o_Paulo).json.golden" "b/coderd/testdata/insights/multiple_users_and_workspaces_week_other_timezone_(S\303\243o_Paulo).json.golden" index 9a623ea92fe49..8f447e4112dd0 100644 --- "a/coderd/testdata/insights/multiple_users_and_workspaces_week_other_timezone_(S\303\243o_Paulo).json.golden" +++ "b/coderd/testdata/insights/multiple_users_and_workspaces_week_other_timezone_(S\303\243o_Paulo).json.golden" @@ -51,13 +51,14 @@ }, { "template_ids": [ + "00000000-0000-0000-0000-000000000001", "00000000-0000-0000-0000-000000000002" ], "type": "app", "display_name": "app1", "slug": "app1", "icon": "/icon1.png", - "seconds": 21600 + "seconds": 21720 }, { "template_ids": [ @@ -67,7 +68,7 @@ "display_name": "app3", "slug": "app3", "icon": "/icon2.png", - "seconds": 4500 + "seconds": 4320 }, { "template_ids": [ From bca6e37f1ca4db01ac4db15818c2ec3a9d6945b6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 13:09:22 +0000 Subject: [PATCH 2/6] fix(coderd): rewrite template app insights query for speed, decrease intervals --- coderd/database/queries.sql.go | 43 ++++++++++++++-------------- coderd/database/queries/insights.sql | 37 ++++++++++++------------ 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4bf44ddc1ec91..bfad61b46b7b0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1462,19 +1462,10 @@ func (q *sqlQuerier) UpdateGroupByID(ctx context.Context, arg UpdateGroupByIDPar } const getTemplateAppInsights = `-- name: GetTemplateAppInsights :many -WITH ts AS ( - SELECT - d::timestamptz AS from_, - (d::timestamptz + '1 minute'::interval) AS to_, - EXTRACT(epoch FROM '1 minute'::interval) AS seconds - FROM - -- Subtract 1 second from end_time to avoid including the next interval in the results. - generate_series($1::timestamptz, ($2::timestamptz) - '1 second'::interval, '1 minute'::interval) d -), app_stats_by_user_and_agent AS ( +WITH app_stats_by_user_and_agent AS ( SELECT - ts.from_, - ts.to_, - ts.seconds, + s.start_time, + 60 as seconds, w.template_id, was.user_id, was.agent_id, @@ -1483,15 +1474,10 @@ WITH ts AS ( wa.display_name, wa.icon, (wa.slug IS NOT NULL)::boolean AS is_app - FROM ts - JOIN workspace_app_stats was ON ( - (was.session_started_at >= ts.from_ AND was.session_started_at < ts.to_) - OR (was.session_ended_at > ts.from_ AND was.session_ended_at < ts.to_) - OR (was.session_started_at < ts.from_ AND was.session_ended_at >= ts.to_) - ) + FROM workspace_app_stats was JOIN workspaces w ON ( w.id = was.workspace_id - AND CASE WHEN COALESCE(array_length($3::uuid[], 1), 0) > 0 THEN w.template_id = ANY($3::uuid[]) ELSE TRUE END + AND CASE WHEN COALESCE(array_length($1::uuid[], 1), 0) > 0 THEN w.template_id = ANY($1::uuid[]) ELSE TRUE END ) -- We do a left join here because we want to include user IDs that have used -- e.g. ports when counting active users. @@ -1499,7 +1485,20 @@ WITH ts AS ( wa.agent_id = was.agent_id AND wa.slug = was.slug_or_port ) - GROUP BY ts.from_, ts.to_, ts.seconds, w.template_id, was.user_id, was.agent_id, was.access_method, was.slug_or_port, wa.display_name, wa.icon, wa.slug + -- This table contains both 1 minute entries and >1 minute entries, + -- to calculate this with our uniqueness constraints, we generate series + -- for the longer intervals. + CROSS JOIN LATERAL generate_series( + date_trunc('minute', was.session_started_at), + -- Subtract 1ms to avoid creating an extra serie. + date_trunc('minute', was.session_ended_at - '1 ms'::interval), + '1 minute'::interval + ) s(start_time) + WHERE + s.start_time >= $2::timestamptz + -- Subtract one minute because the series only contains the start time. + AND s.start_time < ($3::timestamptz) - '1 minute'::interval + GROUP BY s.start_time, w.template_id, was.user_id, was.agent_id, was.access_method, was.slug_or_port, wa.display_name, wa.icon, wa.slug ) SELECT @@ -1517,9 +1516,9 @@ GROUP BY access_method, slug_or_port, display_name, icon, is_app ` type GetTemplateAppInsightsParams struct { + TemplateIDs []uuid.UUID `db:"template_ids" json:"template_ids"` StartTime time.Time `db:"start_time" json:"start_time"` EndTime time.Time `db:"end_time" json:"end_time"` - TemplateIDs []uuid.UUID `db:"template_ids" json:"template_ids"` } type GetTemplateAppInsightsRow struct { @@ -1537,7 +1536,7 @@ type GetTemplateAppInsightsRow struct { // timeframe. The result can be filtered on template_ids, meaning only user data // from workspaces based on those templates will be included. func (q *sqlQuerier) GetTemplateAppInsights(ctx context.Context, arg GetTemplateAppInsightsParams) ([]GetTemplateAppInsightsRow, error) { - rows, err := q.db.QueryContext(ctx, getTemplateAppInsights, arg.StartTime, arg.EndTime, pq.Array(arg.TemplateIDs)) + rows, err := q.db.QueryContext(ctx, getTemplateAppInsights, pq.Array(arg.TemplateIDs), arg.StartTime, arg.EndTime) if err != nil { return nil, err } diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index bfed04fe4993b..0827b854a8315 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -61,19 +61,10 @@ FROM agent_stats_by_interval_and_user; -- GetTemplateAppInsights returns the aggregate usage of each app in a given -- timeframe. The result can be filtered on template_ids, meaning only user data -- from workspaces based on those templates will be included. -WITH ts AS ( - SELECT - d::timestamptz AS from_, - (d::timestamptz + '1 minute'::interval) AS to_, - EXTRACT(epoch FROM '1 minute'::interval) AS seconds - FROM - -- Subtract 1 second from end_time to avoid including the next interval in the results. - generate_series(@start_time::timestamptz, (@end_time::timestamptz) - '1 second'::interval, '1 minute'::interval) d -), app_stats_by_user_and_agent AS ( +WITH app_stats_by_user_and_agent AS ( SELECT - ts.from_, - ts.to_, - ts.seconds, + s.start_time, + 60 as seconds, w.template_id, was.user_id, was.agent_id, @@ -82,12 +73,7 @@ WITH ts AS ( wa.display_name, wa.icon, (wa.slug IS NOT NULL)::boolean AS is_app - FROM ts - JOIN workspace_app_stats was ON ( - (was.session_started_at >= ts.from_ AND was.session_started_at < ts.to_) - OR (was.session_ended_at > ts.from_ AND was.session_ended_at < ts.to_) - OR (was.session_started_at < ts.from_ AND was.session_ended_at >= ts.to_) - ) + FROM workspace_app_stats was JOIN workspaces w ON ( w.id = was.workspace_id AND CASE WHEN COALESCE(array_length(@template_ids::uuid[], 1), 0) > 0 THEN w.template_id = ANY(@template_ids::uuid[]) ELSE TRUE END @@ -98,7 +84,20 @@ WITH ts AS ( wa.agent_id = was.agent_id AND wa.slug = was.slug_or_port ) - GROUP BY ts.from_, ts.to_, ts.seconds, w.template_id, was.user_id, was.agent_id, was.access_method, was.slug_or_port, wa.display_name, wa.icon, wa.slug + -- This table contains both 1 minute entries and >1 minute entries, + -- to calculate this with our uniqueness constraints, we generate series + -- for the longer intervals. + CROSS JOIN LATERAL generate_series( + date_trunc('minute', was.session_started_at), + -- Subtract 1ms to avoid creating an extra serie. + date_trunc('minute', was.session_ended_at - '1 ms'::interval), + '1 minute'::interval + ) s(start_time) + WHERE + s.start_time >= @start_time::timestamptz + -- Subtract one minute because the series only contains the start time. + AND s.start_time < (@end_time::timestamptz) - '1 minute'::interval + GROUP BY s.start_time, w.template_id, was.user_id, was.agent_id, was.access_method, was.slug_or_port, wa.display_name, wa.icon, wa.slug ) SELECT From 6080394efb01525a9a2290a3142636f61bb845a2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 13:11:52 +0000 Subject: [PATCH 3/6] fix comment --- coderd/database/dbfake/dbfake.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 209908c86a69e..50b8061b70f8f 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2039,7 +2039,7 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G t = arg.StartTime } for t.Before(s.SessionEndedAt) && t.Before(arg.EndTime) { - appUsageIntervalsByUserAgentApp[key][t] = 60 // 5 minutes. + appUsageIntervalsByUserAgentApp[key][t] = 60 // 1 minute. t = t.Add(1 * time.Minute) } } From d96187fd8fbcc1552d6234ad8b7cf267b0e41682 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 13:22:38 +0000 Subject: [PATCH 4/6] fix lint --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/insights.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index bfad61b46b7b0..eb81d16ce84b9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1490,7 +1490,7 @@ WITH app_stats_by_user_and_agent AS ( -- for the longer intervals. CROSS JOIN LATERAL generate_series( date_trunc('minute', was.session_started_at), - -- Subtract 1ms to avoid creating an extra serie. + -- Subtract 1ms to avoid creating an extra series. date_trunc('minute', was.session_ended_at - '1 ms'::interval), '1 minute'::interval ) s(start_time) diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 0827b854a8315..693bcbcef8633 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -89,7 +89,7 @@ WITH app_stats_by_user_and_agent AS ( -- for the longer intervals. CROSS JOIN LATERAL generate_series( date_trunc('minute', was.session_started_at), - -- Subtract 1ms to avoid creating an extra serie. + -- Subtract 1ms to avoid creating an extra series. date_trunc('minute', was.session_ended_at - '1 ms'::interval), '1 minute'::interval ) s(start_time) From 66ffdbde0b73c58bebee880ee23daa794f68d80c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 13:26:36 +0000 Subject: [PATCH 5/6] restore fakedb time filter --- coderd/database/dbfake/dbfake.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 50b8061b70f8f..9455e8e69009b 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2004,6 +2004,15 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G appUsageIntervalsByUserAgentApp := make(map[uniqueKey]map[time.Time]int64) for _, s := range q.workspaceAppStats { + // (was.session_started_at >= ts.from_ AND was.session_started_at < ts.to_) + // OR (was.session_ended_at > ts.from_ AND was.session_ended_at < ts.to_) + // OR (was.session_started_at < ts.from_ AND was.session_ended_at >= ts.to_) + if !(((s.SessionStartedAt.After(arg.StartTime) || s.SessionStartedAt.Equal(arg.StartTime)) && s.SessionStartedAt.Before(arg.EndTime)) || + (s.SessionEndedAt.After(arg.StartTime) && s.SessionEndedAt.Before(arg.EndTime)) || + (s.SessionStartedAt.Before(arg.StartTime) && (s.SessionEndedAt.After(arg.EndTime) || s.SessionEndedAt.Equal(arg.EndTime)))) { + continue + } + w, err := q.getWorkspaceByIDNoLock(ctx, s.WorkspaceID) if err != nil { return nil, err From d65295ee61c11b2dfdc7784d72e1c00f727d079a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 24 Aug 2023 14:13:14 +0000 Subject: [PATCH 6/6] use microsecond, postgres time granularity --- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/insights.sql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index eb81d16ce84b9..9d3fefccd842a 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1490,8 +1490,8 @@ WITH app_stats_by_user_and_agent AS ( -- for the longer intervals. CROSS JOIN LATERAL generate_series( date_trunc('minute', was.session_started_at), - -- Subtract 1ms to avoid creating an extra series. - date_trunc('minute', was.session_ended_at - '1 ms'::interval), + -- Subtract 1 microsecond to avoid creating an extra series. + date_trunc('minute', was.session_ended_at - '1 microsecond'::interval), '1 minute'::interval ) s(start_time) WHERE diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 693bcbcef8633..d76d106edd5d1 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -89,8 +89,8 @@ WITH app_stats_by_user_and_agent AS ( -- for the longer intervals. CROSS JOIN LATERAL generate_series( date_trunc('minute', was.session_started_at), - -- Subtract 1ms to avoid creating an extra series. - date_trunc('minute', was.session_ended_at - '1 ms'::interval), + -- Subtract 1 microsecond to avoid creating an extra series. + date_trunc('minute', was.session_ended_at - '1 microsecond'::interval), '1 minute'::interval ) s(start_time) WHERE