Added API call to fetch usage data #12646

svenefftinge · 2022-09-05T10:32:57Z

Description

Added API call to fetch usage data

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

/werft with-preview

easyCZ · 2022-09-05T11:18:31Z

components/usage-api/usage/v1/usage.proto

@@ -10,6 +10,9 @@ service UsageService {
    // ListBilledUsage retrieves all usage for the specified attributionId
    rpc ListBilledUsage(ListBilledUsageRequest) returns (ListBilledUsageResponse) {}

+    // ListBilledUsage retrieves all usage for the specified attributionId
+    rpc FindUsage(FindUsageRequest) returns (FindUsageResponse) {}


If we followed CRUD more closely then the RPC would generally be Get/List/Create/Delete/Update but I'm not too fussed here. It mostly stands out given the RPC above uses List

easyCZ · 2022-09-05T11:22:06Z

components/usage-api/usage/v1/usage.proto

+
+message FindUsageResponse {
+    repeated BilledSession sessions = 1;
+    double total_credits_used = 2;


If I understand this correctly then this would return the total credits for the period in the request, correct? For scenarios like this, the proto is a good place to put comments with this as they are included in all of the code-generated clients so docs can be right there with the SDK

easyCZ · 2022-09-05T13:40:11Z

components/usage/pkg/apiv1/usage.go

+		log.Log.
+			WithField("attribution_id", in.AttributionId).
+			WithField("perPage", limit).
+			WithField("page", page).
+			WithField("from", from).
+			WithField("to", to).
+			WithError(err).Error("Failed to fetch usage.")


The fields are quite useful even for errors later on. Let's lift this higher like this:

Suggested change

log.Log.

WithField("attribution_id", in.AttributionId).

WithField("perPage", limit).

WithField("page", page).

WithField("from", from).

WithField("to", to).

WithError(err).Error("Failed to fetch usage.")

logger := log.Log.

WithField("attribution_id", in.AttributionId).

WithField("perPage", limit).

WithField("page", page).

WithField("from", from).

WithField("to", to)

if err != nil {

logger.WithError(err).Error("Failed to fetch usage.")

return ...

}

This then allows you to re-use the logger fields in other error messages across this method

easyCZ · 2022-09-05T13:41:16Z

components/usage/pkg/db/usage.go

 	if result.Error != nil {
 		return nil, fmt.Errorf("failed to get usage records: %s", result.Error)
 	}
 	return usageRecords, nil
 }
+
+type UsageMetadata struct {


Suggested change

type UsageMetadata struct {

type UsageSummary struct {

The Metadata keyword gets confusign with the Metadata property on the Usage entry

easyCZ · 2022-09-05T14:33:52Z

components/usage/pkg/apiv1/usage.go

+		return nil, status.Errorf(codes.InvalidArgument, "Maximum range exceeded. Range specified can be at most %s", maxQuerySize.String())
+	}
+
+	var order db.Order = db.DescendingOrder


I think the type here is rendundant and could be removed

Suggested change

var order db.Order = db.DescendingOrder

order := db.DescendingOrder

easyCZ · 2022-09-05T14:35:37Z

components/usage/pkg/apiv1/usage.go

+	}
+
+	usageSummary, err := db.GetUsageSummary(ctx, s.conn,
+		db.AttributionID(in.GetAttributionId()),


I think we should first attempt to parse the attribution using db.ParseAttributionID() which does a sanity check that the ID is valid.

easyCZ · 2022-09-05T14:36:45Z

components/usage/pkg/apiv1/usage.go

+		logger.WithError(err).Error("Failed to fetch usage metadata.")
+		return nil, status.Error(codes.Internal, "unable to retrieve usage")
+	}
+	var totalPages = int64(math.Ceil(float64(usageSummary.NumRecordsInRange) / float64(limit)))


This will panic if limit is 0. We don't validate the pagination input from the request.

easyCZ · 2022-09-05T14:38:05Z

components/usage/pkg/apiv1/usage.go

+	var offset int64 = 0
+	if in.Pagination != nil {
+		limit = in.Pagination.PerPage
+		page = in.Pagination.Page


Here we should also validate the numbers are positive integers

easyCZ · 2022-09-05T14:39:07Z

components/usage/pkg/db/usage.go

+func FindUsage(ctx context.Context, conn *gorm.DB, params *FindUsageParams) ([]Usage, error) {
 	var usageRecords []Usage
 	var usageRecordsBatch []Usage

-	result := conn.WithContext(ctx).
-		Where("attributionId = ?", attributionId).
-		Where("? <= effectiveTime AND effectiveTime < ?", from.String(), to.String()).
-		Order("effectiveTime DESC").
-		Offset(int(offset)).
-		Limit(int(limit)).
-		FindInBatches(&usageRecordsBatch, 1000, func(_ *gorm.DB, _ int) error {
-			usageRecords = append(usageRecords, usageRecordsBatch...)
-			return nil
-		})
+	db := conn.WithContext(ctx).
+		Where("attributionId = ?", params.AttributionId).
+		Where("? <= effectiveTime AND effectiveTime < ?", params.From, params.To)
+	if params.ExcludeDrafts {
+		db = db.Where("draft = ?", false)
+	}
+	db = db.Order(fmt.Sprintf("effectiveTime %s", params.Order.ToSQL()))
+	if params.Offset != 0 {
+		db = db.Offset(int(params.Offset))
+	}
+	if params.Limit != 0 {
+		db = db.Limit(int(params.Limit))
+	}
+
+	result := db.FindInBatches(&usageRecordsBatch, 1000, func(_ *gorm.DB, _ int) error {
+		usageRecords = append(usageRecords, usageRecordsBatch...)
+		return nil
+	})
 	if result.Error != nil {
 		return nil, fmt.Errorf("failed to get usage records: %s", result.Error)
 	}
 	return usageRecords, nil
 }
+


For implementing these, you may find gorm scopes very applicable

Not quite sure how you would like them to be used in this context.

easyCZ · 2022-09-05T14:41:36Z

components/usage/pkg/db/usage.go

+	}
+	var creditCentsBalanceInPeriod sql.NullInt64
+	var numRecordsInRange sql.NullInt32
+	err = query2.Row().Scan(&creditCentsBalanceInPeriod, &numRecordsInRange)


I believe we shouldn't need to use the Row().Scan() API here, and instead should be able to do this with https://gorm.io/docs/advanced_query.html#Count

Happy to try and pair on this, I haven't used it myself yet.

But it only gives me the count not the sum, does it?

The intention was to use deserialize into the UsageSummary object directly. If you were to add a gorm annotation to the UsageSummary, you can have those annotations match the returned column names (creditCentsBalanceAtStart, ...)

easyCZ · 2022-09-06T06:25:35Z

components/usage/pkg/apiv1/usage.go

+	var limit int64 = 1000
+	var page int64 = 0
+	var offset int64 = 0
+	if in.Pagination != nil {


Grpc guarantees it won't be nil. If its not specified in the request, it will contain default values. For ints that's 0

easyCZ · 2022-09-06T06:36:54Z

components/usage/pkg/apiv1/usage.go

+		attributionId, err := db.ParseAttributionID(string(usageRecord.AttributionID))
+		if err != nil {
+			logger.WithError(err).Errorf("Ignoring usage entry (ID: %s) with invalid attributionID: %s.", usageRecord.ID, usageRecord.AttributionID)
+			continue
+		}


Persoanlly, I'd return an error here from the RPC. Yes, it would cause it to fail but it would more acutely show up and give us a chance to fix. There's a tendency that these logs are missed and never properly fixed.

I think I may have confused you in the previous comment on this. I think when we take the attribution ID from the DB, we can trust it to be the right shape so doing just db.AttributionID(usageRecord.AttributionID) should be fine. But I think we can do better. We can actually type the UsageRecord as an AttributionID directly, this removes this step.

But we should try to parse the Attribution ID from the incoming request. We currently use it on line 208 without parsing (and therefore at least some validation).

Yes, I got indeed confused (somehow the comments show up on the wrong lines in VS Code sometimes). Makes much more sense now.

easyCZ · 2022-09-06T07:00:31Z

components/usage/pkg/apiv1/usage.go

+	var offset int64 = perPage * page
+
+	listUsageResult, err := db.FindUsage(ctx, s.conn, &db.FindUsageParams{
+		AttributionId: db.AttributionID(in.GetAttributionId()),


Suggested change

AttributionId: db.AttributionID(in.GetAttributionId()),

AttributionId: attributionId,

Also, in Go it's a common style to capitalize shorthands. In this case, ID instead of Id

easyCZ

LGTM, left a couple of last comments but these can happen in a follow-up PR

easyCZ · 2022-09-06T07:03:03Z

components/usage/pkg/db/usage.go

+	}
+	var creditCentsBalanceInPeriod sql.NullInt64
+	var numRecordsInRange sql.NullInt32
+	err = query2.Row().Scan(&creditCentsBalanceInPeriod, &numRecordsInRange)


The intention was to use deserialize into the UsageSummary object directly. If you were to add a gorm annotation to the UsageSummary, you can have those annotations match the returned column names (creditCentsBalanceAtStart, ...)

svenefftinge requested a review from a team September 5, 2022 10:32

roboquat added release-note-none size/XXL labels Sep 5, 2022

github-actions bot added team: webapp Issue belongs to the WebApp team and removed size/XXL labels Sep 5, 2022

svenefftinge marked this pull request as draft September 5, 2022 10:33

roboquat added the do-not-merge/work-in-progress label Sep 5, 2022

easyCZ reviewed Sep 5, 2022

View reviewed changes

svenefftinge force-pushed the se/ledger-usage-view branch from fea3159 to 361e917 Compare September 5, 2022 12:57

roboquat added the size/XXL label Sep 5, 2022

svenefftinge marked this pull request as ready for review September 5, 2022 12:59

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

easyCZ reviewed Sep 5, 2022

View reviewed changes

svenefftinge force-pushed the se/ledger-usage-view branch from 361e917 to 1fa102e Compare September 5, 2022 14:27

easyCZ reviewed Sep 5, 2022

View reviewed changes

svenefftinge force-pushed the se/ledger-usage-view branch 4 times, most recently from 129ea87 to 99494b5 Compare September 6, 2022 06:21

easyCZ reviewed Sep 6, 2022

View reviewed changes

svenefftinge force-pushed the se/ledger-usage-view branch from 99494b5 to 530ed81 Compare September 6, 2022 06:39

Added API call to fetch usage data

aff20f9

svenefftinge force-pushed the se/ledger-usage-view branch from 530ed81 to aff20f9 Compare September 6, 2022 06:48

easyCZ reviewed Sep 6, 2022

View reviewed changes

easyCZ approved these changes Sep 6, 2022

View reviewed changes

roboquat merged commit 29388c9 into main Sep 6, 2022

roboquat deleted the se/ledger-usage-view branch September 6, 2022 07:04

roboquat added the deployed: webapp Meta team change is running in production label Sep 6, 2022

roboquat added the deployed Change is completely running in production label Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added API call to fetch usage data #12646

Added API call to fetch usage data #12646

svenefftinge commented Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022 •

edited

Loading

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

easyCZ Sep 5, 2022

svenefftinge Sep 5, 2022

easyCZ Sep 5, 2022

svenefftinge Sep 5, 2022

easyCZ Sep 6, 2022

easyCZ Sep 6, 2022

easyCZ Sep 6, 2022

svenefftinge Sep 6, 2022

easyCZ Sep 6, 2022

easyCZ Sep 6, 2022

easyCZ left a comment

easyCZ Sep 6, 2022

	var order db.Order = db.DescendingOrder
	order := db.DescendingOrder

	AttributionId: db.AttributionID(in.GetAttributionId()),
	AttributionId: attributionId,

Added API call to fetch usage data #12646

Added API call to fetch usage data #12646

Conversation

svenefftinge commented Sep 5, 2022

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

easyCZ Sep 5, 2022 • edited Loading

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

Choose a reason for hiding this comment

easyCZ Sep 5, 2022 •

edited

Loading