[usage] Use attribution ID to reduce DB queries for usage report #10938

easyCZ · 2022-06-27T15:14:52Z

Description

Updates usage controller flow to do the following:

Find all Workspace Instances (with time bounds) which have an usageAttributionId set.
Groups these (in-code) by Attribution ID to create the report

We can't use SQL level group-by because it can only be used with summary functions (like count). The secondary reason is that the Billing controller (which runs after the usage controller) needs to given the full list of WorkspaceInstances to exclude any WorkspaceInstances which it may have already billed for (in another invoice, for example). For this, it has to get the raw data-set and can't receive an aggregation.

Related Issue(s)

Part of Use attributionId when listing workspace instances for usage #10922

How to test

Unit tests
Can be run against staging by port forwarding

Release Notes

NONE

Documentation

NONE

Werft options:

/werft with-preview

jankeromnes · 2022-06-28T09:13:30Z

components/usage/pkg/db/workspace_instance.go

@@ -79,6 +79,7 @@ func ListWorkspaceInstancesInRange(ctx context.Context, conn *gorm.DB, from, to
 		).
 		Where("creationTime < ?", TimeToISO8601(to)).
 		Where("startedTime != ?", "").
+		Where("usageAttributionId != ?", "").


How will we want to handle potentially un-attributed instances?

Given the usageAttributionId should always be present (the premise of your PR). The check here mostly ensures we only run on "new" data which does have the attribution set

jankeromnes · 2022-06-28T09:15:48Z

components/usage/pkg/controller/reconciler.go

+func groupInstancesByAttributionID(instances []db.WorkspaceInstance) map[db.AttributionID][]db.WorkspaceInstance {
+	result := map[db.AttributionID][]db.WorkspaceInstance{}
+	for _, instance := range instances {
+		if _, ok := result[instance.UsageAttributionID]; !ok {
+			result[instance.UsageAttributionID] = []db.WorkspaceInstance{}
+		}
+
+		result[instance.UsageAttributionID] = append(result[instance.UsageAttributionID], instance)
+	}
+
+	return result
+}


Are you sure it makes sense performance-wise to re-implement a SQL "group by" in Go? I notice this adds another full iteration loop over all the very numerous instances of the current month. I think, given the sheer number of instances, we may want to limit all these iterations to ideally just a single one-shot iteration.

I wonder if maybe this "group by" should be part of the query. Relatedly, have you had a chance to chat with @geropl about the query's performance, and how to efficiently index and shard it? 👀

You can only use group-by with summary functions (like count). That's because a group-by takes multiple rows, and turns them into a single row by aggregating them (and applying the summary function to it).

In practice, we need the full dataset anyway because the billing controller needs to do another pass to drop WSI which have already been billed.

Happy to go into more details on this.

easyCZ · 2022-06-28T11:08:56Z

I've moved this to Draft as I've added a bunch of fixes for flaky tests which I need to pull into a separate PR. The main logic is still reviewable, but I'll move it back to review once cleaned up.

easyCZ · 2022-06-28T13:36:08Z

Fixed up and rebased, ready for review again.

geropl · 2022-06-28T15:07:04Z

Starting to review now...

geropl · 2022-06-28T15:16:28Z

components/usage/pkg/controller/reconciler.go

-	}
+	for attribution, instances := range u {
+		entity, id := attribution.Values()
+		if entity != db.AttributionEntity_Team {


nit: A comment that we handle this in the future would be golden 👍

geropl · 2022-06-28T15:17:52Z

components/usage/pkg/controller/reconciler.go

-			TeamID:     membership.TeamID,
-			Workspaces: workspacesByOwnerID[userID],
-		})
+		attributedUsage[id] = int64(runtime)


This feels "dangerous"/off given that we do use uint64 for usage in other places in the code base. Why not make attributedUsage accumulate in uint64 and stick to that datatype everywhere? 🤔

Happy to update to unit64 everywhere (but Stripe only accepts int64, not uint64). But the reason for using it here is to limit changes to the existing Usage Report reconcile logic with stripe in this PR. That happens here https://github.com/gitpod-io/gitpod/pull/10938/files#diff-b2499b7086d5733f081dcce586e0ff0e77206dd5d8fd6ede638e3fc8f284c798L25-L32 and it currently has int64 as the interface.
I'll update these in a follow-up PR (minus the Stripe API call which will have to be int64) if that's acceptable.

Ok, fine with this for now.

geropl · 2022-06-28T15:25:06Z

Not necessarily sth for this PR, but: before we enable this again I'd love to get away from the fixed interval, and instead wait for the current run to finish before we start a new one (code ref).

geropl · 2022-06-28T15:25:49Z

components/usage/pkg/db/workspace_instance.go

@@ -79,6 +79,7 @@ func ListWorkspaceInstancesInRange(ctx context.Context, conn *gorm.DB, from, to
 		).
 		Where("creationTime < ?", TimeToISO8601(to)).
 		Where("startedTime != ?", "").
+		Where("usageAttributionId != ?", "").
 		FindInBatches(&instancesInBatch, 1000, func(_ *gorm.DB, _ int) error {


This should work out fine. 👍

geropl

👍

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Jun 27, 2022

easyCZ changed the base branch from main to mp/usage-attribution-id June 27, 2022 15:15

roboquat added the size/L label Jun 27, 2022

easyCZ force-pushed the mp/usage-attribution-id branch from 2a54e55 to b966efd Compare June 27, 2022 15:17

roboquat added size/XL size/XXL and removed size/L size/XL labels Jun 28, 2022

easyCZ force-pushed the mp/usage-attribution-id branch from b966efd to d416916 Compare June 28, 2022 08:08

easyCZ force-pushed the mp/usage-use-attribution-id branch 2 times, most recently from 5baf448 to ee88344 Compare June 28, 2022 08:10

roboquat added size/XL and removed size/XXL labels Jun 28, 2022

jankeromnes mentioned this pull request Jun 28, 2022

[usage] Add usageAttributionID to WorkspaceInstance model (in go) #10927

Merged

1 task

jankeromnes reviewed Jun 28, 2022

View reviewed changes

easyCZ force-pushed the mp/usage-attribution-id branch from d416916 to c48fee9 Compare June 28, 2022 09:42

Base automatically changed from mp/usage-attribution-id to main June 28, 2022 09:47

easyCZ force-pushed the mp/usage-use-attribution-id branch from ee88344 to 9ccf14e Compare June 28, 2022 09:49

easyCZ marked this pull request as ready for review June 28, 2022 09:51

easyCZ requested a review from a team June 28, 2022 09:51

roboquat removed the do-not-merge/work-in-progress label Jun 28, 2022

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

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

easyCZ marked this pull request as draft June 28, 2022 11:08

roboquat added the do-not-merge/work-in-progress label Jun 28, 2022

easyCZ force-pushed the mp/usage-use-attribution-id branch from 1610f34 to a77bbc5 Compare June 28, 2022 13:29

easyCZ force-pushed the mp/usage-use-attribution-id branch from a77bbc5 to 7d7be79 Compare June 28, 2022 13:31

easyCZ marked this pull request as ready for review June 28, 2022 13:32

roboquat removed the do-not-merge/work-in-progress label Jun 28, 2022

[usage] Use attribution ID to reduce DB queries for usage report

bc5955d

easyCZ force-pushed the mp/usage-use-attribution-id branch from cb1db57 to bc5955d Compare June 28, 2022 13:35

geropl self-assigned this Jun 28, 2022

geropl reviewed Jun 28, 2022

View reviewed changes

geropl approved these changes Jun 28, 2022

View reviewed changes

roboquat merged commit 2e7d2ef into main Jun 28, 2022

roboquat deleted the mp/usage-use-attribution-id branch June 28, 2022 15:53

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[usage] Use attribution ID to reduce DB queries for usage report #10938

[usage] Use attribution ID to reduce DB queries for usage report #10938

easyCZ commented Jun 27, 2022 •

edited

Loading

jankeromnes Jun 28, 2022

easyCZ Jun 28, 2022

jankeromnes Jun 28, 2022 •

edited

Loading

easyCZ Jun 28, 2022 •

edited

Loading

easyCZ commented Jun 28, 2022

easyCZ commented Jun 28, 2022

geropl commented Jun 28, 2022

geropl Jun 28, 2022

geropl Jun 28, 2022 •

edited

Loading

easyCZ Jun 28, 2022 •

edited

Loading

geropl Jun 28, 2022

geropl commented Jun 28, 2022

geropl Jun 28, 2022

geropl left a comment

[usage] Use attribution ID to reduce DB queries for usage report #10938

[usage] Use attribution ID to reduce DB queries for usage report #10938

Conversation

easyCZ commented Jun 27, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

jankeromnes Jun 28, 2022

Choose a reason for hiding this comment

easyCZ Jun 28, 2022

Choose a reason for hiding this comment

jankeromnes Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ commented Jun 28, 2022

easyCZ commented Jun 28, 2022

geropl commented Jun 28, 2022

geropl Jun 28, 2022

Choose a reason for hiding this comment

geropl Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

geropl Jun 28, 2022

Choose a reason for hiding this comment

geropl commented Jun 28, 2022

geropl Jun 28, 2022

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

easyCZ commented Jun 27, 2022 •

edited

Loading

jankeromnes Jun 28, 2022 •

edited

Loading

easyCZ Jun 28, 2022 •

edited

Loading

geropl Jun 28, 2022 •

edited

Loading

easyCZ Jun 28, 2022 •

edited

Loading