Skip to content

[usage] show sum of usage not balance #13222

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 23, 2022
Merged

[usage] show sum of usage not balance #13222

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
4 changes: 2 additions & 2 deletions components/dashboard/src/components/UsageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function UsageView({ attributionId }: UsageViewProps) {
try {
const page = await getGitpodService().server.listUsage(request);
setUsagePage(page);
setTotalCreditsUsed(page.creditBalanceAtEnd);
setTotalCreditsUsed(page.creditsUsed);
} catch (error) {
if (error.code === ErrorCodes.PERMISSION_DENIED) {
setErrorMessage("Access to usage details is restricted to team owners.");
Expand Down Expand Up @@ -173,7 +173,7 @@ function UsageView({ attributionId }: UsageViewProps) {
<div className="text-base text-gray-500">Total usage</div>
<div className="flex text-lg text-gray-600 font-semibold">
<CreditsSvg className="my-auto mr-1" />
<span>{totalCreditsUsed} Credits</span>
<span>{totalCreditsUsed.toLocaleString()} Credits</span>
</div>
</div>
<div className="flex flex-col truncate mt-8 text-sm">
Expand Down
3 changes: 1 addition & 2 deletions components/gitpod-protocol/src/usage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ export interface PaginationRequest {
export interface ListUsageResponse {
usageEntriesList: Usage[];
pagination?: PaginationResponse;
creditBalanceAtStart: number;
creditBalanceAtEnd: number;
creditsUsed: number;
}

export interface PaginationResponse {
Expand Down
3 changes: 1 addition & 2 deletions components/server/ee/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2355,8 +2355,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
totalPages: response.pagination.totalPages,
}
: undefined,
creditBalanceAtEnd: response.creditBalanceAtEnd,
creditBalanceAtStart: response.creditBalanceAtStart,
creditsUsed: response.creditsUsed,
};
}

Expand Down
217 changes: 102 additions & 115 deletions components/usage-api/go/v1/usage.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

29 changes: 9 additions & 20 deletions components/usage-api/typescript/src/usage/v1/usage.pb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,8 @@ export interface ListUsageResponse {
pagination:
| PaginatedResponse
| undefined;
/** the amount of credits the given account (attributionId) had at the beginning of the requested period */
creditBalanceAtStart: number;
/** the amount of credits the given account (attributionId) had at the end of the requested period */
creditBalanceAtEnd: number;
/** the amount of credits the given account (attributionId) has used during the requested period */
creditsUsed: number;
}

export interface Usage {
Expand Down Expand Up @@ -571,7 +569,7 @@ export const ListUsageRequest = {
};

function createBaseListUsageResponse(): ListUsageResponse {
return { usageEntries: [], pagination: undefined, creditBalanceAtStart: 0, creditBalanceAtEnd: 0 };
return { usageEntries: [], pagination: undefined, creditsUsed: 0 };
}

export const ListUsageResponse = {
Expand All @@ -582,11 +580,8 @@ export const ListUsageResponse = {
if (message.pagination !== undefined) {
PaginatedResponse.encode(message.pagination, writer.uint32(18).fork()).ldelim();
}
if (message.creditBalanceAtStart !== 0) {
writer.uint32(25).double(message.creditBalanceAtStart);
}
if (message.creditBalanceAtEnd !== 0) {
writer.uint32(33).double(message.creditBalanceAtEnd);
if (message.creditsUsed !== 0) {
writer.uint32(25).double(message.creditsUsed);
}
return writer;
},
Expand All @@ -605,10 +600,7 @@ export const ListUsageResponse = {
message.pagination = PaginatedResponse.decode(reader, reader.uint32());
break;
case 3:
message.creditBalanceAtStart = reader.double();
break;
case 4:
message.creditBalanceAtEnd = reader.double();
message.creditsUsed = reader.double();
break;
default:
reader.skipType(tag & 7);
Expand All @@ -622,8 +614,7 @@ export const ListUsageResponse = {
return {
usageEntries: Array.isArray(object?.usageEntries) ? object.usageEntries.map((e: any) => Usage.fromJSON(e)) : [],
pagination: isSet(object.pagination) ? PaginatedResponse.fromJSON(object.pagination) : undefined,
creditBalanceAtStart: isSet(object.creditBalanceAtStart) ? Number(object.creditBalanceAtStart) : 0,
creditBalanceAtEnd: isSet(object.creditBalanceAtEnd) ? Number(object.creditBalanceAtEnd) : 0,
creditsUsed: isSet(object.creditsUsed) ? Number(object.creditsUsed) : 0,
};
},

Expand All @@ -636,8 +627,7 @@ export const ListUsageResponse = {
}
message.pagination !== undefined &&
(obj.pagination = message.pagination ? PaginatedResponse.toJSON(message.pagination) : undefined);
message.creditBalanceAtStart !== undefined && (obj.creditBalanceAtStart = message.creditBalanceAtStart);
message.creditBalanceAtEnd !== undefined && (obj.creditBalanceAtEnd = message.creditBalanceAtEnd);
message.creditsUsed !== undefined && (obj.creditsUsed = message.creditsUsed);
return obj;
},

Expand All @@ -647,8 +637,7 @@ export const ListUsageResponse = {
message.pagination = (object.pagination !== undefined && object.pagination !== null)
? PaginatedResponse.fromPartial(object.pagination)
: undefined;
message.creditBalanceAtStart = object.creditBalanceAtStart ?? 0;
message.creditBalanceAtEnd = object.creditBalanceAtEnd ?? 0;
message.creditsUsed = object.creditsUsed ?? 0;
return message;
},
};
Expand Down
7 changes: 2 additions & 5 deletions components/usage-api/usage/v1/usage.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,8 @@ message ListUsageRequest {
message ListUsageResponse {
repeated Usage usage_entries = 1;
PaginatedResponse pagination = 2;
// the amount of credits the given account (attributionId) had at the beginning of the requested period
double credit_balance_at_start = 3;

// the amount of credits the given account (attributionId) had at the end of the requested period
double credit_balance_at_end = 4;
// the amount of credits the given account (attributionId) has used during the requested period
double credits_used = 3;
Copy link
Member

Choose a reason for hiding this comment

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

This is not backwards compatible. The rollout order between server and usage is undefined, and therefore you're not guaranteed it won't try to pass this value.
Please mark credit_balance_at_start as either reserved or [deprecated=true] and add a new field with ID 5.

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'm aware this is not compatible.
I expect the friction this would cause minimal (very short period between the two component deploys, where errors would be thrown for a feature that is not used by users, yet) compared to the extra turnaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you expect any serious issues here or is this more a matter of principles?

Copy link
Member

Choose a reason for hiding this comment

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

Requests could actually fail to serialize on the usage component and return Unknown error. This is because each field is looked up by their index field. Here, it's kind of lucky that the field is the same type, but the removal of a field (4) would cause issues for the serializer as it expects there to be a field with index 4 (even if it's empty).

Even if we're happy with a temporary issue, we as a team (and organization) need to do better here in order to improve our reliability. It's a matter of getting into the habit of thinking (and dealing) with these else we're gonna struggle to uphold our standards as the systems grow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I agree with the principle behind it. Not sure going through unnecessary ceremony is helpful. I think acknowledging this explicitly, which is what we are doing here, is great. IMHO it is actually even better than blindly following a certain protocol, because we are talking about the real impact. FWIW just making it technically compatible wouldn't help users, because on the client side there would be a different result unless I roll out a full compatibility mode, which I would be happy to do if it was worth it.

}

message Usage {
Expand Down
21 changes: 11 additions & 10 deletions components/usage/pkg/apiv1/usage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,30 +129,31 @@ func (s *UsageService) ListUsage(ctx context.Context, in *v1.ListUsageRequest) (
}

usageSummary, err := db.GetUsageSummary(ctx, s.conn,
attributionId,
from,
to,
excludeDrafts,
db.GetUsageSummaryParams{
AttributionId: attributionId,
From: from,
To: to,
ExcludeDrafts: excludeDrafts,
},
)

if err != nil {
logger.WithError(err).Error("Failed to fetch usage metadata.")
return nil, status.Error(codes.Internal, "unable to retrieve usage")
}
totalPages := int64(math.Ceil(float64(usageSummary.NumRecordsInRange) / float64(perPage)))
totalPages := int64(math.Ceil(float64(usageSummary.NumberOfRecords) / float64(perPage)))

pagination := v1.PaginatedResponse{
PerPage: perPage,
Page: page,
TotalPages: totalPages,
Total: int64(usageSummary.NumRecordsInRange),
Total: int64(usageSummary.NumberOfRecords),
}

return &v1.ListUsageResponse{
UsageEntries: usageData,
CreditBalanceAtStart: usageSummary.CreditCentsBalanceAtStart.ToCredits(),
CreditBalanceAtEnd: usageSummary.CreditCentsBalanceAtEnd.ToCredits(),
Pagination: &pagination,
UsageEntries: usageData,
CreditsUsed: usageSummary.CreditCentsUsed.ToCredits(),
Pagination: &pagination,
}, nil
}

Expand Down
16 changes: 7 additions & 9 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 @@ -313,15 +313,14 @@ func TestListUsage(t *testing.T) {
tests := []struct {
start, end time.Time
// expectations
creditsAtStart float64
creditsAtEnd float64
creditsUsed float64
recordsInRange int64
}{
{start, end, 3, 10, 2},
{end, end, 10, 10, 0},
{start, start, 3, 3, 0},
{start.Add(-200 * 24 * time.Hour), end, 0, 10, 4},
{start.Add(-200 * 24 * time.Hour), end.Add(10 * 24 * time.Hour), 0, 20, 5},
{start, end, 7, 2},
{end, end, 0, 0},
{start, start, 0, 0},
{start.Add(-200 * 24 * time.Hour), end, 10, 4},
{start.Add(-200 * 24 * time.Hour), end.Add(10 * 24 * time.Hour), 20, 5},
}

for i, test := range tests {
Expand All @@ -338,8 +337,7 @@ func TestListUsage(t *testing.T) {
})
require.NoError(t, err)

require.Equal(t, test.creditsAtStart, metaData.CreditBalanceAtStart)
require.Equal(t, test.creditsAtEnd, metaData.CreditBalanceAtEnd)
require.Equal(t, test.creditsUsed, metaData.CreditsUsed)
require.Equal(t, test.recordsInRange, metaData.Pagination.Total)
})
}
Expand Down
6 changes: 3 additions & 3 deletions components/usage/pkg/db/cost_center.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,19 @@ func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, costCenter Cos

func (c *CostCenterManager) ComputeInvoiceUsageRecord(ctx context.Context, attributionID AttributionID) (*Usage, error) {
now := time.Now()
summary, err := GetUsageSummary(ctx, c.conn, attributionID, now, now, false)
creditCents, err := GetBalance(ctx, c.conn, attributionID)
if err != nil {
return nil, err
}
if summary.CreditCentsBalanceAtEnd.ToCredits() <= 0 {
if creditCents.ToCredits() <= 0 {
// account has no debt, do nothing
return nil, nil
}
Comment on lines +165 to 168
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, we could also change the logic to do creditCents <= 0 because the Credits conversion only multiplies by a 100, which won't change the polarity

return &Usage{
ID: uuid.New(),
AttributionID: attributionID,
Description: "Credits",
CreditCents: summary.CreditCentsBalanceAtEnd * -1,
CreditCents: creditCents * -1,
EffectiveTime: NewVarcharTime(now),
Kind: InvoiceUsageKind,
Draft: false,
Expand Down
55 changes: 22 additions & 33 deletions components/usage/pkg/db/usage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package db

import (
"context"
"database/sql"
"encoding/json"
"fmt"
"math"
Expand Down Expand Up @@ -139,7 +138,8 @@ func FindUsage(ctx context.Context, conn *gorm.DB, params *FindUsageParams) ([]U

db := conn.WithContext(ctx).
Where("attributionId = ?", params.AttributionId).
Where("effectiveTime >= ? AND effectiveTime < ?", TimeToISO8601(params.From), TimeToISO8601(params.To))
Where("effectiveTime >= ? AND effectiveTime < ?", TimeToISO8601(params.From), TimeToISO8601(params.To)).
Where("kind = ?", WorkspaceInstanceUsageKind)
if params.ExcludeDrafts {
db = db.Where("draft = ?", false)
}
Expand All @@ -158,48 +158,37 @@ func FindUsage(ctx context.Context, conn *gorm.DB, params *FindUsageParams) ([]U
if result.Error != nil {
return nil, fmt.Errorf("failed to get usage records: %s", result.Error)
}

return usageRecords, nil
}

type UsageSummary struct {
NumRecordsInRange int
CreditCentsBalanceAtStart CreditCents
CreditCentsBalanceAtEnd CreditCents
type GetUsageSummaryParams struct {
AttributionId AttributionID
From, To time.Time
ExcludeDrafts bool
}

type GetUsageSummaryResponse struct {
CreditCentsUsed CreditCents
NumberOfRecords int
}

func GetUsageSummary(ctx context.Context, conn *gorm.DB, attributionId AttributionID, from, to time.Time, excludeDrafts bool) (*UsageSummary, error) {
func GetUsageSummary(ctx context.Context, conn *gorm.DB, params GetUsageSummaryParams) (GetUsageSummaryResponse, error) {
db := conn.WithContext(ctx)
query1 := db.Table((&Usage{}).TableName()).
Select("sum(creditCents) as creditCentsBalanceAtStart").
Where("attributionId = ?", attributionId).
Where("effectiveTime < ?", TimeToISO8601(from))
if excludeDrafts {
Select("sum(creditCents) as CreditCentsUsed, count(*) as NumberOfRecords").
Where("attributionId = ?", params.AttributionId).
Where("effectiveTime >= ? AND effectiveTime < ?", TimeToISO8601(params.From), TimeToISO8601(params.To)).
Where("kind = ?", WorkspaceInstanceUsageKind)
if params.ExcludeDrafts {
query1 = query1.Where("draft = ?", false)
}
var creditCentsBalanceAtStart sql.NullInt64
err := query1.Row().Scan(&creditCentsBalanceAtStart)
if err != nil {
return nil, fmt.Errorf("failed to get usage meta data: %s", err)
}

query2 := db.Table((&Usage{}).TableName()).
Select("sum(creditCents) as creditCentsBalanceInPeriod", "count(id) as numRecordsInRange").
Where("attributionId = ?", attributionId).
Where("? <= effectiveTime AND effectiveTime < ?", TimeToISO8601(from), TimeToISO8601(to))
if excludeDrafts {
query2 = query2.Where("draft = ?", false)
}
var creditCentsBalanceInPeriod sql.NullInt64
var numRecordsInRange sql.NullInt32
err = query2.Row().Scan(&creditCentsBalanceInPeriod, &numRecordsInRange)
var result GetUsageSummaryResponse
err := query1.Find(&result).Error
if err != nil {
return nil, fmt.Errorf("failed to get usage meta data: %s", err)
return result, fmt.Errorf("failed to get usage meta data: %w", err)
}
return &UsageSummary{
NumRecordsInRange: int(numRecordsInRange.Int32),
CreditCentsBalanceAtStart: CreditCents(creditCentsBalanceAtStart.Int64),
CreditCentsBalanceAtEnd: CreditCents(creditCentsBalanceAtStart.Int64 + creditCentsBalanceInPeriod.Int64),
}, nil
return result, nil
}

type Balance struct {
Expand Down
46 changes: 31 additions & 15 deletions components/usage/pkg/db/usage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,33 +93,44 @@ func TestGetUsageSummary(t *testing.T) {
CreditCents: 1000,
})

dbtest.CreateUsageRecords(t, conn, draftBefore, nondraftBefore, draftInside, nonDraftInside, nonDraftAfter)
invoice := dbtest.NewUsage(t, db.Usage{
AttributionID: attributionID,
Kind: db.InvoiceUsageKind,
EffectiveTime: db.NewVarcharTime(start.Add(2 * time.Hour)),
CreditCents: -400,
Draft: false,
})

dbtest.CreateUsageRecords(t, conn, draftBefore, nondraftBefore, draftInside, nonDraftInside, nonDraftAfter, invoice)

tests := []struct {
start, end time.Time
excludeDrafts bool
// expectations
creditsAtStart int64
creditsAtEnd int64
recordsInRange int
creditCents db.CreditCents
numberOfRecords int
}{
{start, end, false, 3, 10, 2},
{start, end, true, 2, 6, 1},
{end, end, false, 10, 10, 0},
{end, end, true, 6, 6, 0},
{start, start, false, 3, 3, 0},
{start.Add(-500 * 24 * time.Hour), end, false, 0, 10, 4},
{start.Add(-500 * 24 * time.Hour), end.Add(500 * 24 * time.Hour), false, 0, 20, 5},
{start, end, false, 700, 2},
{start, end, true, 400, 1},
{end, end, false, 0, 0},
{end, end, true, 0, 0},
{start, start, false, 0, 0},
{start.Add(-500 * 24 * time.Hour), end, false, 1000, 4},
{start.Add(-500 * 24 * time.Hour), end.Add(500 * 24 * time.Hour), false, 2000, 5},
}

for i, test := range tests {
t.Run(fmt.Sprintf("Running test no %d", i+1), func(t *testing.T) {
metaData, err := db.GetUsageSummary(context.Background(), conn, attributionID, test.start, test.end, test.excludeDrafts)
usageSummary, err := db.GetUsageSummary(context.Background(), conn, db.GetUsageSummaryParams{
AttributionId: attributionID,
From: test.start,
To: test.end,
ExcludeDrafts: test.excludeDrafts,
})
require.NoError(t, err)

require.EqualValues(t, test.creditsAtStart, metaData.CreditCentsBalanceAtStart.ToCredits())
require.EqualValues(t, test.creditsAtEnd, metaData.CreditCentsBalanceAtEnd.ToCredits())
require.Equal(t, test.recordsInRange, metaData.NumRecordsInRange)
require.EqualValues(t, test.creditCents, usageSummary.CreditCentsUsed)
require.EqualValues(t, test.numberOfRecords, usageSummary.NumberOfRecords)
})
}
}
Expand Down Expand Up @@ -326,6 +337,7 @@ func TestListBalance(t *testing.T) {
func TestGetBalance(t *testing.T) {
teamAttributionID := db.NewTeamAttributionID(uuid.New().String())
userAttributionID := db.NewUserAttributionID(uuid.New().String())
noUsageAttributionID := db.NewUserAttributionID(uuid.New().String())

conn := dbtest.ConnectForTests(t)
dbtest.CreateUsageRecords(t, conn,
Expand Down Expand Up @@ -355,4 +367,8 @@ func TestGetBalance(t *testing.T) {
userBalance, err := db.GetBalance(context.Background(), conn, userAttributionID)
require.NoError(t, err)
require.EqualValues(t, -50, int(userBalance))

noUsageBalance, err := db.GetBalance(context.Background(), conn, noUsageAttributionID)
require.NoError(t, err)
require.EqualValues(t, 0, int(noUsageBalance))
}