Skip to content

[usage] Store credits as integer values in the usage table #11429

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

Closed
wants to merge 4 commits into from

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Jul 18, 2022

Description

Workspace credits per minute can be configured as fractional values, but we always want to bill and display usage in whole numbers of credits. This PR changes the type of the field that stores credits consumed per workspace instance to be an integer.

Related Issue(s)

Part of #9036 and this comment on #11406.

How to test

Connect to the database for the preview and describe the d_b_workspace_instance_usage table:

image

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@andrew-farries andrew-farries requested a review from a team July 18, 2022 06:22
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jul 18, 2022
@werft-gitpod-dev-com
Copy link

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

@roboquat roboquat added size/M and removed size/S labels Jul 18, 2022
@andrew-farries andrew-farries force-pushed the af/integer-credit-values-in-usage-store branch from 42c1489 to 4be8d13 Compare July 18, 2022 06:25
@geropl
Copy link
Member

geropl commented Jul 18, 2022

int(11) sounds potentially smaller than int64 which we use in code. Maybe a different length or bigint is more suitable? 🤔

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Jul 18, 2022

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

👍 started the job as gitpod-build-af-integer-credit-values-in-usage-store.6
(with .werft/ from main)

@andrew-farries
Copy link
Contributor Author

Changed to bigint in 3c12d8c.

I think this is unnecessarily large though, but it's now consistent with the type used elsewhere.

@jldec
Copy link
Contributor

jldec commented Jul 18, 2022

My understanding was that we have to report total usage to Stripe as an int, but we should collect usage per session in fractional units to avoid rounding errors.

@easyCZ
Copy link
Member

easyCZ commented Jul 18, 2022

Indeed, the stored value should remain a float. This is because only the total billed value should be rounded, not each workspace instance.

@easyCZ
Copy link
Member

easyCZ commented Jul 18, 2022

Regular workspace costs 1 credit per 6 minutes, there's no way we'd be able to express workspace that ran for 5 minutes, or other fractions in this way.

@andrew-farries andrew-farries deleted the af/integer-credit-values-in-usage-store branch July 18, 2022 11:36
@andrew-farries
Copy link
Contributor Author

Indeed, the stored value should remain a float. This is because only the total billed value should be rounded, not each workspace instance.

In that case, I believe we are incorrectly rounding in the billing data sent to Stripe:

var credits int64
for _, instance := range instances {
credits += pricer.CreditsUsedByInstance(&instance, maxStopTime)
}

Here we round each workspace instance's credits, rather than rounding the total once at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants