Skip to content

[usage] store more data for in usage entry #12736

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.

Sign up for GitHub

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 1 commit into from
Sep 7, 2022
Merged

[usage] store more data for in usage entry #12736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/gitpod-protocol/src/usage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export interface WorkspaceInstanceUsageData {
contextURL: string;
startTime: string;
endTime?: string;
userId: string;
userName: string;
userAvatarURL: string;
}
Expand Down
7 changes: 4 additions & 3 deletions components/usage/pkg/apiv1/usage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,12 @@ func newUsageFromInstance(instance db.WorkspaceInstanceForUsage, pricer *Workspa
WorkspaceId: instance.WorkspaceID,
WorkspaceType: instance.Type,
WorkspaceClass: instance.WorkspaceClass,
ContextURL: "",
ContextURL: instance.ContextURL,
StartTime: startedTime,
EndTime: endTime,
UserName: "",
UserAvatarURL: "",
UserID: instance.UserID,
UserName: instance.UserName,
UserAvatarURL: instance.UserAvatarURL,
})
if err != nil {
return db.Usage{}, fmt.Errorf("failed to serialize workspace instance metadata: %w", err)
Expand Down
191 changes: 6 additions & 185 deletions components/usage/pkg/apiv1/usage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@ package apiv1
import (
"context"
"database/sql"
"reflect"
"testing"
"time"

"github.com/gitpod-io/gitpod/usage/pkg/contentservice"

"github.com/gitpod-io/gitpod/common-go/baseserver"
v1 "github.com/gitpod-io/gitpod/usage-api/v1"
"github.com/gitpod-io/gitpod/usage/pkg/db"
Expand Down Expand Up @@ -375,182 +372,6 @@ func TestInstanceToUsageRecords(t *testing.T) {
}
}

func TestReportGenerator_GenerateUsageReport(t *testing.T) {
startOfMay := time.Date(2022, 05, 1, 0, 00, 00, 00, time.UTC)
startOfJune := time.Date(2022, 06, 1, 0, 00, 00, 00, time.UTC)

teamID := uuid.New()
scenarioRunTime := time.Date(2022, 05, 31, 23, 00, 00, 00, time.UTC)

instances := []db.WorkspaceInstance{
// Ran throughout the reconcile period
dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{
ID: uuid.New(),
UsageAttributionID: db.NewTeamAttributionID(teamID.String()),
StartedTime: db.NewVarcharTime(time.Date(2022, 05, 1, 00, 01, 00, 00, time.UTC)),
StoppingTime: db.NewVarcharTime(time.Date(2022, 06, 1, 1, 0, 0, 0, time.UTC)),
}),
// Still running
dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{
ID: uuid.New(),
UsageAttributionID: db.NewTeamAttributionID(teamID.String()),
StartedTime: db.NewVarcharTime(time.Date(2022, 05, 30, 00, 01, 00, 00, time.UTC)),
}),
// No creation time, invalid record, ignored
dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{
ID: uuid.New(),
UsageAttributionID: db.NewTeamAttributionID(teamID.String()),
StoppingTime: db.NewVarcharTime(time.Date(2022, 06, 1, 1, 0, 0, 0, time.UTC)),
}),
}

conn := dbtest.ConnectForTests(t)
dbtest.CreateWorkspaceInstances(t, conn, instances...)

nowFunc := func() time.Time { return scenarioRunTime }
generator := &ReportGenerator{
nowFunc: nowFunc,
conn: conn,
pricer: DefaultWorkspacePricer,
}

report, err := generator.GenerateUsageReport(context.Background(), startOfMay, startOfJune)
require.NoError(t, err)

require.Equal(t, nowFunc(), report.GenerationTime)
require.Equal(t, startOfMay, report.From)
// require.Equal(t, startOfJune, report.To) TODO(gpl) This is not true anymore - does it really make sense to test for it?
require.Len(t, report.InvalidSessions, 0)
require.Len(t, report.UsageRecords, 2)
}

func TestReportGenerator_GenerateUsageReportTable(t *testing.T) {
teamID := uuid.New()
instanceID := uuid.New()

Must := func(ti db.VarcharTime, err error) db.VarcharTime {
if err != nil {
t.Fatal(err)
}
return ti
}
Timestamp := func(timestampAsStr string) db.VarcharTime {
return Must(db.NewVarcharTimeFromStr(timestampAsStr))
}
type Expectation struct {
custom func(t *testing.T, report contentservice.UsageReport)
usageRecords []db.WorkspaceInstanceUsage
}

type TestCase struct {
name string
from time.Time
to time.Time
runtime time.Time
instances []db.WorkspaceInstance
expectation Expectation
}
tests := []TestCase{
{
name: "real example taken from DB: runtime _before_ instance.startedTime",
from: time.Date(2022, 8, 1, 0, 00, 00, 00, time.UTC),
to: time.Date(2022, 9, 1, 0, 00, 00, 00, time.UTC),
runtime: Timestamp("2022-08-17T09:38:28Z").Time(),
instances: []db.WorkspaceInstance{
dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{
ID: instanceID,
UsageAttributionID: db.NewTeamAttributionID(teamID.String()),
CreationTime: Timestamp("2022-08-17T09:40:47.316Z"),
StartedTime: Timestamp("2022-08-17T09:40:53.115Z"),
StoppingTime: Timestamp("2022-08-17T09:42:36.292Z"),
StoppedTime: Timestamp("2022-08-17T09:43:04.874Z"),
}),
},
expectation: Expectation{
usageRecords: nil,
// usageRecords: []db.WorkspaceInstanceUsage{
// {
// InstanceID: instanceID,
// AttributionID: db.NewTeamAttributionID(teamID.String()),
// StartedAt: Timestamp("2022-08-17T09:40:53.115Z").Time(),
// StoppedAt: sql.NullTime{ Time: Timestamp("2022-08-17T09:43:04.874Z").Time(), Valid: true },
// WorkspaceClass: "default",
// CreditsUsed: 3.0,
// },
// },
},
},
{
name: "same as above, but with runtime _after_ startedTime",
from: time.Date(2022, 8, 1, 0, 00, 00, 00, time.UTC),
to: time.Date(2022, 9, 1, 0, 00, 00, 00, time.UTC),
runtime: Timestamp("2022-08-17T09:41:00Z").Time(),
instances: []db.WorkspaceInstance{
dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{
ID: instanceID,
UsageAttributionID: db.NewTeamAttributionID(teamID.String()),
CreationTime: Timestamp("2022-08-17T09:40:47.316Z"),
StartedTime: Timestamp("2022-08-17T09:40:53.115Z"),
StoppingTime: Timestamp("2022-08-17T09:42:36.292Z"),
StoppedTime: Timestamp("2022-08-17T09:43:04.874Z"),
}),
},
expectation: Expectation{
usageRecords: []db.WorkspaceInstanceUsage{
{
InstanceID: instanceID,
AttributionID: db.NewTeamAttributionID(teamID.String()),
StartedAt: Timestamp("2022-08-17T09:40:53.115Z").Time(),
StoppedAt: sql.NullTime{Time: Timestamp("2022-08-17T09:41:00Z").Time(), Valid: true},
WorkspaceClass: "default",
CreditsUsed: 0.019444444444444445,
},
},
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
conn := dbtest.ConnectForTests(t)
dbtest.CreateWorkspaceInstances(t, conn, test.instances...)

nowFunc := func() time.Time { return test.runtime }
generator := &ReportGenerator{
nowFunc: nowFunc,
conn: conn,
pricer: DefaultWorkspacePricer,
}

report, err := generator.GenerateUsageReport(context.Background(), test.from, test.to)
require.NoError(t, err)

require.Equal(t, test.runtime, report.GenerationTime)
require.Equal(t, test.from, report.From)
// require.Equal(t, test.to, report.To) TODO(gpl) This is not true anymore - does it really make sense to test for it?

// These invariants should always be true:
// 1. No negative usage
for _, rec := range report.UsageRecords {
if rec.CreditsUsed < 0 {
t.Error("Got report with negative credits!")
}
}

if !reflect.DeepEqual(test.expectation.usageRecords, report.UsageRecords) {
t.Errorf("report.UsageRecords: expected %v but got %v", test.expectation.usageRecords, report.UsageRecords)
}

// Custom expectations
customTestFunction := test.expectation.custom
if customTestFunction != nil {
customTestFunction(t, report)
require.NoError(t, err)
}
})
}
}

func TestUsageService_ReconcileUsageWithLedger(t *testing.T) {
dbconn := dbtest.ConnectForTests(t)
from := time.Date(2022, 05, 1, 0, 00, 00, 00, time.UTC)
Expand Down Expand Up @@ -673,11 +494,11 @@ func TestReconcileWithLedger(t *testing.T) {
WorkspaceId: instance.WorkspaceID,
WorkspaceType: instance.Type,
WorkspaceClass: instance.WorkspaceClass,
ContextURL: "",
ContextURL: instance.ContextURL,
StartTime: db.TimeToISO8601(instance.StartedTime.Time()),
EndTime: "",
UserName: "",
UserAvatarURL: "",
UserName: instance.UserName,
UserAvatarURL: instance.UserAvatarURL,
}))
require.EqualValues(t, expectedUsage, inserts[0])
})
Expand Down Expand Up @@ -731,11 +552,11 @@ func TestReconcileWithLedger(t *testing.T) {
WorkspaceId: instance.WorkspaceID,
WorkspaceType: instance.Type,
WorkspaceClass: instance.WorkspaceClass,
ContextURL: "",
ContextURL: instance.ContextURL,
StartTime: db.TimeToISO8601(instance.StartedTime.Time()),
EndTime: "",
UserName: "",
UserAvatarURL: "",
UserName: instance.UserName,
UserAvatarURL: instance.UserAvatarURL,
}))
require.EqualValues(t, expectedUsage, updates[0])
})
Expand Down
77 changes: 77 additions & 0 deletions components/usage/pkg/db/dbtest/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright (c) 2022 Gitpod GmbH. All rights reserved.
// Licensed under the GNU Affero General Public License (AGPL).
// See License-AGPL.txt in the project root for license information.

package dbtest

import (
"testing"
"time"

"github.com/gitpod-io/gitpod/usage/pkg/db"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"gorm.io/gorm"
)

type User struct {
ID uuid.UUID `gorm:"primary_key;column:id;type:char;size:36;"`
AvatarURL string `gorm:"column:avatarUrl;type:char;size:255;"`
Name string `gorm:"column:name;type:char;size:255;"`
FullName string `gorm:"column:fullName;type:char;size:255;"`
CreationDate db.VarcharTime `gorm:"column:creationDate;type:varchar;size:255;"`

// user has more field but we don't care here as they are just used in tests.
}
Comment on lines +17 to +25
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the db package, not the dbtest package

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 put it here, because we don't need this in production code, it's only used for setting up testing state in DB.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I guess I can live with this, but I'd generally define it on the db package as someone else will come along and not find the User object there so they would go and define it again


func (user *User) TableName() string {
return "d_b_user"
}

func NewUser(t *testing.T, user User) User {
Copy link
Member

Choose a reason for hiding this comment

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

It's bit of an anti-pattern to have a testing.T used in a non test package. Normally we'd pull this into a separate package - it also has the benefit of not being included in the resulting binary if it's in a different package (not referenced by "prod" code)

Copy link
Member Author

Choose a reason for hiding this comment

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

it's in a testing package

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I got confused. The User model definition should be moved out of the dbtest package into the db package

t.Helper()

result := User{
ID: uuid.New(),
AvatarURL: "https://avatars.githubusercontent.com/u/9071",
Name: "HomerJSimpson",
FullName: "Homer Simpson",
CreationDate: db.NewVarcharTime(time.Now()),
}

if user.ID != uuid.Nil {
result.ID = user.ID
}

if user.AvatarURL != "" {
result.AvatarURL = user.AvatarURL
}
if user.Name != "" {
result.Name = user.Name
}
if user.FullName != "" {
result.FullName = user.FullName
}

return result
}

func CreatUser(t *testing.T, conn *gorm.DB, user ...User) []User {
t.Helper()

var records []User
var ids []uuid.UUID
for _, u := range user {
record := NewUser(t, u)
records = append(records, record)
ids = append(ids, record.ID)
}

require.NoError(t, conn.CreateInBatches(&records, 1000).Error)

t.Cleanup(func() {
require.NoError(t, conn.Where(ids).Delete(&User{}).Error)
})

return records
}
32 changes: 31 additions & 1 deletion components/usage/pkg/db/dbtest/workspace_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package dbtest

import (
"context"
"database/sql"
"testing"
"time"
Expand All @@ -23,7 +24,7 @@ func NewWorkspaceInstance(t *testing.T, instance db.WorkspaceInstance) db.Worksp
t.Helper()

id := uuid.New()
if instance.ID.ID() != 0 { // empty value
if instance.ID != uuid.Nil {
Copy link
Member

Choose a reason for hiding this comment

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

Cool! I didn't know about this.

id = instance.ID
}

Expand Down Expand Up @@ -120,3 +121,32 @@ func CreateWorkspaceInstances(t *testing.T, conn *gorm.DB, instances ...db.Works

return records
}

// ListWorkspaceInstancesInRange filters out instances by workspaceID to make tests robust and work only on their own data
func ListWorkspaceInstancesInRange(t *testing.T, conn *gorm.DB, from, to time.Time, workspaceID string) []db.WorkspaceInstanceForUsage {
all, err := db.ListWorkspaceInstancesInRange(context.Background(), conn, from, to)
require.NoError(t, err)
return filterByWorkspaceId(all, workspaceID)
}

func FindStoppedWorkspaceInstancesInRange(t *testing.T, conn *gorm.DB, from, to time.Time, workspaceID string) []db.WorkspaceInstanceForUsage {
all, err := db.FindStoppedWorkspaceInstancesInRange(context.Background(), conn, from, to)
require.NoError(t, err)
return filterByWorkspaceId(all, workspaceID)
}

func FindRunningWorkspaceInstances(t *testing.T, conn *gorm.DB, workspaceID string) []db.WorkspaceInstanceForUsage {
all, err := db.FindRunningWorkspaceInstances(context.Background(), conn)
require.NoError(t, err)
return filterByWorkspaceId(all, workspaceID)
}

func filterByWorkspaceId(all []db.WorkspaceInstanceForUsage, workspaceID string) []db.WorkspaceInstanceForUsage {
result := []db.WorkspaceInstanceForUsage{}
for _, candidate := range all {
if candidate.WorkspaceID == workspaceID {
result = append(result, candidate)
}
}
return result
}
1 change: 1 addition & 0 deletions components/usage/pkg/db/usage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type WorkspaceInstanceUsageData struct {
ContextURL string `json:"contextURL"`
StartTime string `json:"startTime"`
EndTime string `json:"endTime"`
UserID uuid.UUID `json:"userId"`
UserName string `json:"userName"`
UserAvatarURL string `json:"userAvatarURL"`
}
Expand Down
Loading