feat(customer): csv export #2828
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for filtering customers by multiple subjects instead of a single subject throughout the customer listing flow. This involves updating the input struct, validation, query construction, and HTTP handling logic. Additionally, the meter CSV query endpoint is reintroduced with enhancements to enrich results with customer information, requiring new dependencies, utility functions for customer lookup and pagination, and modifications to handler construction and routing. Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openmeter/meter/httphandler/util.go (1)
14-37: Shadowed parameter and pagination tuning
params := paramsinside the loop re-assigns to a new variable that shadows the function parameter.
Although harmless, it is confusing and blocks accidental future writes back to the outer variable.
At the same time,limitis hard-coded to100, and we never look atresult.TotalCount, so we cannot pre-allocate the slice, and callers cannot influence page size.- params := params - params.Page = pagination.NewPage(page, limit) + local := params // copy to avoid mutating caller-supplied struct + local.Page = pagination.NewPage(page, limit)Consider:
- Renaming
localto something explicit (e.g.pageParams).- Accepting
pageSizeas a function argument or a constant at the package level so it can be tuned centrally.- Pre-allocating
customerswhenresult.TotalCountis available to reduce reallocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openmeter/customer/adapter/customer.go(1 hunks)openmeter/customer/customer.go(2 hunks)openmeter/customer/httpdriver/customer.go(1 hunks)openmeter/meter/httphandler/handler.go(3 hunks)openmeter/meter/httphandler/query.go(9 hunks)openmeter/meter/httphandler/util.go(1 hunks)openmeter/server/router/router.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
openmeter/server/router/router.go (2)
openmeter/customer/customer.go (1)
Customer(15-23)app/common/customer.go (1)
Customer(18-20)
openmeter/customer/httpdriver/customer.go (3)
api/api.gen.go (1)
Subject(6343-6360)api/client/go/client.gen.go (1)
Subject(5807-5824)api/client/javascript/src/client/subjects.ts (1)
Subjects(10-89)
openmeter/customer/customer.go (2)
api/client/javascript/src/client/subjects.ts (1)
Subjects(10-89)pkg/models/error.go (1)
NewGenericValidationError(138-140)
openmeter/meter/httphandler/util.go (1)
openmeter/customer/customer.go (2)
ListCustomersInput(139-156)Customer(15-23)
openmeter/meter/httphandler/query.go (4)
api/api.gen.go (3)
Subject(6343-6360)Customer(2129-2176)MeterQueryRow(4748-4764)openmeter/customer/customer.go (2)
Customer(15-23)ListCustomersInput(139-156)openmeter/meter/httphandler/util.go (1)
ListAllCustomers(13-37)openmeter/meter/meter.go (1)
MeterQueryRow(333-339)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Commit hooks
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
openmeter/server/router/router.go (1)
266-272: Approved: Clean dependency injectionThe customer service is now properly passed to the meter handler constructor, enabling the new CSV export functionality with customer data enrichment.
openmeter/customer/httpdriver/customer.go (1)
70-73: Approved: Good backward compatibilityThe code elegantly preserves API compatibility by converting a single subject parameter to the new multi-subject structure, supporting the new CSV export feature without breaking existing integrations.
openmeter/meter/httphandler/handler.go (3)
8-8: Approved: Necessary importAdding the customer package import to support the new dependency.
39-39: Approved: New dependency fieldAdding the customer service field to the handler struct to enable customer data enrichment in CSV exports.
51-64: Approved: Constructor updateThe constructor function is properly updated to accept and store the customer service dependency, maintaining the handler's initialization pattern.
openmeter/customer/customer.go (2)
153-153: Approved: Enhanced filtering capabilityChanging from a single subject to multiple subjects allows more flexible customer filtering, which directly supports the CSV export feature.
163-165: Approved: Proper validationThe added validation ensures that if a subjects filter is provided, it contains at least one value, preventing meaningless empty filter queries.
openmeter/customer/adapter/customer.go (1)
69-77: Predicate may unintentionally match partially
SubjectKeyContainsFold(subject)performs a contains match (ILIKE %subject%).
If callers expect exact matching (previous API accepted a single exact subject), this widens the query surface and may return the wrong customers, especially with short tokens (e.g."abc"will match"xabcx").If exact matching is desired, switch to
SubjectKeyEQ(subject)(orSubjectKey(subject)in ent):- return customerdb.HasSubjectsWith(customersubjectsdb.SubjectKeyContainsFold(subject)) + return customerdb.HasSubjectsWith(customersubjectsdb.SubjectKeyEQ(subject))Otherwise, document the new fuzzy-matching behaviour in the API.
openmeter/meter/httphandler/query.go (1)
293-297: Nil-safe email extraction can drop compile iflo.FromPtrOris unavailable
lo.FromPtrOrexists only in ≥ v1.38. Ensure the project is pinned to a recentsamber/loor replace with a trivial helper:email := "" if row.Customer.PrimaryEmail != nil { email = *row.Customer.PrimaryEmail } data = append(data, row.Customer.ID, row.Customer.Name, email)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
openmeter/meter/httphandler/util.go (3)
32-36: Duplicate‐key overwrites are silent – consider detecting / logging collisionsWhen multiple customers share the same
SubjectKey, the later customer will overwrite the earlier one incustomersBySubjectKey.
Depending on your data-model this may be undesirable or at least something you want to be aware of.for i, c := range customers { for _, key := range c.UsageAttribution.SubjectKeys { if _, exists := customersBySubjectKey[key]; exists { // log/debug or return an error? } customersBySubjectKey[key] = &customers[i] } }Even a simple debug-level log would make troubleshooting easier if the mapping ever behaves unexpectedly.
43-67: Shadowing theparamsidentifier makes the code harder to readInside the loop you re-declare
params := params. While legal, the shadowing is easy to stumble over when skimming the code.- for { - params := params // shadowed + for { + pageParams := params // explicit copy pageParams.Page = pagination.NewPage(page, limit)A distinct variable name clarifies intent and avoids confusion about which value is being mutated.
45-64: Expose page size and back-off strategy for large customer basesThe hard-coded
limit := 100combined with a tight loop means listing 10 000 customers will
perform 100 synchronous requests without any pacing. Consider:
- Making the limit configurable (env or constant).
- Adding a small back-off / context check to respect cancellation.
- Exploring a streaming or cursor-based API on the customer service to avoid holding all rows in memory.
These tweaks improve scalability without changing semantics.
openmeter/meter/httphandler/query_csv.go (3)
152-157: Avoid%fdefault formatting for metric values
fmt.Sprintf("%f", row.Value)always prints six decimals (e.g.12.340000) which bloats the CSV
and may mislead users into thinking the precision is fixed.
Consider:- data = append(data, fmt.Sprintf("%f", row.Value)) + data = append(data, strconv.FormatFloat(row.Value, 'g', -1, 64))
'g'chooses the shortest representation that round-trips, keeping files smaller and neater.
65-74: Potentially expensive customer lookup not paginated by caller
getSubjectsFromQueryResultcan return hundreds/thousands of subjects.
listCustomersBySubjectKeythen fetches all matching customers in one go.If the customer service is remote this may introduce latency spikes.
You might:
- Short-circuit when
subjectsis empty (already done 👍).- Batch the
Subjectsslice into pages matchingCustomerService’s pagination instead of the fixed100used inlistAllCustomers.- Cache subjects seen in prior requests.
These optimisations are optional but will help once the feature is used on large datasets.
196-210: Minor optimisation: pre-allocate and deduplicate in one passCurrently you (1) collect all subjects, (2) call
lo.Uniqfor deduplication.
You can avoid the extra slice and allocation by using amap[string]struct{}.Not critical at this size, but worth considering for hot paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openmeter/meter/httphandler/query.go(0 hunks)openmeter/meter/httphandler/query_csv.go(1 hunks)openmeter/meter/httphandler/util.go(1 hunks)
💤 Files with no reviewable changes (1)
- openmeter/meter/httphandler/query.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
openmeter/meter/httphandler/util.go (2)
openmeter/customer/customer.go (2)
Customer(15-23)ListCustomersInput(139-156)api/client/javascript/src/client/subjects.ts (1)
Subjects(10-89)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Artifacts / Container image
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
openmeter/meter/httphandler/query_csv.go (6)
157-157: Consider more control over numeric formattingThe current implementation uses
fmt.Sprintf("%f", row.Value)which doesn't provide control over decimal precision or handling of very large/small numbers.- data = append(data, fmt.Sprintf("%f", row.Value)) + data = append(data, fmt.Sprintf("%.6f", row.Value))Alternatively, consider using a more specialized formatting function that handles different numeric ranges appropriately.
143-145: Inconsistent handling of Name vs PrimaryEmailThe code directly uses
row.Customer.Namebut useslo.FromPtrOr(row.Customer.PrimaryEmail, "")for email. This suggests thatNameis a string whilePrimaryEmailis a pointer to a string. For consistency, consider handling both fields similarly.- data = append(data, row.Customer.Name, lo.FromPtrOr(row.Customer.PrimaryEmail, "")) + data = append(data, lo.FromPtrOr(&row.Customer.Name, ""), lo.FromPtrOr(row.Customer.PrimaryEmail, ""))Or if
Nameis guaranteed to be non-nil:- data = append(data, row.Customer.Name, lo.FromPtrOr(row.Customer.PrimaryEmail, "")) + // Name is a string, PrimaryEmail is a *string + data = append(data, row.Customer.Name, lo.FromPtrOr(row.Customer.PrimaryEmail, ""))
65-77: Consider handling the case of no matching customersWhile there's good error handling for when
listCustomersBySubjectKeyreturns an error, there's no specific handling for when it successfully returns an empty map. Consider adding a log message or metric to track this scenario to help diagnose potential customer data issues.customersBySubjectKey, err := listCustomersBySubjectKey( ctx, h.customerService, request.namespace, subjects, ) if err != nil { return nil, fmt.Errorf("failed to get customers by subject key: %w", err) } + + // Log when no customers found for any subjects + if len(subjects) > 0 && len(customersBySubjectKey) == 0 { + // Consider adding logging or metrics here + // log.Info("No customers found for any subjects", "subjectCount", len(subjects)) + }
126-132: Consider using constants for column headersThe CSV header strings are hardcoded. Consider defining these as constants or in a configuration to make them easier to maintain and reference elsewhere in the code.
+ // Define header constants + const ( + HeaderWindowStart = "window_start" + HeaderWindowEnd = "window_end" + HeaderSubject = "subject" + HeaderCustomerName = "customer_name" + HeaderCustomerEmail = "customer_email" + HeaderValue = "value" + ) // CSV headers - headers := []string{"window_start", "window_end", "subject", "customer_name", "customer_email"} + headers := []string{HeaderWindowStart, HeaderWindowEnd, HeaderSubject, HeaderCustomerName, HeaderCustomerEmail} if len(groupByKeys) > 0 { headers = append(headers, groupByKeys...) } - headers = append(headers, "value") + headers = append(headers, HeaderValue)
196-212: Efficient subject extraction, but consider early returnThe subject extraction logic efficiently deduplicates the subjects using
lo.Uniq. For slight optimization and readability, consider adding an early return for empty rows.func getSubjectsFromQueryResult(rows []meter.MeterQueryRow) []string { + // Early return for empty result set + if len(rows) == 0 { + return []string{} + } // Collect subjects from query results if any subjects := []string{} for _, row := range rows { if row.Subject == nil { continue } subjects = append(subjects, *row.Subject) } // Deduplicate subjects subjects = lo.Uniq(subjects) return subjects }
166-168: Consider adding timestamp to CSV filenameThe current implementation uses only the meter slug for the filename. Consider adding a timestamp to prevent overwriting previous exports and to help with file organization.
func (a *queryMeterCSVResult) FileName() string { - return fmt.Sprintf("%s.csv", a.meterSlug) + return fmt.Sprintf("%s_%s.csv", a.meterSlug, time.Now().Format("20060102150405")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openmeter/meter/httphandler/query_csv.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/meter/httphandler/query_csv.go (1)
143-147: Fixed column count issue correctlyThe code now properly appends two empty strings when a customer is absent, which matches the two columns in the header for customer information. This addresses the previous issue where three empty strings were being appended, causing column misalignment.
out.csv
Summary by CodeRabbit