refactor: notification event reconciling #3581
Conversation
📝 WalkthroughWalkthroughAdds per-attempt delivery records and next-attempt scheduling to notification delivery statuses; introduces configurable pending/sending timeouts and defaults; refactors reconciliation to channel-type (webhook) processing; updates DB schema, adapters, API mappings, webhook errors/handlers, svix integration, wiring, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant Reconciler
participant Repo as EventRepo
participant WebhookHandler
participant Adapter
Scheduler->>Reconciler: Trigger Reconcile(now)
Reconciler->>Repo: ListEvents(NextAttemptBefore=now)
Repo-->>Reconciler: events[]
loop per channel-type
Reconciler->>WebhookHandler: reconcileWebhookEvent(event, channelType)
WebhookHandler->>WebhookHandler: evaluate delivery statuses (pending/sending)
alt needs transition -> sending
WebhookHandler->>Adapter: Update status to Sending, set Attempts/NextAttempt
end
alt send attempt
WebhookHandler->>WebhookHandler: send, record attempt (response,duration,url)
alt success
WebhookHandler->>Adapter: Update status=Success, Attempts
else retryable
WebhookHandler->>Adapter: Append Attempt, set NextAttempt
else unrecoverable
WebhookHandler->>Adapter: Update status=Failed, Attempts
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Suggested labels
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: 5
🧹 Nitpick comments (3)
openmeter/notification/service/rule.go (1)
41-43: Optional: Duplicate validation check.This check for empty channels is already performed in
CreateRuleInput.Validate()andUpdateRuleInput.Validate()(lines 212-214, 274-276 in rule.go), which are called before this validator. You could simplify by removing this duplicate check.openmeter/notification/webhook/handler.go (1)
242-249: Add validation to GetMessageInput for consistency.Good catch! The search confirms that
GetMessageInputcurrently lacks struct-level validation, while other input types in the same file implementmodels.Validator. The validation is happening at the handler layer in the svix implementation (where it checks that eitherIDorEventIDis provided), but it should really live on the struct itself to match the codebase's existing patterns and catch issues earlier.Your suggested validation looks solid—it'll enforce that
Namespaceand at least one ofID/EventIDare provided, which aligns with how the field is actually used across the codebase.pkg/models/annotation.go (1)
23-39: GetString conflates missing keys with empty values.The method returns
("", false)both when a key is missing and when it exists with an empty string value. This makes it impossible to distinguish the two cases. If you need to differentiate, consider adjusting the logic—but if empty strings are equivalent to missing keys for your use case, this works fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/__init__.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlopenmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/notificationeventdeliverystatus.gois excluded by!**/ent/db/**openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.gois excluded by!**/ent/db/**openmeter/ent/db/notificationeventdeliverystatus/where.gois excluded by!**/ent/db/**openmeter/ent/db/notificationeventdeliverystatus_create.gois excluded by!**/ent/db/**openmeter/ent/db/notificationeventdeliverystatus_update.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (37)
api/spec/src/notification/event.tsp(1 hunks)app/common/notification.go(2 hunks)app/config/notification.go(3 hunks)cmd/balance-worker/wire_gen.go(1 hunks)cmd/jobs/internal/wire_gen.go(1 hunks)cmd/notification-service/wire_gen.go(1 hunks)cmd/server/wire_gen.go(1 hunks)openmeter/ent/schema/notification.go(2 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(2 hunks)openmeter/notification/adapter/event.go(1 hunks)openmeter/notification/annotations.go(1 hunks)openmeter/notification/channel.go(2 hunks)openmeter/notification/deliverystatus.go(3 hunks)openmeter/notification/event.go(1 hunks)openmeter/notification/eventhandler.go(1 hunks)openmeter/notification/eventhandler/deliverystatus.go(1 hunks)openmeter/notification/eventhandler/dispatch.go(1 hunks)openmeter/notification/eventhandler/handler.go(4 hunks)openmeter/notification/eventhandler/reconcile.go(4 hunks)openmeter/notification/eventhandler/webhook.go(1 hunks)openmeter/notification/eventpayload.go(1 hunks)openmeter/notification/httpdriver/mapping.go(2 hunks)openmeter/notification/rule.go(2 hunks)openmeter/notification/service/rule.go(4 hunks)openmeter/notification/webhook/errors.go(2 hunks)openmeter/notification/webhook/handler.go(9 hunks)openmeter/notification/webhook/noop/noop.go(1 hunks)openmeter/notification/webhook/secret/secret.go(2 hunks)openmeter/notification/webhook/svix/annotations.go(1 hunks)openmeter/notification/webhook/svix/internal/error.go(5 hunks)openmeter/notification/webhook/svix/message.go(2 hunks)openmeter/notification/webhook/svix/webhook.go(4 hunks)pkg/models/annotation.go(1 hunks)test/notification/channel.go(2 hunks)tools/migrate/migrations/20251106124357_notification_deliverystatus.down.sql(1 hunks)tools/migrate/migrations/20251106124357_notification_deliverystatus.up.sql(1 hunks)
🧰 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/notification/event.goopenmeter/notification/annotations.goopenmeter/notification/rule.goapp/config/notification.gotest/notification/channel.goopenmeter/notification/webhook/noop/noop.goopenmeter/notification/adapter/deliverystatus.gocmd/jobs/internal/wire_gen.goapp/common/notification.gopkg/models/annotation.goopenmeter/notification/eventhandler/handler.gocmd/notification-service/wire_gen.gocmd/server/wire_gen.goopenmeter/notification/httpdriver/mapping.goopenmeter/notification/service/rule.goopenmeter/ent/schema/notification.goopenmeter/notification/eventhandler/dispatch.goopenmeter/notification/eventhandler/deliverystatus.goopenmeter/notification/adapter/event.goopenmeter/notification/channel.goopenmeter/notification/webhook/handler.goopenmeter/notification/webhook/svix/annotations.goopenmeter/notification/webhook/svix/webhook.goopenmeter/notification/eventhandler/webhook.goopenmeter/notification/eventhandler/reconcile.goopenmeter/notification/webhook/errors.goopenmeter/notification/adapter/entitymapping.goopenmeter/notification/webhook/secret/secret.goopenmeter/notification/webhook/svix/message.goopenmeter/notification/eventhandler.goopenmeter/notification/eventpayload.goopenmeter/notification/deliverystatus.gocmd/balance-worker/wire_gen.goopenmeter/notification/webhook/svix/internal/error.go
**/*.tsp
⚙️ CodeRabbit configuration file
**/*.tsp: Review the TypeSpec code for conformity with TypeSpec best practices. When recommending changes also consider the fact that multiple codegeneration toolchains depend on the TypeSpec code, each of which have their idiosyncrasies and bugs.The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.
Files:
api/spec/src/notification/event.tsp
🧬 Code graph analysis (25)
app/config/notification.go (1)
openmeter/notification/eventhandler.go (3)
DefaultReconcileInterval(9-9)DefaultDeliveryStateSendingTimeout(12-12)DefaultDeliveryStatePendingTimeout(11-11)
test/notification/channel.go (1)
openmeter/notification/webhook/secret/secret.go (1)
NewSigningSecretWithDefaultSize(24-26)
openmeter/notification/webhook/noop/noop.go (2)
openmeter/notification/webhook/handler.go (3)
Handler(288-292)GetMessageInput(242-249)Message(187-204)openmeter/notification/webhook/errors.go (1)
ErrNotImplemented(9-9)
openmeter/notification/adapter/deliverystatus.go (3)
api/client/javascript/src/client/schemas.ts (1)
Annotations(11692-11692)api/api.gen.go (1)
Annotations(1048-1048)api/client/go/client.gen.go (1)
Annotations(962-962)
cmd/jobs/internal/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
app/common/notification.go (1)
app/config/notification.go (1)
NotificationConfiguration(21-29)
openmeter/notification/eventhandler/handler.go (1)
openmeter/notification/eventhandler.go (2)
DefaultDeliveryStatePendingTimeout(11-11)DefaultDeliveryStateSendingTimeout(12-12)
cmd/notification-service/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
cmd/server/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
openmeter/notification/httpdriver/mapping.go (2)
api/api.gen.go (3)
NotificationEventDeliveryStatusState(5426-5426)NotificationEventDeliveryAttempt(5390-5399)EventDeliveryAttemptResponse(3464-3476)openmeter/notification/deliverystatus.go (2)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)
openmeter/notification/service/rule.go (5)
openmeter/notification/rule.go (2)
CreateRuleInput(168-185)UpdateRuleInput(228-245)openmeter/notification/service/service.go (1)
Service(20-29)openmeter/notification/channel.go (2)
ListChannelsInput(118-127)Channel(37-51)pkg/pagination/page.go (1)
Page(14-17)pkg/sortx/order.go (1)
Order(3-3)
openmeter/ent/schema/notification.go (1)
openmeter/notification/deliverystatus.go (1)
EventDeliveryAttempt(56-60)
openmeter/notification/eventhandler/deliverystatus.go (5)
openmeter/notification/event.go (1)
Event(51-67)openmeter/notification/channel.go (1)
ChannelType(19-19)openmeter/notification/deliverystatus.go (6)
EventDeliveryStatus(77-93)EventDeliveryStatusState(25-25)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSuccess(14-14)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateSending(16-16)openmeter/notification/rule.go (1)
Rule(17-33)openmeter/ent/db/notificationeventdeliverystatus/where.go (1)
ChannelID(90-92)
openmeter/notification/adapter/event.go (3)
openmeter/ent/db/notificationevent/where.go (1)
HasDeliveryStatusesWith(376-385)openmeter/ent/db/notificationeventdeliverystatus/where.go (3)
StateNotIn(411-417)NextAttemptAtIsNil(535-537)NextAttemptAtLTE(530-532)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusStateSuccess(14-14)EventDeliveryStatusStateFailed(15-15)
openmeter/notification/channel.go (1)
openmeter/notification/webhook/secret/secret.go (1)
ValidateSigningSecret(32-45)
openmeter/notification/webhook/handler.go (3)
openmeter/notification/webhook/secret/secret.go (1)
ValidateSigningSecret(32-45)openmeter/notification/webhook/errors.go (1)
NewValidationError(37-43)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusState(25-25)EventDeliveryAttempt(56-60)
openmeter/notification/webhook/svix/webhook.go (2)
openmeter/notification/webhook/secret/secret.go (1)
NewSigningSecretWithDefaultSize(24-26)openmeter/notification/webhook/svix/svix.go (1)
NullChannel(25-25)
openmeter/notification/eventhandler/webhook.go (9)
openmeter/notification/eventhandler/handler.go (2)
New(113-152)Handler(54-71)openmeter/notification/webhook/handler.go (8)
Handler(288-292)MessageDeliveryStatus(180-185)Message(187-204)GetMessageInput(242-249)ExpandParams(235-240)SendMessageInput(208-215)EventType(261-268)Payload(178-178)openmeter/notification/event.go (2)
Event(51-67)EventType(31-31)openmeter/notification/webhook/errors.go (4)
IsNotFoundError(69-71)IsMessageNotReadyError(134-136)IsUnrecoverableError(150-152)RetryableError(79-83)pkg/clock/clock.go (1)
Now(14-21)openmeter/notification/deliverystatus.go (5)
UpdateEventDeliveryStatusInput(180-193)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusStateSuccess(14-14)pkg/framework/tracex/tracex.go (2)
StartWithNoValue(75-82)Start(32-39)openmeter/notification/httpdriver/mapping.go (4)
FromEventAsBalanceThresholdPayload(581-599)FromEventAsEntitlementResetPayload(601-618)FromEventAsInvoiceCreatedPayload(620-636)FromEventAsInvoiceUpdatedPayload(638-654)openmeter/notification/eventpayload.go (1)
AsRawPayload(14-26)
openmeter/notification/eventhandler/reconcile.go (5)
openmeter/notification/eventhandler/handler.go (1)
Handler(54-71)openmeter/notification/event.go (2)
Event(51-67)ListEventsInput(74-97)openmeter/notification/channel.go (3)
Channel(37-51)ChannelType(19-19)ChannelTypeWebhook(14-14)openmeter/notification/deliverystatus.go (3)
EventDeliveryStatusState(25-25)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateSending(16-16)pkg/clock/clock.go (1)
Now(14-21)
openmeter/notification/adapter/entitymapping.go (1)
openmeter/ent/db/notificationeventdeliverystatus/where.go (1)
NextAttemptAt(100-102)
openmeter/notification/webhook/secret/secret.go (1)
pkg/models/errors.go (1)
NewNillableGenericValidationError(129-135)
openmeter/notification/webhook/svix/message.go (6)
openmeter/notification/webhook/handler.go (5)
EventType(261-268)Message(187-204)Payload(178-178)MessageDeliveryStatus(180-185)GetMessageInput(242-249)openmeter/notification/webhook/svix/internal/error.go (1)
WrapSvixError(68-110)openmeter/notification/webhook/errors.go (2)
NewMessageNotReadyError(127-132)NewValidationError(37-43)openmeter/notification/webhook/svix/annotations.go (4)
AnnotationMessageID(4-4)AnnotationMessageEventID(5-5)AnnotationEventType(6-6)AnnotationApplicationUID(8-8)pkg/framework/tracex/tracex.go (1)
Start(32-39)openmeter/notification/deliverystatus.go (6)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusState(25-25)EventDeliveryStatusStateSuccess(14-14)
openmeter/notification/deliverystatus.go (5)
api/client/javascript/src/client/schemas.ts (2)
EventDeliveryAttemptResponse(11918-11919)Annotations(11692-11692)api/api.gen.go (2)
EventDeliveryAttemptResponse(3464-3476)Annotations(1048-1048)api/client/go/client.gen.go (2)
EventDeliveryAttemptResponse(3177-3189)Annotations(962-962)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/notificationeventdeliverystatus/where.go (5)
EventID(85-87)ChannelID(90-92)Reason(95-97)CreatedAt(75-77)UpdatedAt(80-82)
cmd/balance-worker/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
openmeter/notification/webhook/svix/internal/error.go (1)
openmeter/notification/webhook/errors.go (4)
NewUnrecoverableError(154-162)NewValidationError(37-43)NewNotFoundError(61-67)NewRetryableError(103-116)
⏰ 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). (8)
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
openmeter/notification/eventpayload.go (2)
12-13: Nice addition for type clarity!Introducing the
RawPayloadtype alias makes the intent clearer and improves type documentation. Good refactoring.
14-26: Breaking change is all set—nice work on the complete migration!Verified that all call sites have been updated to use
AsRawPayload. The newRawPayloadtype is properly defined, and there are zero remaining references to the oldPayloadToMapInterfacefunction anywhere in the codebase. The logic is sound and the migration is complete.openmeter/notification/rule.go (2)
212-214: Nice addition!This validation ensures rules can't be created without channels, catching the issue early at the model level before hitting the service/database layers.
274-276: Consistent validation, looks good!The same channel requirement is enforced for updates, maintaining consistency with the create operation.
openmeter/notification/service/rule.go (2)
96-99: Good addition for validation flow!Adding channel existence validation before creating the rule is a solid improvement. Once the pagination bug in
ValidateRuleChannelsis fixed, this will properly prevent rules from being created with non-existent channels.
214-220: Clearer error message and consistent validation!The error message on line 214 is now more specific ("invalid rule type"), and the channel validation addition mirrors the CreateRule implementation nicely. Once the pagination issue is resolved, this will work as intended.
openmeter/notification/webhook/noop/noop.go (1)
73-77: LGTM! Consistent noop implementation.The
GetMessagemethod follows the established pattern in this file perfectly—logging the request and returning the not-implemented error. Nice and clean!openmeter/notification/webhook/handler.go (7)
30-30: Nice refactor to public validator interface!Switching from a local validator interface to
models.Validatoris clean and consistent across all input types. This makes the validation contract more explicit and reusable.Also applies to: 50-50, 87-87, 130-130, 154-154, 206-206
79-79: Secret validation looks good!Delegating to
secret.ValidateSigningSecretkeeps the validation logic centralized and properly checks the secret format. The nil checks before validation are appropriate too.Also applies to: 122-122
178-178: Clean type alias for payload!Using
Payload = map[string]anygives semantic meaning to the type while keeping the flexibility of a map. Nice and idiomatic!
180-185: Solid delivery status struct!The
MessageDeliveryStatusdesign looks great—theNextAttemptpointer handles optional retry timing nicely, and tracking multiple attempts aligns perfectly with the PR's goal of adding delivery attempts to delivery status.
187-204: Well-structured message expansion!The addition of
Annotationsand the optional expanded fields (PayloadandDeliveryStatuses) as pointers is spot-on. This pattern makes it clear when data was requested vs. not included, avoiding confusion with empty values.
235-240: Clean expansion control!The
ExpandParamsstruct gives callers a straightforward way to request optional data. The boolean flags are simple and effective.
283-286: Interface extension looks good!Adding
GetMessageto theMessageHandlerinterface completes the message lifecycle nicely—now you can both send and retrieve messages. The method signature is consistent with the rest of the interface.openmeter/notification/adapter/event.go (1)
51-59: LGTM! Retry eligibility filter looks solid.The filter correctly narrows down events for reconciliation by excluding finalized deliveries and selecting those ready for a next attempt. The
Orcondition elegantly handles both cases: events without a scheduled next attempt and events whose next attempt time has arrived.openmeter/notification/annotations.go (1)
21-24: Nice! Annotation constants look good.The new constants follow existing naming conventions, and I appreciate the explicit deprecated annotation for backward compatibility during migration.
openmeter/notification/eventhandler.go (1)
9-13: LGTM! Timeout constants are sensible.The new timeout values (3h for pending, 48h for sending) provide reasonable guardrails for delivery state transitions. The formatting alignment is clean and consistent with the existing constants.
pkg/models/annotation.go (2)
59-71: Clone looks good!Clean shallow copy implementation that handles nil receivers correctly.
73-89: Merge implementation is solid.The nil-safe merge logic is clean, and the semantics (later values overwrite earlier ones) are intuitive and conventional.
openmeter/notification/webhook/svix/annotations.go (1)
1-10: LGTM! SVIX annotation constants are well-organized.Nice job keeping the SVIX-specific constants in their own package with a consistent
svix.prefix. Clean and self-documenting.test/notification/channel.go (2)
11-11: Import refactor looks good.The webhook secret functionality has been properly moved to its own package with a clear alias.
111-111: Usage updated correctly.Test code properly reflects the new import path and alias.
tools/migrate/migrations/20251106124357_notification_deliverystatus.up.sql (1)
1-4: Migration looks solid.The new columns and index align well with the retry eligibility query filter added in the adapter. The composite index on
(namespace, state, next_attempt_at)should efficiently support the reconciliation queries filtering by state and next attempt time.openmeter/notification/channel.go (2)
7-7: Import refactor applied consistently.The webhook secret package import matches the refactoring seen in the test files.
105-105: Validation call updated correctly.Proper usage of the refactored signing secret validation function.
openmeter/notification/httpdriver/mapping.go (1)
547-563: Love the richer attempt payload mapping. Pulling duration, status, body, and URL into the API model keeps the story tight—looks great.openmeter/notification/deliverystatus.go (1)
80-93: The review comment is incorrect—EventDeliveryStatus is never marshaled directly to JSON.I searched for direct JSON marshaling of
EventDeliveryStatusand found none. Instead, the codebase uses an explicit mapper pattern:httpdriver/mapping.gotransforms the domain struct into a separateapi.NotificationEventDeliveryStatusstruct before serialization. This means the domain struct's JSON tags are irrelevant—the API response shape is controlled entirely by the API struct's tags.This is a clean domain/API model separation pattern, so dropping JSON tags from the domain struct has zero impact on serialization behavior or downstream consumers. No regressions will occur.
Likely an incorrect or invalid review comment.
chrisgacsal
force-pushed
the
refactor/reconcile
branch
from
a310b71 to
ec4a0ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/notification/webhook/svix/internal/error.go (1)
19-25: Now let me verify if the Svix SDK error object provides access to response headers:Let me search more specifically for the Svix Go SDK Error type to understand what methods/fields it exposes:Based on my verification of the code, the review comment correctly identifies a real issue. The
RetryAfterfield is defined in theSvixErrorstruct and is actually being used in theWrap()method (lines 46, 48) to pass retry timing information towebhook.NewRetryableError(). However, the field is never populated whenSvixErrorinstances are created.Both instantiation points (lines 86 and 102) only set
HTTPStatus,Code, andDetails— leavingRetryAfterat its zero value (0 duration). The Svix SDK error object appears to only exposeStatus()andBody()methods, with no apparent header access to extract theRetry-Afterheader that would typically come from HTTP 429 or 5xx responses.
Wire up
RetryAfterfield extraction from response headers.The
RetryAfterfield inSvixErroris defined and used in theWrap()method for retryable errors (429, 500, 502, 503, 504), but it's never populated when the error is constructed. You'll need to extract this from HTTP response headers (typicallyRetry-AfterorX-RateLimit-Reset) when handling error responses. Verify that the Svix SDK error object exposes the response headers, then parse and populateRetryAfteraccordingly in the error construction at lines 86 and 102.
♻️ Duplicate comments (4)
openmeter/notification/service/rule.go (1)
45-49: Critical: Pagination parameters are swapped!As previously flagged, this will only fetch 1 channel from page N instead of fetching all N channels from page 1. If you're validating 3 channels, this grabs 1 item from page 3, completely skipping the first two pages. Existing channels will be incorrectly reported as missing.
Swap them:
- Page: pagination.Page{ - PageSize: 1, - PageNumber: len(channels), - }, + Page: pagination.Page{ + PageSize: len(channels), + PageNumber: 1, + },openmeter/notification/webhook/svix/webhook.go (1)
253-257: Please restore the event-type-only flow.Line 253 currently drops
NullChannelinto every update whenChannelsis empty—even ifEventTypesis populated. Svix requires both filters to match, so event-type-only endpoints fall silent after this update. Let’s gate the fallback behind bothlen(params.Channels) == 0andlen(params.EventTypes) == 0, just like we used to.- if len(params.Channels) == 0 { + if len(params.Channels) == 0 && len(params.EventTypes) == 0 { params.Channels = []string{ NullChannel, } }openmeter/notification/eventhandler/reconcile.go (1)
35-49: Don’t trust rule channels for reconciliation.Line 35 gathers channel types from
event.Rule.Channelsonly. If the repo doesn’t eager-load channels or one was deleted post-event, that slice won’t include the channel ID, so we never callreconcileWebhookEvent. Any pending/sending statuses tied to that channel just sit there forever. Please iterate the delivery statuses (or fall back to them) so we always reconcile what the DB holds.- channelTypes := lo.UniqMap(event.Rule.Channels, func(item notification.Channel, _ int) notification.ChannelType { - return item.Type - }) - - for _, channelType := range channelTypes { - switch channelType { - case notification.ChannelTypeWebhook: - if err := h.reconcileWebhookEvent(ctx, event); err != nil { - errs = append(errs, err) - } - default: - h.logger.ErrorContext(ctx, "unsupported channel type", "type", channelType) - } + for _, status := range event.DeliveryStatus { + switch status.ChannelType { + case notification.ChannelTypeWebhook: + if err := h.reconcileWebhookEvent(ctx, event); err != nil { + errs = append(errs, err) + } + default: + h.logger.ErrorContext(ctx, "unsupported channel type", "type", status.ChannelType) + }openmeter/notification/eventhandler/webhook.go (1)
334-337: Critical: Same bug flagged in past review—this prevents message creation.Line 335 has
if err != nil || webhook.IsNotFoundError(err), which returns an error whenever the message is not found. This prevents the function from falling through to the send path (lines 343-348), so brand-new messages can never be created. This is the exact issue raised in the past review comment.Apply this fix:
- msg, err := h.getWebhookMessage(ctx, event) - if err != nil || webhook.IsNotFoundError(err) { - return nil, fmt.Errorf("failed to get webhook message: %w", err) - } + msg, err := h.getWebhookMessage(ctx, event) + if err != nil && !webhook.IsNotFoundError(err) { + return nil, fmt.Errorf("failed to get webhook message: %w", err) + }Note: Check if this function is actually used—if it's dead code, consider removing it instead.
🧹 Nitpick comments (2)
openmeter/notification/webhook/svix/message.go (1)
105-246: Consider making rate-limit sleep configurable.The hardcoded 100ms sleep (lines 173, 234) between pagination requests is reasonable for avoiding rate limits, but it could add noticeable latency when fetching many pages of delivery attempts. For high-volume scenarios, this might become a bottleneck in the event ingestion path.
Consider extracting the sleep duration to a configurable constant at the package level:
+const ( + ListAttemptLimit = 25 + // PaginationSleepDuration is the delay between paginated API calls to avoid rate limits + PaginationSleepDuration = 100 * time.Millisecond +) - time.Sleep(100 * time.Millisecond) + time.Sleep(PaginationSleepDuration)This makes it easier to tune performance vs. rate-limit trade-offs in the future. Based on coding guidelines.
openmeter/notification/eventhandler/webhook.go (1)
231-248: Hardcoded 1-hour threshold for stuck messages.The logic to revert a message stuck in "sending" back to "pending" after 1 hour (line 240) is a reasonable safety net, but the duration is hardcoded. Consider making this configurable alongside
sendingTimeoutandpendingTimeoutfor consistency and easier tuning.+ stuckSendingThreshold time.Duration // e.g., in Handler struct - if clock.Now().Sub(stuckInSendingSince) > time.Hour { + if clock.Now().Sub(stuckInSendingSince) > h.stuckSendingThreshold {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
app/common/notification.go(2 hunks)app/config/config_test.go(1 hunks)app/config/notification.go(3 hunks)app/config/testdata/complete.yaml(1 hunks)cmd/balance-worker/wire_gen.go(1 hunks)cmd/jobs/internal/wire_gen.go(1 hunks)cmd/notification-service/wire_gen.go(1 hunks)cmd/server/wire_gen.go(1 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(2 hunks)openmeter/notification/adapter/event.go(1 hunks)openmeter/notification/channel.go(2 hunks)openmeter/notification/deliverystatus.go(3 hunks)openmeter/notification/event.go(1 hunks)openmeter/notification/eventhandler.go(1 hunks)openmeter/notification/eventhandler/deliverystatus.go(1 hunks)openmeter/notification/eventhandler/dispatch.go(1 hunks)openmeter/notification/eventhandler/handler.go(4 hunks)openmeter/notification/eventhandler/reconcile.go(4 hunks)openmeter/notification/eventhandler/webhook.go(1 hunks)openmeter/notification/eventpayload.go(1 hunks)openmeter/notification/httpdriver/mapping.go(2 hunks)openmeter/notification/rule.go(2 hunks)openmeter/notification/service/rule.go(4 hunks)openmeter/notification/webhook/errors.go(2 hunks)openmeter/notification/webhook/handler.go(9 hunks)openmeter/notification/webhook/noop/noop.go(1 hunks)openmeter/notification/webhook/secret/secret.go(2 hunks)openmeter/notification/webhook/svix/annotations.go(1 hunks)openmeter/notification/webhook/svix/internal/error.go(5 hunks)openmeter/notification/webhook/svix/message.go(2 hunks)openmeter/notification/webhook/svix/webhook.go(4 hunks)test/notification/channel.go(2 hunks)test/notification/rule.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/config/testdata/complete.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- openmeter/notification/event.go
- test/notification/channel.go
- cmd/balance-worker/wire_gen.go
- openmeter/notification/adapter/deliverystatus.go
- app/config/notification.go
- openmeter/notification/channel.go
- openmeter/notification/eventhandler.go
- openmeter/notification/webhook/secret/secret.go
- openmeter/notification/eventhandler/handler.go
- openmeter/notification/webhook/errors.go
- openmeter/notification/webhook/svix/annotations.go
- openmeter/notification/eventpayload.go
- openmeter/notification/eventhandler/deliverystatus.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/notification/webhook/noop/noop.goopenmeter/notification/webhook/svix/webhook.goapp/common/notification.goopenmeter/notification/httpdriver/mapping.goopenmeter/notification/deliverystatus.goopenmeter/notification/service/rule.gotest/notification/rule.goopenmeter/notification/eventhandler/dispatch.goopenmeter/notification/adapter/entitymapping.goopenmeter/notification/webhook/handler.goopenmeter/notification/eventhandler/reconcile.goapp/config/config_test.gocmd/jobs/internal/wire_gen.goopenmeter/notification/eventhandler/webhook.gocmd/server/wire_gen.goopenmeter/notification/webhook/svix/internal/error.goopenmeter/notification/rule.goopenmeter/notification/adapter/event.goopenmeter/notification/webhook/svix/message.gocmd/notification-service/wire_gen.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:
app/config/config_test.go
🧠 Learnings (1)
📚 Learning: 2025-04-29T19:53:31.951Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2746
File: openmeter/notification/adapter/rule.go:155-161
Timestamp: 2025-04-29T19:53:31.951Z
Learning: The `GetRule` method in the notification rule repository is intentionally designed to return rules even if they have been soft-deleted (`deleted_at` is set), while `ListRules` filters out deleted rules with `Where(ruledb.DeletedAtIsNil())`. This difference in behavior between get and list operations is by design.
Applied to files:
test/notification/rule.go
🧬 Code graph analysis (17)
openmeter/notification/webhook/noop/noop.go (2)
openmeter/notification/webhook/handler.go (3)
Handler(288-292)GetMessageInput(242-249)Message(187-204)openmeter/notification/webhook/errors.go (1)
ErrNotImplemented(9-9)
openmeter/notification/webhook/svix/webhook.go (2)
openmeter/notification/webhook/secret/secret.go (1)
NewSigningSecretWithDefaultSize(24-26)openmeter/notification/webhook/svix/svix.go (1)
NullChannel(25-25)
app/common/notification.go (1)
app/config/notification.go (1)
NotificationConfiguration(21-29)
openmeter/notification/httpdriver/mapping.go (2)
api/api.gen.go (4)
Annotations(1048-1048)NotificationEventDeliveryStatusState(5426-5426)NotificationEventDeliveryAttempt(5390-5399)EventDeliveryAttemptResponse(3464-3476)openmeter/notification/deliverystatus.go (2)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)
openmeter/notification/deliverystatus.go (5)
api/client/javascript/src/client/schemas.ts (2)
EventDeliveryAttemptResponse(11918-11919)Annotations(11692-11692)api/api.gen.go (2)
EventDeliveryAttemptResponse(3464-3476)Annotations(1048-1048)api/client/go/client.gen.go (2)
EventDeliveryAttemptResponse(3177-3189)Annotations(962-962)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/notificationeventdeliverystatus/where.go (5)
EventID(85-87)ChannelID(90-92)Reason(95-97)CreatedAt(75-77)UpdatedAt(80-82)
openmeter/notification/service/rule.go (5)
openmeter/notification/rule.go (2)
CreateRuleInput(168-185)UpdateRuleInput(228-245)openmeter/notification/service/service.go (1)
Service(20-29)openmeter/notification/channel.go (2)
ListChannelsInput(118-127)Channel(37-51)pkg/pagination/page.go (1)
Page(14-17)pkg/sortx/order.go (1)
Order(3-3)
test/notification/rule.go (2)
openmeter/ent/db/notificationchannel/where.go (1)
Namespace(70-72)openmeter/ent/db/notificationrule/where.go (1)
Namespace(70-72)
openmeter/notification/adapter/entitymapping.go (1)
openmeter/ent/db/notificationeventdeliverystatus/where.go (1)
NextAttemptAt(100-102)
openmeter/notification/webhook/handler.go (3)
openmeter/notification/webhook/secret/secret.go (1)
ValidateSigningSecret(32-45)openmeter/notification/webhook/errors.go (1)
NewValidationError(37-43)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusState(25-25)EventDeliveryAttempt(56-60)
openmeter/notification/eventhandler/reconcile.go (6)
openmeter/notification/eventhandler/handler.go (1)
Handler(54-71)openmeter/notification/event.go (2)
Event(51-67)ListEventsInput(74-97)openmeter/notification/rule.go (1)
Rule(17-33)openmeter/notification/channel.go (3)
Channel(37-51)ChannelType(19-19)ChannelTypeWebhook(14-14)openmeter/notification/deliverystatus.go (3)
EventDeliveryStatusState(25-25)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateSending(16-16)pkg/clock/clock.go (1)
Now(14-21)
cmd/jobs/internal/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
openmeter/notification/eventhandler/webhook.go (9)
openmeter/notification/eventhandler/handler.go (2)
New(113-152)Handler(54-71)openmeter/notification/webhook/handler.go (8)
Handler(288-292)MessageDeliveryStatus(180-185)Message(187-204)GetMessageInput(242-249)ExpandParams(235-240)SendMessageInput(208-215)EventType(261-268)Payload(178-178)openmeter/notification/event.go (2)
Event(51-67)EventType(31-31)openmeter/notification/webhook/errors.go (4)
IsNotFoundError(69-71)IsMessageNotReadyError(134-136)IsUnrecoverableError(150-152)RetryableError(79-83)pkg/clock/clock.go (1)
Now(14-21)openmeter/notification/deliverystatus.go (5)
UpdateEventDeliveryStatusInput(180-193)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusStateSuccess(14-14)pkg/framework/tracex/tracex.go (2)
StartWithNoValue(75-82)Start(32-39)openmeter/notification/httpdriver/mapping.go (4)
FromEventAsBalanceThresholdPayload(581-599)FromEventAsEntitlementResetPayload(601-618)FromEventAsInvoiceCreatedPayload(620-636)FromEventAsInvoiceUpdatedPayload(638-654)openmeter/notification/eventpayload.go (1)
AsRawPayload(14-26)
cmd/server/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
openmeter/notification/webhook/svix/internal/error.go (1)
openmeter/notification/webhook/errors.go (4)
NewUnrecoverableError(154-162)NewValidationError(37-43)NewNotFoundError(61-67)NewRetryableError(103-116)
openmeter/notification/adapter/event.go (3)
openmeter/ent/db/notificationevent/where.go (1)
HasDeliveryStatusesWith(376-385)openmeter/ent/db/notificationeventdeliverystatus/where.go (3)
StateNotIn(411-417)NextAttemptAtIsNil(535-537)NextAttemptAtLTE(530-532)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusStateSuccess(14-14)EventDeliveryStatusStateFailed(15-15)
openmeter/notification/webhook/svix/message.go (3)
openmeter/notification/webhook/handler.go (5)
EventType(261-268)Message(187-204)Payload(178-178)MessageDeliveryStatus(180-185)GetMessageInput(242-249)openmeter/notification/webhook/svix/internal/error.go (1)
WrapSvixError(68-110)openmeter/notification/deliverystatus.go (6)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusState(25-25)EventDeliveryStatusStateSuccess(14-14)
cmd/notification-service/wire_gen.go (1)
app/common/notification.go (1)
NewNotificationEventHandler(63-96)
⏰ 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: Artifacts / Benthos Collector Container image
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (34)
openmeter/notification/rule.go (2)
212-214: LGTM! Good validation addition.This ensures rules have at least one channel, which makes sense given the channel validation happening downstream.
274-276: LGTM! Consistent validation.Mirrors the CreateRuleInput validation, ensuring both create and update paths enforce the channel requirement.
test/notification/rule.go (2)
171-171: LGTM! Consistent test updates.All call sites properly updated to pass the namespace parameter.
Also applies to: 176-176, 214-214, 264-264, 281-281
239-239: LGTM! Necessary test update.This aligns with the new requirement that UpdateRuleInput must include channels.
openmeter/notification/service/rule.go (3)
60-76: LGTM! Solid validation logic.The approach of building a channel ID map and checking membership is clean and efficient. Once the pagination bug is fixed, this will correctly validate channel existence.
96-99: LGTM! Good validation placement.Running channel validation before rule creation makes sense, and the error wrapping is consistent with the feature validation above.
214-220: LGTM! Clear separation of validation concerns.The distinction between rule type validation and channel validation is clean, with specific error messages for each.
app/config/config_test.go (1)
384-386: LGTM! New timeout fields look good.The new notification configuration fields (ReconcileInterval, SendingTimeout, PendingTimeout) are properly wired into the test expectations. The values are sensible for a delivery retry system.
openmeter/notification/webhook/svix/internal/error.go (2)
38-51: Nice error classification improvements!The expanded error mapping looks solid:
- Treating 400 and 409 as unrecoverable validation errors makes sense
- Handling 429 and 5xx as retryable with backoff is the right pattern
- The use of
RetryAfterfield aligns with standard retry semanticsOne quick note: the 422 case on line 40 is now treated differently than before (as unrecoverable validation). Just want to confirm this is intentional and aligns with how Svix uses 422.
86-93: Cleaner validation error formatting.The new format
[location=.. type=..]: messageis more structured and parseable. Good improvement for debugging!openmeter/notification/adapter/event.go (1)
51-59: Solid retry eligibility filter!The logic for filtering events by
NextAttemptBeforelooks correct:
- Excludes terminal states (Success/Failed) ✓
- Handles both unscheduled attempts (NextAttemptAt is nil) and scheduled attempts that are due ✓
- Proper UTC conversion ✓
This should efficiently find events ready for retry without pulling in already-completed deliveries.
openmeter/notification/webhook/noop/noop.go (1)
73-77: LGTM! Consistent noop implementation.The new
GetMessagemethod follows the same pattern as the other noop handlers in this file. Logging + returningErrNotImplementedis exactly what we'd expect here.openmeter/notification/adapter/entitymapping.go (1)
136-143: Clean mapping for new delivery fields.The UTC pointer handling for
NextAttemptis solid—proper nil check and conversion. Direct assignment ofAttemptslooks good too. The use oflohelpers keeps this readable.cmd/notification-service/wire_gen.go (1)
152-152: Generated code reflects wiring changes correctly.The updated call to
NewNotificationEventHandlernow passesnotificationConfigurationas the first parameter, aligning with the new signature inapp/common/notification.go. This enables the handler to use the new timeout configurations (ReconcileInterval, SendingTimeout, PendingTimeout).Since this is Wire-generated code, no action needed here—just confirming it matches the expected signature.
openmeter/notification/eventhandler/dispatch.go (1)
13-18: Nice refactor to unified reconciliation!Simplifying the dispatch logic to delegate to
reconcileEventis a solid move. This should make the event handling flow more consistent and easier to maintain. The goroutine, timeout, and tracing setup all look good.cmd/jobs/internal/wire_gen.go (1)
406-406: Generated code updated correctly.Same as the notification-service wiring—the call to
NewNotificationEventHandlernow includesnotificationConfigurationas the first parameter. This is expected Wire-generated code reflecting the signature change.openmeter/notification/webhook/svix/message.go (4)
49-59: Nice improvement to span attribute naming!The hierarchical dot-notation (e.g.,
svix.event.idinstead ofsvix.event_id) is clearer and more consistent. Moving theidempotency_keyto the AddEvent call keeps it scoped to the specific operation without cluttering all child spans.
248-305: Clean implementation of conditional expansion!The flexible ID/EventID coalescing (line 250) and the conditional expansion of payload and delivery status (lines 269-285) make the API both convenient and efficient. The annotations population ensures full context is available for downstream processing.
307-320: LGTM!The helper cleanly maps Svix message statuses to internal delivery states. Returning an empty string for unknown statuses allows callers to detect and handle unmapped cases appropriately.
81-95: Good traceability with annotations.Populating annotations immediately on message creation (lines 89-94) ensures the message is traceable throughout its lifecycle. Keeping
DeliveryStatusesnil initially makes sense since no delivery attempts have occurred yet.openmeter/notification/deliverystatus.go (4)
49-60: Well-structured delivery attempt types.The
EventDeliveryAttemptandEventDeliveryAttemptResponsestructs capture all the necessary details for tracking delivery attempts. Usingtime.Durationinstead of milliseconds as an integer is cleaner and more type-safe.
62-75: LGTM!The sorting function correctly implements descending order by timestamp using the idiomatic
slices.SortFunc. The logic is clear and handles equal timestamps properly.
180-193: Good extension of the input struct.Adding
NextAttemptandAttemptstoUpdateEventDeliveryStatusInputaligns with the new delivery status fields and enables the reconciliation logic to track retry schedules and attempt history.
77-93: No action needed—the JSON tag removal is safe.The code has a proper mapping layer:
FromEvent()explicitly transforms the domainnotification.EventDeliveryStatusinto a separateapi.NotificationEventDeliveryStatusstruct (line ~160 in mapping.go). Each field is manually mapped, so removing JSON tags from the domain model doesn't affect the API response contract, which is controlled by theapi.*types.Likely an incorrect or invalid review comment.
openmeter/notification/eventhandler/webhook.go (6)
20-25: Clear error categorization.The error constants nicely distinguish between user-side recipient failures and system-side issues, and they explicitly state retry behavior. This makes the reconciliation logic's intent clearer.
27-60: Good defensive setup and the past review concern is addressed.The nil check, early return for no active statuses, and filtering logic (lines 29-53) are solid. The error handling on line 58 now correctly returns only on real errors (not
NotFoundError), which allows the reconciliation to proceed to sending the message when it doesn't exist yet—fixing the issue raised in the past review comment.
79-97: Double-check the pending timeout calculation.On line 86, you're calculating the timeout as
nextAttempt.Sub(status.CreatedAt), which measures the total time elapsed since the status was created. This works ifnextAttemptis only set once, but if it's updated multiple times during retries, this calculation might not accurately represent how long the status has been pending. Consider whether you want to measure "time since creation" or "time since last retry attempt."If
nextAttemptis updated on each retry, the timeout check may not behave as intended. Please verify the intended semantics.
278-306: Solid error aggregation and completion.The handling of terminal states (lines 278-282) and the error aggregation pattern (lines 297-302) ensure that all delivery statuses are processed even if some updates fail. This resilience is important for the reconciliation loop.
354-442: Defensive message sending with proper error handling.The
sendWebhookMessagefunction wisely checks if the message already exists (lines 407-420) before attempting to send, which avoids duplicate sends. The error handling correctly distinguishes between "not found" (expected) and real errors (line 414).
444-483: Clean event-to-payload serialization.The switch statement handles all known event types and delegates to the appropriate mapping functions from
httpdriver. The explicit error for unknown event types (line 472) ensures new event types won't silently fail.openmeter/notification/webhook/handler.go (4)
30-30: Nice standardization on models.Validator!Switching all input structs to implement
models.Validatorcreates consistency across the codebase and makes it easier to handle validation uniformly.Also applies to: 50-50, 87-87, 130-130, 154-154, 206-206
178-204: Well-designed message structure with optional expansion.The
Messagestruct now supports optional expansion of payload and delivery statuses through pointer fields (lines 200, 203), which keeps the default response lean while allowing clients to request additional details when needed. ThePayloadtype alias (line 178) also makes the API cleaner.
206-249: Good API design for message retrieval and expansion.The
ExpandParamsstruct (lines 235-240) provides a clean way to control what data is included in message responses. Supporting lookup by eitherIDorEventIDinGetMessageInput(lines 242-249) gives clients flexibility.
283-286: Interface extension looks good.Adding
GetMessageto theMessageHandlerinterface completes the message lifecycle API and enables the reconciliation logic to retrieve message status from the webhook provider.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openmeter/notification/httpdriver/mapping.go (2)
547-566: Consider side effects: the input slice is modified in place.The function calls
SortEventDeliveryAttemptsInDescOrder(attempts), which sorts the input slice directly. This could surprise callers who don't expect their slice to be reordered. Consider making a copy before sorting to avoid this side effect:func AsEventDeliveryAttempts(attempts []notification.EventDeliveryAttempt) []api.NotificationEventDeliveryAttempt { result := make([]api.NotificationEventDeliveryAttempt, 0, len(attempts)) - notification.SortEventDeliveryAttemptsInDescOrder(attempts) + attemptsCopy := slices.Clone(attempts) + notification.SortEventDeliveryAttemptsInDescOrder(attemptsCopy) - for _, attempt := range attempts { + for _, attempt := range attemptsCopy { result = append(result, api.NotificationEventDeliveryAttempt{
558-558: Minor: Duration conversion tointmight overflow.The conversion from
int64(returned byMilliseconds()) tointcould theoretically overflow on 32-bit systems for very long durations (though unlikely in practice for delivery attempts). If you want to be extra safe, consider usingint64in the API type or adding bounds checking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openmeter/notification/deliverystatus.go(3 hunks)openmeter/notification/httpdriver/mapping.go(2 hunks)openmeter/notification/service/rule.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/notification/service/rule.goopenmeter/notification/deliverystatus.goopenmeter/notification/httpdriver/mapping.go
🧬 Code graph analysis (3)
openmeter/notification/service/rule.go (5)
openmeter/notification/rule.go (2)
CreateRuleInput(168-185)UpdateRuleInput(228-245)openmeter/notification/service/service.go (1)
Service(20-29)openmeter/notification/channel.go (2)
ListChannelsInput(118-127)Channel(37-51)pkg/pagination/page.go (1)
Page(14-17)pkg/sortx/order.go (1)
Order(3-3)
openmeter/notification/deliverystatus.go (5)
api/client/javascript/src/client/schemas.ts (2)
EventDeliveryAttemptResponse(11918-11919)Annotations(11692-11692)api/api.gen.go (2)
EventDeliveryAttemptResponse(3464-3476)Annotations(1048-1048)api/client/go/client.gen.go (2)
EventDeliveryAttemptResponse(3177-3189)Annotations(962-962)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/notificationeventdeliverystatus/where.go (5)
EventID(85-87)ChannelID(90-92)Reason(95-97)CreatedAt(75-77)UpdatedAt(80-82)
openmeter/notification/httpdriver/mapping.go (2)
api/api.gen.go (3)
NotificationEventDeliveryStatusState(5426-5426)NotificationEventDeliveryAttempt(5390-5399)EventDeliveryAttemptResponse(3464-3476)openmeter/notification/deliverystatus.go (3)
EventDeliveryAttempt(56-60)SortEventDeliveryAttemptsInDescOrder(63-67)EventDeliveryAttemptResponse(49-54)
⏰ 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: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
openmeter/notification/service/rule.go (4)
45-55: Nice fix on the pagination! 🎉Great job addressing the previous review - the pagination parameters are now correct (
PageSize: len(channels), PageNumber: 1). This will properly fetch all requested channels in a single page.One tiny optimization: the
OrderByandOrderparameters don't really matter here since we're just checking existence. You could omit them to simplify the call, but honestly it's not a big deal.
96-99: Solid validation addition!Good call adding channel validation before rule creation. This prevents creating rules with non-existent channels, which would break at runtime.
214-215: More specific error message - nice improvement!Changing from a generic error to "invalid rule type" makes it clearer what went wrong when someone tries to change a rule's type.
217-220: Double database call for channels - is this intentional?I noticed that
UpdateRulenow callsListChannelstwice:
- Here via
ValidateRuleChannels(fetches all channels inparams.Channelsto validate they exist)- Later at lines 245-255 (fetches channels in the diff to update webhooks)
The two calls fetch different channel sets and serve different purposes, so I understand why they're separate. The validation call ensures we fail fast before making any changes, which is good. The second call is also conditional (only runs if channels changed).
That said, since this is in a critical path (per your coding guidelines), just wanted to verify this is the intended approach and the performance impact is acceptable.
openmeter/notification/httpdriver/mapping.go (1)
466-474: Nice mapping of the new delivery status fields!The mapping logic looks solid. All the new fields (Annotations, Attempts, NextAttempt, Reason) are properly populated from the delivery status.
openmeter/notification/deliverystatus.go (3)
49-67: New delivery attempt types look good!The
EventDeliveryAttemptResponseandEventDeliveryAttemptstructs are well-structured with appropriate JSON tags. The sorting function correctly usestime.Compareand reverses the order for descending sort.
181-184: New fields added cleanly to update input.The
NextAttemptandAttemptsfields are added with clear comments. Looks good!
73-84: No issues found. The JSON tag removal is safe.Here's why: EventDeliveryStatus is never directly JSON marshaled. The codebase uses an explicit mapping pattern in
openmeter/notification/httpdriver/mapping.go(lines 463–475) where it field-by-field constructs the API response type (api.NotificationEventDeliveryStatus) from the domain model. The API response type has its own JSON tags, so consumers see the correct field names.The Event struct does include
DeliveryStatus []EventDeliveryStatus, but when Event is serialized for API responses, it goes through httpdriver mapping—not direct marshaling.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
openmeter/notification/eventhandler/webhook.go (3)
232-248: Reconsider the "stuck in SENDING" recovery logic.When the message is not found while in SENDING state, the code waits an hour before moving back to PENDING. A few things feel off here:
- If we're in SENDING but the provider has no record of the message, that's a pretty serious inconsistency—not just a stuck state
- The hardcoded 1-hour wait doesn't align with the configurable
sendingTimeout- This looks like a defensive workaround that might paper over an upstream bug
Would it make more sense to either:
- Treat this as an unrecoverable error (the message mysteriously vanished), or
- Move immediately back to PENDING to retry sending?
254-265: Consider treating missing channel status as transient rather than unrecoverable.Right now, if the provider returns a message without a matching
channelIDin its delivery statuses, we immediately mark it as FAILED with an unrecoverable error. But this could just be a timing issue—the provider might not have synced that channel's status yet.Failing permanently on what might be a transient condition feels too aggressive. Maybe log a warning and schedule a retry instead?
371-384: Remove redundant GetMessage call.The caller (
reconcileWebhookEvent) already checked whether the message exists before callingsendWebhookMessage. This extraGetMessagecall just duplicates that work.- msg, err := h.webhook.GetMessage(ctx, webhook.GetMessageInput{ - Namespace: event.Namespace, - EventID: event.ID, - Expand: webhook.ExpandParams{ - DeliveryStatus: true, - }, - }) - if err != nil && !webhook.IsNotFoundError(err) { - return nil, fmt.Errorf("failed to get message for webhook channel: %w", err) - } - - if msg != nil { - return msg, nil - } - payload, err := eventAsPayload(event)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openmeter/notification/eventhandler/webhook.go(1 hunks)openmeter/notification/webhook/svix/message.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/notification/eventhandler/webhook.goopenmeter/notification/webhook/svix/message.go
🧠 Learnings (1)
📚 Learning: 2025-11-07T11:28:40.399Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/webhook/svix/webhook.go:253-257
Timestamp: 2025-11-07T11:28:40.399Z
Learning: In the openmeter notification system (specifically in openmeter/notification/webhook/svix/webhook.go), event type filtering for webhooks is not allowed or supported. Only channel-based filtering is used.
Applied to files:
openmeter/notification/eventhandler/webhook.go
🧬 Code graph analysis (2)
openmeter/notification/eventhandler/webhook.go (10)
openmeter/notification/eventhandler/handler.go (2)
New(113-152)Handler(54-71)openmeter/notification/webhook/handler.go (8)
Handler(288-292)MessageDeliveryStatus(180-185)Message(187-204)GetMessageInput(242-249)ExpandParams(235-240)SendMessageInput(208-215)EventType(261-268)Payload(178-178)openmeter/notification/event.go (2)
Event(51-67)EventType(31-31)openmeter/notification/webhook/errors.go (4)
IsNotFoundError(69-71)IsMessageNotReadyError(134-136)IsUnrecoverableError(150-152)RetryableError(79-83)pkg/clock/clock.go (1)
Now(14-21)openmeter/notification/deliverystatus.go (5)
UpdateEventDeliveryStatusInput(172-185)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusStateSuccess(14-14)openmeter/notification/webhook/svix/svix.go (1)
Error(28-28)pkg/framework/tracex/tracex.go (2)
StartWithNoValue(75-82)Start(32-39)openmeter/notification/httpdriver/mapping.go (4)
FromEventAsBalanceThresholdPayload(583-601)FromEventAsEntitlementResetPayload(603-620)FromEventAsInvoiceCreatedPayload(622-638)FromEventAsInvoiceUpdatedPayload(640-656)openmeter/notification/eventpayload.go (1)
AsRawPayload(14-26)
openmeter/notification/webhook/svix/message.go (4)
openmeter/notification/webhook/handler.go (5)
EventType(261-268)Message(187-204)Payload(178-178)MessageDeliveryStatus(180-185)GetMessageInput(242-249)openmeter/notification/webhook/svix/internal/error.go (1)
WrapSvixError(68-110)openmeter/notification/webhook/svix/annotations.go (4)
AnnotationMessageID(4-4)AnnotationMessageEventID(5-5)AnnotationEventType(6-6)AnnotationApplicationUID(8-8)openmeter/notification/deliverystatus.go (6)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusState(25-25)EventDeliveryStatusStateSuccess(14-14)
⏰ 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). (8)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
openmeter/notification/webhook/svix/message.go (4)
61-87: Nice fix on the payload handling!Good call using
¶ms.Payloadfrom the input instead of&out.Payloadfrom the Svix response. SinceWithContent: falseis set to optimize the API call, the response payload would be empty anyway. This addresses the concern from the previous review.
173-173: Verify the sleep timing is appropriate for expected load.The 100ms sleep between pagination requests is rate-limit friendly, but since this is in a message processing path, it could add latency when dealing with many delivery attempts. Is this timing appropriate for your expected volume of endpoints and attempts per message?
Consider making the sleep duration configurable or documenting the expected latency characteristics.
206-210: Nice handling of the transient failure state.Good catch treating
FAILEDwith a non-nilNextAttemptasSENDING. The comment clearly explains why Svix might returnFAILeven while still attempting delivery. This makes the status more accurate for end users.
269-294: Clean conditional expansion logic.The implementation properly respects the
Expandparameters for both payload and delivery statuses. SettingWithContentbased onparams.Expand.Payloadand conditionally fetching delivery statuses keeps the API efficient while providing flexibility. Nice work!openmeter/notification/eventhandler/webhook.go (1)
211-229: Verify the sending timeout calculation logic.Same concern as with the PENDING timeout: using
nextAttempt.Sub(status.CreatedAt)might not accurately reflect how long the status has been in SENDING state. Consider verifying whetherclock.Now().Sub(status.CreatedAt)is the intended check.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openmeter/notification/webhook/svix/message.go (1)
261-261: Span attribute doesn't reflect the actual message ID used.You're setting the span attribute to
params.ID, but the actual message lookup on line 250 usesmsgID(which isCoalesceOrEmpty(params.ID, params.EventID)). Ifparams.IDis empty andparams.EventIDis used, the trace won't show what was actually queried.Apply this diff to use the coalesced value:
msgID := lo.CoalesceOrEmpty(params.ID, params.EventID) if msgID == "" { return nil, webhook.NewValidationError( errors.New("either svix message id or event id must be provided"), ) } span := trace.SpanFromContext(ctx) spanAttrs := []attribute.KeyValue{ - attribute.String(AnnotationMessageEventID, params.ID), + attribute.String(AnnotationMessageEventID, msgID), attribute.String(AnnotationApplicationUID, params.Namespace), }
🧹 Nitpick comments (2)
openmeter/notification/webhook/svix/message.go (2)
108-109: Consider more specific error messages.The generic "invalid params" message doesn't tell the caller which parameter is missing. While it works, something like
"namespace and eventID are required"would be more helpful for debugging.- return nil, fmt.Errorf("invalid params") + return nil, fmt.Errorf("namespace and eventID are required")
173-173: Consider making rate-limit sleep duration configurable.The hardcoded 100ms sleeps work, but making this configurable (or at least a package constant) would make it easier to tune for different rate limits or testing scenarios. Not critical for now, but worth considering if you need to adjust these values later.
You could add a constant at the package level:
const ( ListAttemptLimit uint64 = 25 RateLimitSleepDuration = 100 * time.Millisecond )Then use
time.Sleep(RateLimitSleepDuration)instead of the hardcoded values.Also applies to: 234-234
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openmeter/notification/eventhandler/webhook.go(1 hunks)openmeter/notification/webhook/svix/annotations.go(1 hunks)openmeter/notification/webhook/svix/application.go(1 hunks)openmeter/notification/webhook/svix/event.go(1 hunks)openmeter/notification/webhook/svix/message.go(2 hunks)openmeter/notification/webhook/svix/webhook.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/notification/webhook/svix/webhook.goopenmeter/notification/webhook/svix/event.goopenmeter/notification/eventhandler/webhook.goopenmeter/notification/webhook/svix/message.goopenmeter/notification/webhook/svix/application.goopenmeter/notification/webhook/svix/annotations.go
🧠 Learnings (2)
📚 Learning: 2025-11-07T12:29:09.025Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/eventhandler/webhook.go:391-396
Timestamp: 2025-11-07T12:29:09.025Z
Learning: In the OpenMeter notification system (specifically in openmeter/notification/eventhandler/webhook.go), webhook channels are equivalent to notification rules. When sending webhook messages, the rule ID (event.Rule.ID) should be used as the channel identifier in the Channels field of SendMessageInput.
Applied to files:
openmeter/notification/webhook/svix/webhook.goopenmeter/notification/eventhandler/webhook.goopenmeter/notification/webhook/svix/message.go
📚 Learning: 2025-11-07T11:28:40.399Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/webhook/svix/webhook.go:253-257
Timestamp: 2025-11-07T11:28:40.399Z
Learning: In the openmeter notification system (specifically in openmeter/notification/webhook/svix/webhook.go), event type filtering for webhooks is not allowed or supported. Only channel-based filtering is used.
Applied to files:
openmeter/notification/webhook/svix/webhook.goopenmeter/notification/webhook/svix/event.goopenmeter/notification/eventhandler/webhook.go
🧬 Code graph analysis (5)
openmeter/notification/webhook/svix/webhook.go (3)
openmeter/notification/webhook/secret/secret.go (1)
NewSigningSecretWithDefaultSize(24-26)openmeter/notification/webhook/svix/annotations.go (3)
AnnotationApplicationUID(10-10)AnnotationEndpointUID(13-13)AnnotationEndpointURL(14-14)openmeter/notification/webhook/svix/svix.go (1)
NullChannel(25-25)
openmeter/notification/webhook/svix/event.go (1)
openmeter/notification/webhook/svix/annotations.go (2)
AnnotationEventType(7-7)AnnotationEventGroupName(8-8)
openmeter/notification/eventhandler/webhook.go (9)
openmeter/notification/eventhandler/handler.go (2)
New(113-152)Handler(54-71)openmeter/notification/webhook/handler.go (8)
Handler(288-292)MessageDeliveryStatus(180-185)Message(187-204)GetMessageInput(242-249)ExpandParams(235-240)SendMessageInput(208-215)EventType(261-268)Payload(178-178)openmeter/notification/event.go (2)
Event(51-67)EventType(31-31)openmeter/notification/webhook/errors.go (4)
IsNotFoundError(69-71)IsMessageNotReadyError(134-136)IsUnrecoverableError(150-152)RetryableError(79-83)pkg/clock/clock.go (1)
Now(14-21)openmeter/notification/deliverystatus.go (5)
UpdateEventDeliveryStatusInput(172-185)EventDeliveryStatusStatePending(17-17)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusStateSuccess(14-14)pkg/framework/tracex/tracex.go (2)
StartWithNoValue(75-82)Start(32-39)openmeter/notification/httpdriver/mapping.go (4)
FromEventAsBalanceThresholdPayload(583-601)FromEventAsEntitlementResetPayload(603-620)FromEventAsInvoiceCreatedPayload(622-638)FromEventAsInvoiceUpdatedPayload(640-656)openmeter/notification/eventpayload.go (1)
AsRawPayload(14-26)
openmeter/notification/webhook/svix/message.go (4)
openmeter/notification/webhook/svix/annotations.go (5)
AnnotationMessageEventID(5-5)AnnotationEventType(7-7)AnnotationApplicationUID(10-10)AnnotationMessageID(4-4)AnnotationMessageAttemptsCount(6-6)openmeter/notification/webhook/handler.go (5)
EventType(261-268)Message(187-204)Payload(178-178)MessageDeliveryStatus(180-185)GetMessageInput(242-249)openmeter/notification/webhook/svix/internal/error.go (1)
WrapSvixError(68-110)openmeter/notification/deliverystatus.go (6)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusState(25-25)EventDeliveryStatusStateSuccess(14-14)
openmeter/notification/webhook/svix/application.go (1)
openmeter/notification/webhook/svix/annotations.go (2)
AnnotationApplicationName(11-11)AnnotationApplicationUID(10-10)
⏰ 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: Migration Checks
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
openmeter/notification/webhook/svix/message.go (6)
50-59: Nice tracing additions!The span attributes and events add good observability to the message sending flow. The attributes are well-chosen and will help with debugging.
87-87: Payload assignment looks correct.You're returning
¶ms.Payload(the input payload), not&out.Payload(the Svix response). SinceWithContent: falseis used for optimization, the Svix response won't include payload content, but that's fine—you're echoing back what was sent. This correctly preserves the caller's payload in the response.
208-210: Smart handling of transient failed state!Good catch that Svix returns FAIL even when it's still attempting delivery (if NextAttempt is set). Mapping that to SENDING makes the state more accurate for callers.
270-270: Payload expansion logic looks good!The conditional
WithContentbased onparams.Expand.Payload(line 270) and the corresponding payload assignment (line 293) are properly aligned. Nice job keeping the optimization consistent with what's actually requested.Also applies to: 293-293
278-285: Clean conditional delivery status fetching.The expansion logic for delivery statuses is clear and well-structured. The error handling is appropriate too.
307-320: Clean status mapping.The
deliveryStateFromSvixMessageStatushelper is straightforward and handles all the expected Svix message statuses. Returning an empty string for unknown statuses allows callers to detect and handle unexpected values, which is good.
| return nil, fmt.Errorf("failed to map delivery attempt state [svix.message.status=%d svix.message.status_test=%s]: %w", | ||
| attempt.Status, attempt.StatusText, err) |
There was a problem hiding this comment.
Fix error reference in error messages.
Lines 141-142 and 213-214 reference %w, err in the error format string, but err is nil at those points (it was already checked and wrapped on the previous lines 133 and 199). Remove the %w, err portion or these will panic or format incorrectly.
Apply this diff:
if state == "" {
- return nil, fmt.Errorf("failed to map delivery attempt state [svix.message.status=%d svix.message.status_test=%s]: %w",
- attempt.Status, attempt.StatusText, err)
+ return nil, fmt.Errorf("failed to map delivery attempt state [svix.message.status=%d svix.message.status_text=%s]",
+ attempt.Status, attempt.StatusText)
}And similarly:
if state == "" {
- return nil, fmt.Errorf("failed to map delivery status [svix.message.status=%d svix.message.status_test=%s]: %w",
- dest.Status, dest.StatusText, err)
+ return nil, fmt.Errorf("failed to map delivery status [svix.message.status=%d svix.message.status_text=%s]",
+ dest.Status, dest.StatusText)
}Note: Also fixed the typo "status_test" → "status_text" in both error messages.
Also applies to: 213-214
🤖 Prompt for AI Agents
In openmeter/notification/webhook/svix/message.go around lines 141-142 and
213-214, the fmt.Errorf calls include a "%w" and "err" even though err is nil at
those points (it was already handled/wrapped earlier), which will panic or
misformat; remove the "%w, err" portion from both error format calls and update
the field name from "status_test" to "status_text" in the message format so the
error strings correctly include only attempt.Status and attempt.StatusText
without attempting to wrap a non-existent error.
There was a problem hiding this comment.
Will fix it in a separate PR.
There was a problem hiding this comment.
@chrisgacsal, sounds good! Would you like me to create a GitHub issue to track this fix for the follow-up PR? I can include the details about the error formatting and the typo correction.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
openmeter/notification/webhook/svix/message.go (3)
141-142: Fix error formatting -erris nil at this point.On line 141-142, you're trying to wrap
errwith%w, buterris already nil here (it was checked and wrapped on line 133-134). This will either panic or produce confusing output. Also noticed a small typo: "status_test" should be "status_text".Apply this diff:
if state == "" { - return nil, fmt.Errorf("failed to map delivery attempt state [svix.message.status=%d svix.message.status_test=%s]: %w", - attempt.Status, attempt.StatusText, err) + return nil, fmt.Errorf("failed to map delivery attempt state [svix.message.status=%d svix.message.status_text=%s]", + attempt.Status, attempt.StatusText) }
213-214: Same error formatting issue here.Similar to the issue on lines 141-142, you're using
%w, errwhenerris nil (already handled on line 199). Also has the "status_test" typo.Apply this diff:
if state == "" { - return nil, fmt.Errorf("failed to map delivery status [svix.message.status=%d svix.message.status_test=%s]: %w", - dest.Status, dest.StatusText, err) + return nil, fmt.Errorf("failed to map delivery status [svix.message.status=%d svix.message.status_text=%s]", + dest.Status, dest.StatusText) }
260-263: Span attributes should reflect the actual message ID used.On lines 261-262, you're setting span attributes to
params.IDandparams.EventIDseparately, but line 250 coalesces them intomsgIDwhich is what's actually used in the API call (line 270). The trace would be more accurate if it showed the ID that was actually queried.Consider this adjustment:
fn := func(ctx context.Context) (*webhook.Message, error) { msgID := lo.CoalesceOrEmpty(params.ID, params.EventID) if msgID == "" { return nil, webhook.NewValidationError( errors.New("either svix message id or event id must be provided"), ) } span := trace.SpanFromContext(ctx) spanAttrs := []attribute.KeyValue{ - attribute.String(AnnotationMessageID, params.ID), - attribute.String(AnnotationMessageEventID, params.EventID), + attribute.String(AnnotationMessageID, msgID), attribute.String(AnnotationApplicationUID, params.Namespace), }Alternatively, if you want to keep both for debugging, add the coalesced value as a separate attribute.
🧹 Nitpick comments (2)
openmeter/notification/webhook/svix/message.go (2)
173-173: Consider extracting the rate-limit sleep duration to a constant.The hardcoded
100 * time.Millisecondsleep appears both here and on line 234. Extracting it to a named constant (e.g.,rateLimitSleepDuration) would make it easier to tune and maintain.For example, near your other constants:
const ( ListAttemptLimit uint64 = 25 rateLimitSleepDuration time.Duration = 100 * time.Millisecond )Then use it on lines 173 and 234:
- time.Sleep(100 * time.Millisecond) + time.Sleep(rateLimitSleepDuration)
294-294: Consider conditional payload assignment based on expansion.When
params.Expand.Payloadis false (line 271), the Svix response won't populatemsgOut.Payloadwith meaningful content, but you're still assigning&msgOut.Payloadon line 294. While this isn't breaking, it might be clearer to conditionally set it tonilwhen not expanded, matching the semantic intent of thewebhook.Message.Payloadfield documentation ("stores the message payload if it was requested").Something like:
+ var payload *webhook.Payload + if params.Expand.Payload { + payload = &msgOut.Payload + } + return &webhook.Message{ Namespace: params.Namespace, ID: msgOut.Id, EventID: lo.FromPtr(msgOut.EventId), EventType: msgOut.EventType, Channels: msgOut.Channels, - Payload: &msgOut.Payload, + Payload: payload, DeliveryStatuses: lo.ToPtr(statuses),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openmeter/notification/webhook/svix/message.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/notification/webhook/svix/message.go
🧠 Learnings (1)
📚 Learning: 2025-11-07T12:29:09.025Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/eventhandler/webhook.go:391-396
Timestamp: 2025-11-07T12:29:09.025Z
Learning: In the OpenMeter notification system (specifically in openmeter/notification/eventhandler/webhook.go), webhook channels are equivalent to notification rules. When sending webhook messages, the rule ID (event.Rule.ID) should be used as the channel identifier in the Channels field of SendMessageInput.
Applied to files:
openmeter/notification/webhook/svix/message.go
🧬 Code graph analysis (1)
openmeter/notification/webhook/svix/message.go (6)
openmeter/notification/webhook/svix/annotations.go (5)
AnnotationMessageEventID(5-5)AnnotationEventType(7-7)AnnotationApplicationUID(10-10)AnnotationMessageID(4-4)AnnotationMessageAttemptsCount(6-6)openmeter/notification/webhook/handler.go (5)
EventType(261-268)Message(187-204)Payload(178-178)MessageDeliveryStatus(180-185)GetMessageInput(242-249)openmeter/notification/webhook/svix/internal/error.go (1)
WrapSvixError(68-110)openmeter/notification/webhook/svix/svix.go (2)
Error(28-28)New(86-121)pkg/framework/tracex/tracex.go (1)
Start(32-39)openmeter/notification/deliverystatus.go (6)
EventDeliveryAttempt(56-60)EventDeliveryAttemptResponse(49-54)EventDeliveryStatusStateFailed(15-15)EventDeliveryStatusStateSending(16-16)EventDeliveryStatusState(25-25)EventDeliveryStatusStateSuccess(14-14)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
openmeter/notification/webhook/svix/message.go (2)
24-99: SendMessage implementation looks solid!The optimization to set
WithContent: false(line 63) makes sense since you're returning the input payload you already have (¶ms.Payloadon line 87), avoiding unnecessary data transfer from Svix. The new annotations structure (lines 89-94) correctly tracks the message metadata using the defined annotation keys.
308-321: Helper function looks good!The status mapping logic is clean and straightforward. Returning an empty string for unknown statuses is a reasonable sentinel value, and the callers (lines 140 and 212) correctly check for it.
3 participants
Overview
Summary by CodeRabbit
New Features
Bug Fixes
Improvements