Added Usage table #12584

svenefftinge · 2022-09-01T14:08:41Z

Description

Introduces a usage table to keep track of any account's credit balance.

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

/werft with-preview

easyCZ · 2022-09-01T15:03:36Z

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

+                \`workspaceInstanceId\` char(36) NULL,
+                \`status\` char(10) NOT NULL DEFAULT 'finalized',
+                \`metadata\` text NOT NULL,
+                \`_lastModified\` timestamp(6) NOT NULL,


Do we also want a created timestamp?

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

geropl · 2022-09-02T07:07:16Z

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

+                \`_lastModified\` timestamp(6) NOT NULL,
+
+                INDEX \`IDX_usage__attribution_id\` (\`attributionId\`),
+                INDEX \`IDX_usage_workspaceInstanceId\` (\`workspaceInstanceId\`),


Suggested change

INDEX \`IDX_usage_workspaceInstanceId\` (\`workspaceInstanceId\`),

INDEX \`IDX_usage_workspaceInstanceId\` (\`workspaceInstanceId\`),

INDEX \`IDX_db_sync\` (\`_lastModified\`),

Adding the index for db-sync.

easyCZ · 2022-09-02T07:07:32Z

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

+                \`description\` varchar(255) NOT NULL,
+                \`creditCents\` bigint NOT NULL,
+                \`effectiveTime\` varchar(255) NOT NULL,
+                \`creationTime\` varchar(255) NOT NULL,


Should this have an AutoSet on create property?

We don't usually do this for such columns. Also, it might be good to use the same time for all the entries we add in one reconciliation run.

For me this was intended to be a sort of record metadata. Basically storing two timestamps - First creation, and Last Update (and automatically maintaining this by the DB) would give us an audit when something did change or when it first entered the DB.

For our custom application logic, I'd use the effectiveTime but I'm open on this.

Ok, I was leaning to make it similar to some of the creationTime fields we already have. But I wasn't too sure about it's usefulness and semantic anyway. I'll change it to _created timestamp(6) ....

easyCZ · 2022-09-02T07:07:47Z

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

+                \`workspaceInstanceId\` char(36) NULL,
+                \`draft\` BOOLEAN NOT NULL,
+                \`metadata\` text NULL,
+                \`_lastModified\` timestamp(6) NOT NULL,


This should have an auto-update property on it

It has implicitly. Should I make it explicit?

will make it explicit

I think ensuring that the DB does this for us guards against mistakes we make in code and forget to update the timestamp. It would also reduce the amount of code (the setting of the last modified).

I'd personally like that the DB (explicit) takes care of this rather than doing it our application layer.

Yes, I meant it does it for us, because timestamp(6) NOT NULL does insert/update the current timestamp. But writing it explicitly into the migration file is better.

What I'm after is this:

ts TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,

Is that the behavuour the current statement would have?

yes. @andrew-farries was sharing this the other day.
But it's better to be explicit.

easyCZ · 2022-09-02T07:08:53Z

components/usage/pkg/db/dbtest/usage.go

+
+	require.NoError(t, conn.CreateInBatches(&records, 1000).Error)
+
+	t.Cleanup(func() {


Thanks for adding this! This ensures that tests clean up after themselves and that multiple tests against hte same table do not interfere

easyCZ · 2022-09-02T07:09:46Z

components/usage/pkg/db/usage.go

+	var usageRecords []Usage
+	result := db.
+		WithContext(ctx).
+		Table((&Usage{}).TableName()).


I think this shouldn't be needed. It "should" be able to reflect that from the Find(&usageRecords) statement and check if the struct implements the TableName() interface.

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts

easyCZ

This now looks good to me.

/hold in case there's other feedback

svenefftinge · 2022-09-02T09:29:55Z

Thanks, @easyCZ! Please remove the label yourself anytime, if you want to have this on main in order to rebase on top of it.

easyCZ · 2022-09-02T09:32:50Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/M labels Sep 1, 2022

easyCZ reviewed Sep 1, 2022

View reviewed changes

geropl reviewed Sep 1, 2022

View reviewed changes

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts Outdated Show resolved Hide resolved

svenefftinge force-pushed the se/usage-table branch from 36ce643 to c2767e6 Compare September 1, 2022 16:01

roboquat added size/L and removed size/M labels Sep 1, 2022

svenefftinge force-pushed the se/usage-table branch from c2767e6 to 22fdc4e Compare September 2, 2022 06:56

svenefftinge marked this pull request as ready for review September 2, 2022 07:04

svenefftinge requested a review from a team September 2, 2022 07:04

roboquat removed the do-not-merge/work-in-progress label Sep 2, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 2, 2022

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Sep 2, 2022

geropl reviewed Sep 2, 2022

View reviewed changes

easyCZ reviewed Sep 2, 2022

View reviewed changes

geropl reviewed Sep 2, 2022

View reviewed changes

components/gitpod-db/src/typeorm/migration/1662040283793-AddUsageTable.ts Show resolved Hide resolved

svenefftinge closed this Sep 2, 2022

svenefftinge force-pushed the se/usage-table branch from 22fdc4e to ad355c4 Compare September 2, 2022 07:56

roboquat added size/XS and removed size/L labels Sep 2, 2022

Added Usage table

ef34ca9

svenefftinge reopened this Sep 2, 2022

roboquat added size/L and removed size/XS labels Sep 2, 2022

easyCZ approved these changes Sep 2, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Sep 2, 2022

roboquat removed the do-not-merge/hold label Sep 2, 2022

roboquat merged commit f54482b into main Sep 2, 2022

roboquat deleted the se/usage-table branch September 2, 2022 09:33

svenefftinge mentioned this pull request Sep 2, 2022

[usage] Usage ledger API protos #12618

Merged

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Usage table #12584

Added Usage table #12584

svenefftinge commented Sep 1, 2022 •

edited

Loading

easyCZ Sep 1, 2022

geropl Sep 2, 2022

geropl Sep 2, 2022

easyCZ Sep 2, 2022

svenefftinge Sep 2, 2022

easyCZ Sep 2, 2022

svenefftinge Sep 2, 2022

easyCZ Sep 2, 2022

svenefftinge Sep 2, 2022

svenefftinge Sep 2, 2022

easyCZ Sep 2, 2022

svenefftinge Sep 2, 2022

easyCZ Sep 2, 2022

svenefftinge Sep 2, 2022

easyCZ Sep 2, 2022

easyCZ Sep 2, 2022

easyCZ left a comment

svenefftinge commented Sep 2, 2022

easyCZ commented Sep 2, 2022

	INDEX \`IDX_usage_workspaceInstanceId\` (\`workspaceInstanceId\`),
	INDEX \`IDX_usage_workspaceInstanceId\` (\`workspaceInstanceId\`),
	INDEX \`IDX_db_sync\` (\`_lastModified\`),


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

		t.Cleanup(func() {

Added Usage table #12584

Added Usage table #12584

Conversation

svenefftinge commented Sep 1, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easyCZ left a comment

Choose a reason for hiding this comment

svenefftinge commented Sep 2, 2022

easyCZ commented Sep 2, 2022

svenefftinge commented Sep 1, 2022 •

edited

Loading