[usage] Persist usage reports in database by andrew-farries · Pull Request #11343 · gitpod-io/gitpod

andrew-farries · 2022-07-13T10:23:03Z

Description

Store the usage report in the database table created in #11311 whenever usage reconciliation runs.

⚠️ This PR does not yet populate the creditsUsed or generationId fields ⚠️

Related Issue(s)

Part of #10323

How to test

Run the usage component against the database in this PR's preview environment:

Port forward to the database in the preview env
Edit the local config components/usage/config.json for the usage controller to run every 10s.
Run the usage component:

DB_USERNAME=gitpod DB_PASSWORD=<> DB_HOST=localhost DB_PORT=3306 go run . run

(get the password from server environment)

Wait a while for reconciliation to run a couple of times.
Connect to the port-forwarded database with eg mycli and observe the data in d_b_workspace_instance_usage.

Unit tests.

Release Notes

NONE

Werft options:

/werft with-preview

easyCZ · 2022-07-13T10:25:09Z

+	return "d_b_workspace_instance_usage"
+}
+
+func CreateUsageRecords(ctx context.Context, conn *gorm.DB, report map[AttributionID][]WorkspaceInstanceForUsage) error {


I'd suggest the DB layer only takes []WorksapceInstanceForUsage to store, rather than the map, and persists these. The re-mapping can be done in the controller layer. This allows us to keep the db package slightly more general.

Or actually, it should take []WorkspaceInstanceUsage

done. The conversion between reports and []WorkspaceInstanceUsage is done in the reconciler.

easyCZ · 2022-07-13T10:25:33Z

+							// the same workspace again
+							ID:                 instanceID,
+							UsageAttributionID: teamAttributionID,
+							WorkspaceClass:     "default",


There should be a constant for this default value

no longer required now that the CreateUsageRecords function takes WorkspaceInstanceUsage structs directly.

easyCZ · 2022-07-13T10:27:04Z

+}
+
+func TestCanHandleMultipleReportsWithDuplicateEntries(t *testing.T) {
+	conn := dbtest.ConnectForTests(t)


I'd recommend moving the conn into each test scenario, to ensure that after each test the data is deleted before the next test run

easyCZ · 2022-07-13T10:27:52Z

 	}
 	log.Infof("Wrote usage report into %s", filepath.Join(dir, stat.Name()))

+	db.CreateUsageRecords(ctx, u.conn, report)


Should handle error

easyCZ · 2022-07-13T10:28:54Z

+	for _, scenario := range scenarios {
+		t.Run(scenario.Name, func(t *testing.T) {
+			for _, report := range scenario.Reports {
+				require.NoError(t, db.CreateUsageRecords(ctx, conn, report))


You may need the helpers from dbtest which add hooks into the created records to delete them once the test completes. See dbtest.CreateWorkspace for example

I've added t.Cleanup calls to each subtest to delete the usage records between each subtest.

Commented in https://github.com/gitpod-io/gitpod/pull/11343/files#r919996694

easyCZ · 2022-07-13T12:04:16Z

+		conn := dbtest.ConnectForTests(t)
+		t.Run(scenario.Name, func(t *testing.T) {
+			t.Cleanup(func() {
+				require.NoError(t, conn.Where("1=1").Delete(&db.WorkspaceInstanceUsage{}).Error)


This was the cause of the problems with flaky tests. When more tests exist and do this, it deletes the data needed for other runs of the test. That's why the current approach adds a hook for each ID created to only deleted the IDs created in each test.

🤔

None of these tests call t.Parallel, so AIUI the tests in this file will run in series, and within each test each subtest will also run in series. So where is the risk of having a subtest clearing the table after it finishes?

Is it tests running in different packages (which will be run in parallel by go test) that is the concern here?

But tests in different files will be run concurrently. At some point, we'll add a test which also interacts with the WorkspaceInstanceUsage table and we'll be in trouble. The problem comes from the following interleaving:

test1: Create 10 records
test2: create 2 records
test1: Check we've got 10 records - fail, got 12

The exact same issue happened previously, we've added tests for d_b_workspace_instance and then we also added tests for the controller (at higher level) which also created d_b_workspace_instance records

#10971 has extra context on this, including a link to some literature

But tests in different files will be run concurrently.

I don't think that's correct. See for example here:

By default, execution of test code using the testing package will be done sequentially. However, note that it is only the tests within a given package that run sequentially.

If tests from multiple packages are specified, the tests will be run in parallel at the package level. For example, imagine there are two packages, package a and package b. The test code in package a will be run sequentially, and the test code in package b will also be run sequentially. However, the tests for package a and package b will be run in parallel. Let’s look more closely into how these will be run in parallel.

So I guess I'm struggling to understand how test cleanup code within a package can interfere with other tests in the same package if there is no t.Parallel anywhere. Maybe there is a race in actually deleting the rows from the database?

The issues we observed were across different packages. And the same will happen here once we add proper tests to the controller package.

test cleanup changed to only remove records that were created by each test.

werft-gitpod-dev-com · 2022-07-13T13:57:32Z

started the job as gitpod-build-af-store-usage-data.7 because the annotations in the pull request description changed
(with .werft/ from main)

Add a GORM type that represents the "d_b_workspace_instance_usage" table and methods for working with it.

geropl · 2022-07-14T07:15:50Z

+)
+
+type WorkspaceInstanceUsage struct {
+	WorkspaceID   uuid.UUID     `gorm:"primary_key;column:workspaceId;type:char;size:36;" json:"workspaceId"`


Does it make sense to merge the other PR with the rename into this one...? I though it had already been merged.

💡 Or has the table defintion, but not the Go struct?

geropl · 2022-07-14T07:16:40Z

Doing a quick test...

geropl · 2022-07-14T07:26:59Z

Works as advertised. I understood that credits and generationId will be set later.

Only thing I noticed is that stoppedAt is not set, while the workspace_instance is definitely stopped (and has a stoppedTime).

@andrew-farries Any idea why that might be?

Rename the misnamed primary key `workspaceId` -> `instanceId`. Usage based billing is unreleased so just drop the table and recreate it - no data to migrate. Also add index names.

In the case where the instance has stopped.

andrew-farries · 2022-07-14T10:28:53Z

Only thing I noticed is that stoppedAt is not set, while the workspace_instance is definitely stopped (and has a stoppedTime).

Thanks. I've fixed this and added tests for the conversion from a usage report to instance usage records. (f25999d and b9016e9).

geropl

Note that the current preview env is in an undefined state.

But I applied the migration manually, ran the test and 🎉 : it worked. 🧘

andrew-farries · 2022-07-14T11:57:15Z

/werft run with-clean-slate-deployment=true with-preview=true

👍 started the job as gitpod-build-af-store-usage-data.12
(with .werft/ from main)

liam-j-bennett · 2022-07-14T12:28:39Z

/hold this is blocking the merge queue of a fix that fixes the merge queue 😢 I'll unhold when I've merged

liam-j-bennett · 2022-07-14T12:31:28Z

/unhold You will need to restart the werft job. Apologies for the downtime 🤦

andrew-farries · 2022-07-14T12:32:29Z

/werft run with-clean-slate-deployment=true with-preview=true

👍 started the job as gitpod-build-af-store-usage-data.13
(with .werft/ from main)

geropl · 2022-07-14T13:02:31Z

@andrew-farries What about runnign with-preview=false? I already tested twice and am confident it works, so let's get this in! 🚢

werft-gitpod-dev-com · 2022-07-14T13:04:34Z

started the job as gitpod-build-af-store-usage-data.14 because the annotations in the pull request description changed
(with .werft/ from main)

andrew-farries · 2022-07-14T13:24:33Z

/unhold

andrew-farries requested a review from a team July 13, 2022 10:23

roboquat added release-note-none size/L labels Jul 13, 2022

github-actions Bot added the team: webapp Issue belongs to the WebApp team label Jul 13, 2022

andrew-farries force-pushed the af/store-usage-data branch from e7340d6 to 31c0b55 Compare July 13, 2022 10:28

easyCZ reviewed Jul 13, 2022

View reviewed changes

andrew-farries force-pushed the af/store-usage-data branch from 31c0b55 to 8cf9560 Compare July 13, 2022 11:40

andrew-farries requested a review from easyCZ July 13, 2022 11:43

easyCZ reviewed Jul 13, 2022

View reviewed changes

andrew-farries force-pushed the af/store-usage-data branch from 1f2fc4c to cdaf021 Compare July 13, 2022 13:57

andrew-farries mentioned this pull request Jul 13, 2022

Setup usage datastore and store usage #10323

Closed

3 tasks

Andrew Farries added 4 commits July 13, 2022 14:07

Off topic: Use type alias in return type

669c840

Add new type to db package

556699a

Add a GORM type that represents the "d_b_workspace_instance_usage" table and methods for working with it.

Add tests for usage record creation

fa3a90c

Store usage after reconciler runs

844259b

andrew-farries force-pushed the af/store-usage-data branch from cdaf021 to 844259b Compare July 13, 2022 14:07

andrew-farries mentioned this pull request Jul 13, 2022

[usage] Rename misnamed column in usage table #11353

Merged

1 task

geropl reviewed Jul 14, 2022

View reviewed changes

geropl self-assigned this Jul 14, 2022

Andrew Farries added 4 commits July 14, 2022 10:23

Rename column in database entity

6175781

Rename the misnamed primary key `workspaceId` -> `instanceId`. Usage based billing is unreleased so just drop the table and recreate it - no data to migrate. Also add index names.

Update refs to column name in usage component

09ac3dd

Create valid sql.NullTime

f25999d

In the case where the instance has stopped.

Add tests for usage report/usage record conversion

b9016e9

andrew-farries force-pushed the af/store-usage-data branch from 373118e to b9016e9 Compare July 14, 2022 10:23

roboquat added size/XL and removed size/L labels Jul 14, 2022

geropl approved these changes Jul 14, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Jul 14, 2022

roboquat removed the do-not-merge/hold label Jul 14, 2022

roboquat merged commit 4692d0a into main Jul 14, 2022

roboquat deleted the af/store-usage-data branch July 14, 2022 13:25

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jul 19, 2022

Uh oh!

Conversation

andrew-farries commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Werft options:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easyCZ Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

werft-gitpod-dev-com Bot commented Jul 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geropl commented Jul 14, 2022

Uh oh!

geropl commented Jul 14, 2022

Uh oh!

andrew-farries commented Jul 14, 2022

Uh oh!

geropl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-farries commented Jul 14, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liam-j-bennett commented Jul 14, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liam-j-bennett commented Jul 14, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-farries commented Jul 14, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geropl commented Jul 14, 2022

Uh oh!

andrew-farries commented Jul 13, 2022 •

edited

Loading

easyCZ Jul 13, 2022 •

edited

Loading

geropl left a comment •

edited

Loading

andrew-farries commented Jul 14, 2022 •

edited by werft-gitpod-dev-com Bot

Loading

liam-j-bennett commented Jul 14, 2022 •

edited by werft-gitpod-dev-com Bot

Loading

liam-j-bennett commented Jul 14, 2022 •

edited by werft-gitpod-dev-com Bot

Loading

andrew-farries commented Jul 14, 2022 •

edited by werft-gitpod-dev-com Bot

Loading