Skip to content

feat(coderd): support weekly aggregated insights #9684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Sep 19, 2023
13 changes: 10 additions & 3 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 11 additions & 3 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion coderd/database/queries.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion coderd/database/queries/workspaceagentstats.sql
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ ORDER BY
date ASC;

-- name: DeleteOldWorkspaceAgentStats :exec
DELETE FROM workspace_agent_stats WHERE created_at < NOW() - INTERVAL '30 days';
DELETE FROM workspace_agent_stats WHERE created_at < NOW() - INTERVAL '6 months';
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Depending on the size of the deployment, this could be a big increase in database size. I'd recommend making this configurable.

Copy link
Member

Choose a reason for hiding this comment

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

I think it could be an acceptable new default value if we stopped writing "zero" data for agent stats, or if we deleted the "zero" data every 7 days and non-zero data every 6 months.

With "zero" data, I mean:

WHERE rx_bytes = 0
	AND rx_packets = 0
	AND tx_bytes = 0
	AND tx_packets = 0
	AND connections_by_proto = '{}'
	AND connection_count = 0
	AND connection_median_latency_ms = -0.001
	AND session_count_vscode = 0
	AND session_count_jetbrains = 0
	AND session_count_reconnecting_pty = 0
	AND session_count_ssh = 0

These rows don't tell us anything other than the agent is alive/sending data.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could extend the DELETE query to remove "zero" data, but then need to list all columns unless it is fine to mention only representatives like rx_bytes and tx_bytes? Otherwise, I'm afraid that somebody will and extra column and the DELETE query will stop working at all.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a follow-up candidate.


-- name: GetDeploymentWorkspaceAgentStats :one
WITH agent_stats AS (
Expand Down
29 changes: 24 additions & 5 deletions coderd/insights.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) {
if !ok {
return
}
interval, ok := verifyInsightsInterval(ctx, rw, intervalString)
interval, ok := parseInsightsInterval(ctx, rw, intervalString, startTime, endTime)
if !ok {
return
}
Expand All @@ -198,7 +198,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) {
eg.SetLimit(4)

// The following insights data queries have a theoretical chance to be
// inconsistent between eachother when looking at "today", however, the
// inconsistent between each other when looking at "today", however, the
// overhead from a transaction is not worth it.
eg.Go(func() error {
var err error
Expand All @@ -207,7 +207,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) {
StartTime: startTime,
EndTime: endTime,
TemplateIDs: templateIDs,
IntervalDays: 1,
IntervalDays: interval.Days(),
})
if err != nil {
return xerrors.Errorf("get template daily insights: %w", err)
Expand Down Expand Up @@ -531,20 +531,39 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s
return startTime, endTime, true
}

func verifyInsightsInterval(ctx context.Context, rw http.ResponseWriter, intervalString string) (codersdk.InsightsReportInterval, bool) {
func parseInsightsInterval(ctx context.Context, rw http.ResponseWriter, intervalString string, startTime, endTime time.Time) (codersdk.InsightsReportInterval, bool) {
switch v := codersdk.InsightsReportInterval(intervalString); v {
case codersdk.InsightsReportIntervalDay, "":
return v, true
case codersdk.InsightsReportIntervalWeek:
if !lastReportIntervalHasAtLeastSixDays(startTime, endTime) {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Query parameter has invalid value.",
Detail: "Last report interval should have at least 6 days.",
})
return "", false
}
return v, true
default:
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Query parameter has invalid value.",
Validations: []codersdk.ValidationError{
{
Field: "interval",
Detail: fmt.Sprintf("must be one of %v", []codersdk.InsightsReportInterval{codersdk.InsightsReportIntervalDay}),
Detail: fmt.Sprintf("must be one of %v", []codersdk.InsightsReportInterval{codersdk.InsightsReportIntervalDay, codersdk.InsightsReportIntervalWeek}),
},
},
})
return "", false
}
}

func lastReportIntervalHasAtLeastSixDays(startTime, endTime time.Time) bool {
lastReportIntervalDays := endTime.Sub(startTime) % (7 * 24 * time.Hour)
if lastReportIntervalDays == 0 {
return true // this is a perfectly full week!
}
// Ensure that the last interval has at least 6 days, or check the special case, forward DST change,
// when the duration can be shorter than 6 days: 5 days 23 hours.
return lastReportIntervalDays >= 6*24*time.Hour || startTime.AddDate(0, 0, 6).Equal(endTime)
}
155 changes: 155 additions & 0 deletions coderd/insights_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/codersdk"
)

func Test_parseInsightsStartAndEndTime(t *testing.T) {
Expand Down Expand Up @@ -148,3 +150,156 @@ func Test_parseInsightsStartAndEndTime(t *testing.T) {
})
}
}

func Test_parseInsightsInterval_week(t *testing.T) {
t.Parallel()

layout := insightsTimeLayout
sydneyLoc, err := time.LoadLocation("Australia/Sydney") // Random location
require.NoError(t, err)

now := time.Now()
y, m, d := now.Date()
today := time.Date(y, m, d, 0, 0, 0, 0, sydneyLoc)

thisHour := time.Date(y, m, d, now.Hour(), 0, 0, 0, sydneyLoc)
twoHoursAgo := thisHour.Add(-2 * time.Hour)
thirteenDaysAgo := today.AddDate(0, 0, -13)

sixDaysAgo := today.AddDate(0, 0, -6)
nineDaysAgo := today.AddDate(0, 0, -9)

type args struct {
startTime string
endTime string
}
tests := []struct {
name string
args args
wantOk bool
}{
{
name: "Two full weeks",
args: args{
startTime: "2023-08-10T00:00:00+02:00",
endTime: "2023-08-24T00:00:00+02:00",
},
wantOk: true,
},
{
name: "Two weeks",
args: args{
startTime: thirteenDaysAgo.Format(layout),
endTime: twoHoursAgo.Format(layout),
},
wantOk: true,
},
{
name: "One full week",
args: args{
startTime: "2023-09-06T00:00:00+02:00",
endTime: "2023-09-13T00:00:00+02:00",
},
wantOk: true,
},
{
name: "6 days are acceptable",
args: args{
startTime: sixDaysAgo.Format(layout),
endTime: thisHour.Format(layout),
},
wantOk: true,
},
{
name: "Shorter than a full week",
args: args{
startTime: "2023-09-08T00:00:00+02:00",
endTime: "2023-09-13T00:00:00+02:00",
},
wantOk: false,
},
{
name: "9 days (7 + 2) are not acceptable",
args: args{
startTime: nineDaysAgo.Format(layout),
endTime: thisHour.Format(layout),
},
wantOk: false,
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

rw := httptest.NewRecorder()
startTime, endTime, ok := parseInsightsStartAndEndTime(context.Background(), rw, tt.args.startTime, tt.args.endTime)
if !ok {
//nolint:bodyclose
t.Log("Status: ", rw.Result().StatusCode)
t.Log("Body: ", rw.Body.String())
}
require.True(t, ok, "start_time and end_time must be valid")

parsedInterval, gotOk := parseInsightsInterval(context.Background(), rw, "week", startTime, endTime)
if !assert.Equal(t, tt.wantOk, gotOk) {
//nolint:bodyclose
t.Log("Status: ", rw.Result().StatusCode)
t.Log("Body: ", rw.Body.String())
}
if tt.wantOk {
assert.Equal(t, codersdk.InsightsReportIntervalWeek, parsedInterval)
}
})
}
}

func TestLastReportIntervalHasAtLeastSixDays(t *testing.T) {
t.Parallel()

loc, err := time.LoadLocation("Europe/Warsaw")
require.NoError(t, err)

testCases := []struct {
name string
startTime time.Time
endTime time.Time
expected bool
}{
{
name: "perfectly full week",
startTime: time.Date(2023, time.September, 11, 12, 0, 0, 0, loc),
endTime: time.Date(2023, time.September, 18, 12, 0, 0, 0, loc),
expected: true,
},
{
name: "exactly 6 days apart",
startTime: time.Date(2023, time.September, 11, 12, 0, 0, 0, loc),
endTime: time.Date(2023, time.September, 17, 12, 0, 0, 0, loc),
expected: true,
},
{
name: "less than 6 days apart",
startTime: time.Date(2023, time.September, 11, 12, 0, 0, 0, time.UTC),
endTime: time.Date(2023, time.September, 17, 11, 0, 0, 0, time.UTC),
expected: false,
},
{
name: "forward DST change, 5 days and 23 hours apart",
startTime: time.Date(2023, time.March, 22, 12, 0, 0, 0, loc), // A day before DST starts
endTime: time.Date(2023, time.March, 28, 12, 0, 0, 0, loc), // Exactly 6 "days" apart
expected: true,
},
}

for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
result := lastReportIntervalHasAtLeastSixDays(tc.startTime, tc.endTime)
if result != tc.expected {
t.Errorf("Expected %v, but got %v for start time %v and end time %v", tc.expected, result, tc.startTime, tc.endTime)
}
})
}
}
Loading