refactor(meterexport): break up param signature #3743
Conversation
GAlexIHU
added
the
release-note/ignore
📝 WalkthroughWalkthroughRefactors export API to replace passing Changes
Sequence Diagram(s)(omitted — changes are refactors of parameter types and validation, not multi-component control-flow additions) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/meterexport/service.go (1)
68-88: Wrap validation errors withmodels.NewNillableGenericValidationError()for consistency.The validation looks solid, but it's inconsistent with the rest of the codebase. Nearly every other
Validate()method wraps the error withmodels.NewNillableGenericValidationError(errors.Join(errs...)). BothDataExportConfig.Validate()andDataExportParams.Validate()should follow the same pattern.
🧹 Nitpick comments (2)
openmeter/meterexport/service/service_test.go (2)
642-658: Missing ExportWindowTimeZone in invalid config test.This test case for "should return error for invalid config" doesn't set
ExportWindowTimeZone. While the test expects an error for missing meter ID, it would also trigger the "export window time zone is required" validation error.This is actually fine since both errors would be joined, and the test checks
Containsfor the meter ID error. But if you want the test to be more precise about which validation fails, you might consider setting the timezone.Optional: Make the test more specific
params := meterexport.DataExportParams{ DataExportConfig: meterexport.DataExportConfig{ - ExportWindowSize: meter.WindowSizeMinute, + ExportWindowSize: meter.WindowSizeMinute, + ExportWindowTimeZone: time.UTC, MeterID: models.NamespacedID{ Namespace: "test-ns", ID: "", // Missing ID }, },
266-287: Consider adding a test for missing ExportWindowTimeZone.The validation now requires
ExportWindowTimeZoneto be set, but there's no explicit test case verifying this validation error. Adding one would improve coverage of the new validation logic.Example test case to add
{ name: "should fail validation with missing time zone", meter: meter.Meter{}, params: meterexport.DataExportParams{ DataExportConfig: meterexport.DataExportConfig{ ExportWindowSize: meter.WindowSizeMinute, ExportWindowTimeZone: nil, // Missing timezone MeterID: models.NamespacedID{ Namespace: "test-ns", ID: "meter-1", }, }, Period: timeutil.StartBoundedPeriod{ From: now.Add(-5 * time.Minute), To: lo.ToPtr(now), }, }, wantErr: true, wantErrMsg: "export window time zone is required", },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openmeter/meterexport/service.goopenmeter/meterexport/service/service_test.goopenmeter/meterexport/service/syntheticdata.goopenmeter/meterexport/service/syntheticdata_iter.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/meterexport/service/syntheticdata_iter.goopenmeter/meterexport/service/syntheticdata.goopenmeter/meterexport/service.goopenmeter/meterexport/service/service_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/meterexport/service/service_test.go
🧬 Code graph analysis (4)
openmeter/meterexport/service/syntheticdata_iter.go (1)
openmeter/meterexport/service.go (2)
DataExportParams(90-95)DataExportConfig(57-66)
openmeter/meterexport/service/syntheticdata.go (2)
openmeter/meterexport/service.go (2)
DataExportParams(90-95)DataExportConfig(57-66)openmeter/streaming/connector.go (1)
RawEvent(24-36)
openmeter/meterexport/service.go (2)
pkg/timeutil/boundedperiod.go (1)
StartBoundedPeriod(8-11)pkg/models/validator.go (1)
Validate(16-26)
openmeter/meterexport/service/service_test.go (4)
openmeter/meterexport/service.go (2)
DataExportParams(90-95)DataExportConfig(57-66)pkg/models/id.go (1)
NamespacedID(7-10)app/common/namespace.go (1)
Namespace(13-15)pkg/timeutil/boundedperiod.go (1)
StartBoundedPeriod(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Lint
- GitHub Check: Artifacts / Container image
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Build
🔇 Additional comments (9)
openmeter/meterexport/service.go (2)
57-66: Clean separation of concerns here.The
DataExportConfignow holds the reusable configuration aspects (window size, timezone, meter ID), making it independent of any specific export operation. This aligns well with the PR objective of makingConfigusable separately for upcoming cloud-related work.
90-109: DataExportParams wrapper is well-structured.The embedding of
DataExportConfigallows access to its fields directly onDataExportParams, which keeps the API ergonomic. The compositeValidate()method correctly delegates to both the embedded config and the period.openmeter/meterexport/service/syntheticdata.go (2)
57-66: Signature update looks good.The method now takes
DataExportParamsand correctly accesses the embeddedDataExportConfigfor validation. Clean transition.
119-131: Query params construction is correct.The fields are properly extracted from the
paramsstruct:
FromandTofromparams.PeriodWindowSizeandWindowTimeZonefrom the embeddedDataExportConfigThis aligns with the type definitions in
service.go.openmeter/meterexport/service/syntheticdata_iter.go (2)
13-17: Upfront validation correctly uses embedded config.The iterator wrapper validates using
params.DataExportConfigwhich is appropriate sincevalidateAndGetMeterexpects aDataExportConfig. The rest of the iterator logic remains unchanged, which is expected.
30-36: Goroutine correctly passes the full params object.Since
ExportSyntheticMeterDatanow expectsDataExportParams, passing the entireparamshere is correct.openmeter/meterexport/service/service_test.go (3)
53-98: Test case structure updated correctly.The test data now uses
meterexport.DataExportParamswith the embeddedDataExportConfig. TheExportWindowTimeZone: time.UTCis consistently set across all test cases, which aligns with the new validation requirement.
249-251: Good use of embedded config for descriptor lookup.Using
tt.params.DataExportConfigto callGetTargetMeterDescriptoris the right approach since that method still acceptsDataExportConfig(notDataExportParams), keeping the descriptor lookup independent of the period.
292-454: Context cancellation tests are thorough.Good coverage for both mid-export cancellation and pre-canceled context scenarios. The tests verify that exactly one
context.Cancelederror is emitted (no duplicates), which is important for the streaming error handling behavior.
GAlexIHU
force-pushed
the
refactor/meterexport
branch
from
9373901 to
54d65e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openmeter/meterexport/service.go (1)
75-77: Good validation, but consider the error message.The validation correctly catches nil time zones, but the error message "export window time zone is required" might not give users enough guidance. Consider mentioning that it should be a valid
*time.Locationor providing an example.Optional: More descriptive error message
if c.ExportWindowTimeZone == nil { - errs = append(errs, errors.New("export window time zone is required")) + errs = append(errs, errors.New("export window time zone is required (e.g., time.UTC or time.Local)")) }openmeter/meterexport/service/service_test.go (1)
642-658: Nice edge case coverage!This test verifies that validation works correctly with the new structure, specifically checking that missing meter ID is caught. However, I notice that
ExportWindowTimeZoneis missing from this test case's config (line 644-648), but the test expects to catch the missing ID error.This might accidentally also catch the missing timezone validation error. Consider either:
- Setting the timezone to test only the ID validation
- Verifying that both errors are returned (since validation aggregates errors)
Optional: Make validation test more specific
params := meterexport.DataExportParams{ DataExportConfig: meterexport.DataExportConfig{ ExportWindowSize: meter.WindowSizeMinute, + ExportWindowTimeZone: time.UTC, MeterID: models.NamespacedID{ Namespace: "test-ns", ID: "", // Missing ID }, },Or alternatively, verify both errors are present:
_, err = svc.ExportSyntheticMeterDataIter(context.Background(), params) require.Error(t, err) assert.Contains(t, err.Error(), "meter id is required") + assert.Contains(t, err.Error(), "export window time zone is required")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openmeter/meterexport/service.goopenmeter/meterexport/service/service_test.goopenmeter/meterexport/service/syntheticdata.goopenmeter/meterexport/service/syntheticdata_iter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/meterexport/service/syntheticdata_iter.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/meterexport/service/service_test.goopenmeter/meterexport/service.goopenmeter/meterexport/service/syntheticdata.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/meterexport/service/service_test.go
🧠 Learnings (1)
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/meterexport/service/service_test.go
🧬 Code graph analysis (3)
openmeter/meterexport/service/service_test.go (3)
openmeter/meterexport/service.go (2)
DataExportParams(90-95)DataExportConfig(57-66)pkg/models/id.go (1)
NamespacedID(7-10)pkg/timeutil/boundedperiod.go (1)
StartBoundedPeriod(8-11)
openmeter/meterexport/service.go (2)
pkg/timeutil/boundedperiod.go (1)
StartBoundedPeriod(8-11)pkg/models/validator.go (1)
Validate(16-26)
openmeter/meterexport/service/syntheticdata.go (2)
openmeter/meterexport/service.go (2)
DataExportParams(90-95)DataExportConfig(57-66)openmeter/streaming/connector.go (1)
RawEvent(24-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/meterexport/service.go (2)
61-62: Nice separation of concerns here!The
ExportWindowTimeZonefield makes sense as part ofDataExportConfigsince it defines how the export windows are calculated. The pointer allows for explicit validation, though you might want to consider if there's a sensible default (like UTC) that could be applied when nil, or if explicit nil is the right approach for your use case.
90-109: Clean validation aggregation!The
DataExportParams.Validate()method nicely aggregates errors from both the embedded config and the period. The use oferrors.Jointhroughout provides a great user experience by reporting all validation issues at once rather than one at a time.I also checked
Period.Validate()(inpkg/timeutil/boundedperiod.go), and it's solid—it properly validates thatFromis required and thatFromcomes beforeTo, with good handling of the optionalTofield.openmeter/meterexport/service/syntheticdata.go (2)
57-66: Refactoring applied correctly!The method signature update and the access to
params.DataExportConfigfor validation are both correct. The embedded struct pattern means you can accessDataExportConfigfields directly throughparams, but being explicit withparams.DataExportConfigin the validation call is clearer and more maintainable.
121-130: Clean parameter extraction from the new structure!The funnel parameters now correctly pull from the new
DataExportParamsstructure:
params.Period.Fromandparams.Period.Tofor the time rangeparams.ExportWindowSizeandparams.ExportWindowTimeZonefor window configurationThis is exactly what you'd expect from the refactoring. The field access is correct since
DataExportConfigis embedded inDataExportParams.openmeter/meterexport/service/service_test.go (3)
79-92: Test structure updated consistently!All test cases now properly construct
DataExportParamswith the embeddedDataExportConfigand separatePeriod. The structure is clean and consistent across all tests.One minor observation: you're correctly setting
ExportWindowTimeZone: time.UTCin all tests, which is good for avoiding nil validation errors.
239-250: Good test pattern with descriptor verification!The test correctly:
- Calls
ExportSyntheticMeterDatawith the full params (line 239)- Separately calls
GetTargetMeterDescriptorwith just the config (line 250)This verifies that the config can be used independently, which aligns perfectly with the PR objective to enable config reuse.
337-350: Context cancellation test properly updated!The test constructs the params structure correctly for the cancellation scenario. The large time range (1000 minutes) ensures multiple query iterations, which is good for testing mid-operation cancellation.
Overview
Depends on #3689
Breaks up Config signature so Config can separately be used (see later cloud PRs)
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.