Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSystematic timestamp normalization was added across the charges subsystem: new normalization helpers, Intent/State Normalized() methods, normalization at service/adapter/persistence boundaries, input constructors and getters for patch types, and tests verifying sub-second truncation behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openmeter/billing/charges/usagebased/charge.go (1)
137-155:⚠️ Potential issue | 🟡 MinorReject non-nil zero
AdvanceAftervalues.
NormalizeOptionalTimestampkeeps&time.Time{}as a non-nil zero timestamp, and this state still validates successfully. Flat-fee already guards the same shape, so usage-based can quietly carry an invalidAdvanceAfter.💡 Suggested fix
func (s State) Validate() error { var errs []error if s.CurrentRealizationRunID != nil && *s.CurrentRealizationRunID == "" { errs = append(errs, fmt.Errorf("current realization run ID must be non-empty")) } + + if s.AdvanceAfter != nil && s.AdvanceAfter.IsZero() { + errs = append(errs, fmt.Errorf("advance after is required")) + } return errors.Join(errs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/charge.go` around lines 137 - 155, State.Normalized currently calls meta.NormalizeOptionalTimestamp but that helper leaves a non-nil &time.Time{} as a valid pointer; update normalization and/or validation so zero timestamps are treated as nil and rejected: in State.Normalized (and/or in Validate) ensure s.AdvanceAfter is set to nil when it is zero (time.Time.IsZero()), and in State.Validate add a check that if s.AdvanceAfter != nil and s.AdvanceAfter.IsZero() then return an error (or append to errs) stating advanceAfter must be a non-zero timestamp; reference functions/fields: State, Normalized(), Validate(), AdvanceAfter, and meta.NormalizeOptionalTimestamp.openmeter/billing/charges/meta/patchextend.go (1)
104-123:⚠️ Potential issue | 🟠 MajorNormalize
intentbefore these extend comparisons.The new patch values are already truncated in the constructor, but
ValidateWithstill compares them against the rawintentperiods. During the rollout, a pre-existing charge with sub-second...Tovalues can fail a same-second extend here after truncation. Normalizingintentup front keeps the comparison stable.Worth mirroring the same first line in
PatchShrink.ValidateWith, too.💡 Suggested fix
func (p PatchExtend) ValidateWith(intent Intent) error { + intent = intent.Normalized() + var errs []error if err := p.Validate(); err != nil { errs = append(errs, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/meta/patchextend.go` around lines 104 - 123, ValidateWith on PatchExtend compares truncated new times against the raw intent, causing false failures; before the comparisons in PatchExtend.ValidateWith you should normalize/truncate the intent's time fields (ServicePeriod.To, FullServicePeriod.To, BillingPeriod.To) to the same granularity used in the constructor (e.g., truncate to seconds) so comparisons with GetNewServicePeriodTo, GetNewFullServicePeriodTo, and GetNewBillingPeriodTo are consistent; apply the same initial normalization line in PatchShrink.ValidateWith to mirror behavior.openmeter/billing/charges/usagebased/service/creditsonly.go (1)
175-186:⚠️ Potential issue | 🟠 MajorNormalization is shifting the usage query cutoff and can drop events.
Line 175 and Line 276 now normalize
storedAtOffsetbefore callinggetRatingForUsage. Since the query uses an exclusive< StoredAtOffsetfilter, this can exclude events between the raw timestamp and the truncated timestamp.Suggested fix (keep raw cutoff for query, keep normalized for persistence)
- storedAtOffset := meta.NormalizeTimestamp(clock.Now().Add(-usagebased.InternalCollectionPeriod)) + rawStoredAtOffset := clock.Now().Add(-usagebased.InternalCollectionPeriod) + storedAtOffset := meta.NormalizeTimestamp(rawStoredAtOffset) ratingResult, err := s.Service.getRatingForUsage(ctx, getRatingForUsageInput{ Charge: s.Charge, Customer: s.CustomerOverride, FeatureMeter: s.FeatureMeter, - StoredAtOffset: storedAtOffset, + StoredAtOffset: rawStoredAtOffset, })Apply the same pattern in
FinalizeRealizationRunas well.Also applies to: 276-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditsonly.go` around lines 175 - 186, The code normalizes storedAtOffset before calling s.Service.getRatingForUsage which uses an exclusive "< StoredAtOffset" filter and may drop events; change to compute rawStoredAt := clock.Now().Add(-usagebased.InternalCollectionPeriod), then compute normalizedStoredAt := meta.NormalizeTimestamp(rawStoredAt) and pass rawStoredAt to getRatingForUsage via getRatingForUsageInput.StoredAtOffset while using normalizedStoredAt only for persistence/metadata (e.g., any fields or stores that require truncated timestamps). Apply the same change in FinalizeRealizationRun (the block around lines 276-283) so the query cutoff uses the raw timestamp and normalization is only used for persisted offsets.
🧹 Nitpick comments (1)
openmeter/billing/charges/flatfee/adapter/charge.go (1)
47-47: Redundant.In(time.UTC)call afterNormalizeTimestamp.
meta.NormalizeTimestamp(...)already returns a UTC time, so the.In(time.UTC)conversion is unnecessary. Same applies to line 227. Not a bug, just a bit of extra work.✨ Optional cleanup
- SetInvoiceAt(meta.NormalizeTimestamp(intent.InvoiceAt).In(time.UTC)). + SetInvoiceAt(meta.NormalizeTimestamp(intent.InvoiceAt)).And at line 227:
- SetInvoiceAt(meta.NormalizeTimestamp(intent.InvoiceAt).In(time.UTC)). + SetInvoiceAt(meta.NormalizeTimestamp(intent.InvoiceAt)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/flatfee/adapter/charge.go` at line 47, The call to .In(time.UTC) after meta.NormalizeTimestamp(...) is redundant because NormalizeTimestamp already returns a UTC time; remove the .In(time.UTC) chaining wherever it appears (e.g., the SetInvoiceAt(...) call and the second occurrence around the other timestamp at the same file) so the code simply uses meta.NormalizeTimestamp(...) directly; look for usages of SetInvoiceAt and any other calls that currently do meta.NormalizeTimestamp(...).In(time.UTC) and drop the .In(time.UTC) suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/billing/charges/meta/patchextend.go`:
- Around line 104-123: ValidateWith on PatchExtend compares truncated new times
against the raw intent, causing false failures; before the comparisons in
PatchExtend.ValidateWith you should normalize/truncate the intent's time fields
(ServicePeriod.To, FullServicePeriod.To, BillingPeriod.To) to the same
granularity used in the constructor (e.g., truncate to seconds) so comparisons
with GetNewServicePeriodTo, GetNewFullServicePeriodTo, and GetNewBillingPeriodTo
are consistent; apply the same initial normalization line in
PatchShrink.ValidateWith to mirror behavior.
In `@openmeter/billing/charges/usagebased/charge.go`:
- Around line 137-155: State.Normalized currently calls
meta.NormalizeOptionalTimestamp but that helper leaves a non-nil &time.Time{} as
a valid pointer; update normalization and/or validation so zero timestamps are
treated as nil and rejected: in State.Normalized (and/or in Validate) ensure
s.AdvanceAfter is set to nil when it is zero (time.Time.IsZero()), and in
State.Validate add a check that if s.AdvanceAfter != nil and
s.AdvanceAfter.IsZero() then return an error (or append to errs) stating
advanceAfter must be a non-zero timestamp; reference functions/fields: State,
Normalized(), Validate(), AdvanceAfter, and meta.NormalizeOptionalTimestamp.
In `@openmeter/billing/charges/usagebased/service/creditsonly.go`:
- Around line 175-186: The code normalizes storedAtOffset before calling
s.Service.getRatingForUsage which uses an exclusive "< StoredAtOffset" filter
and may drop events; change to compute rawStoredAt :=
clock.Now().Add(-usagebased.InternalCollectionPeriod), then compute
normalizedStoredAt := meta.NormalizeTimestamp(rawStoredAt) and pass rawStoredAt
to getRatingForUsage via getRatingForUsageInput.StoredAtOffset while using
normalizedStoredAt only for persistence/metadata (e.g., any fields or stores
that require truncated timestamps). Apply the same change in
FinalizeRealizationRun (the block around lines 276-283) so the query cutoff uses
the raw timestamp and normalization is only used for persisted offsets.
---
Nitpick comments:
In `@openmeter/billing/charges/flatfee/adapter/charge.go`:
- Line 47: The call to .In(time.UTC) after meta.NormalizeTimestamp(...) is
redundant because NormalizeTimestamp already returns a UTC time; remove the
.In(time.UTC) chaining wherever it appears (e.g., the SetInvoiceAt(...) call and
the second occurrence around the other timestamp at the same file) so the code
simply uses meta.NormalizeTimestamp(...) directly; look for usages of
SetInvoiceAt and any other calls that currently do
meta.NormalizeTimestamp(...).In(time.UTC) and drop the .In(time.UTC) suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18e3c4e2-2509-403f-962e-1572837eb1ba
📒 Files selected for processing (20)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/meta/patchdelete.goopenmeter/billing/charges/meta/patchextend.goopenmeter/billing/charges/meta/patchshrink.goopenmeter/billing/charges/meta/timestamps.goopenmeter/billing/charges/models/chargemeta/mixin.goopenmeter/billing/charges/service/patchtmp.goopenmeter/billing/charges/service/truncation_test.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/realizationrun.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcharge.go
tothandras
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This change introduces centralized timestamp normalization for charge persistence by truncating persisted intent, state, and realization-run timestamps to streaming.MinimumWindowSizeDuration through shared helpers in openmeter/billing/charges/meta.
Billing already had this for flat fees and usage based lines, so this is feature parity.
Normalization is now applied at the domain layer before validation and calculation, and at persistence write sites for fields like InvoiceAt, AdvanceAfter, AsOf, and CollectionEnd, so stored values are consistently second-aligned.
Shrink, extend, and delete patches were tightened up by adding constructors, making patch payload fields private, and introducing validated input structs plus accessors for shrink and extend patch data.
The temporary shrink/extend remap and the subscription sync reconciler were updated to use the new patch constructors so normalized patch timestamps are created at construction time instead of being assembled ad hoc.
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation