refactor: improve event reconcile

[chrisgacsal]

Overview

Improve event reconciliation by removing unnecessary branching in reconcile logic.

Summary by CodeRabbit

• Bug Fixes
  • Improved webhook event reconciliation error handling and recovery logic for better delivery reliability.
  • Enhanced error message formatting for clearer diagnostics.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR modifies notification event handling by updating an error message format in reconcile.go and restructuring the control flow and error handling logic in webhook.go's reconcileWebhookEvent function to prioritize not-found recovery and simplify downstream state processing.

Changes

Cohort / File(s)	Summary
Error message format update openmeter/notification/eventhandler/reconcile.go	Updated reconciliation error message to reference notification.event.id instead of event.id in the formatting string.
Webhook reconciliation control flow refactoring openmeter/notification/eventhandler/webhook.go	Restructured error handling in reconcileWebhookEvent by adding an early not-found check that triggers webhook resending, consolidated multiple error-specific branches within the Pending path, and simplified error accumulation to directly return errors.Join() without intermediate logging. Removed span.RecordError call in unhandled state path.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

• webhook.go: Trace through the restructured control flow paths, particularly the early not-found check and how it gates message handling and status updates
• webhook.go: Verify error accumulation and return logic changes don't inadvertently lose error context
• webhook.go: Confirm removal of logging and span.RecordError calls doesn't impact observability needs
• reconcile.go: Ensure the error message key change is intentional and doesn't affect error message parsing or alerting downstream

Possibly related PRs

• refactor(notification): error handling #3008 — Modifies both reconcile.go and webhook event reconciliation logic in related handlers
• fix: notification channel state out-of-sync #3600 — Reworks control flow in reconcileWebhookEvent with additional channel/state/rule checks and error handling
• fix: delivery state sync #3586 — Restructures Pending→Sending control flow and provider-derived message/status handling in reconcileWebhookEvent

Suggested labels

kind/refactor

Suggested reviewers

• tothandras
• turip
• gergely-kurucz-konghq

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor: improve event reconcile' directly relates to the main changes—it describes the refactoring of event reconciliation logic in webhook.go, which is the primary focus of this PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/event-reconcile

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

openmeter/notification/eventhandler/webhook.go (1)

221-227: Let’s merge the webhook annotations here too

Since the sending branch already merges status.Annotations with msg.Annotations, doing the same in this pending-to-sending handoff keeps provider metadata (like delivery IDs) from getting dropped. Easy win for parity:

 					input = &notification.UpdateEventDeliveryStatusInput{
 						NamespacedID: status.NamespacedID,
 						State:        state,
-						Annotations:  status.Annotations,
+						Annotations:  status.Annotations.Merge(msg.Annotations),
 						NextAttempt:  nextAttempt,
 						Attempts:     attempts,
 					}

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ad0701 and 2fc43ef.

📒 Files selected for processing (2)

• openmeter/notification/eventhandler/reconcile.go (1 hunks)
• openmeter/notification/eventhandler/webhook.go (3 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/reconcile.go
• openmeter/notification/eventhandler/webhook.go

🧠 Learnings (2)

📚 Learning: 2025-11-07T12:29:09.087Z

Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/eventhandler/webhook.go:391-396
Timestamp: 2025-11-07T12:29:09.087Z
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/eventhandler/reconcile.go
• openmeter/notification/eventhandler/webhook.go

📚 Learning: 2025-11-07T11:28:40.430Z

Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3581
File: openmeter/notification/webhook/svix/webhook.go:253-257
Timestamp: 2025-11-07T11:28:40.430Z
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 (1)

openmeter/notification/eventhandler/webhook.go (4)

openmeter/notification/webhook/errors.go (1)

• IsNotFoundError (69-71)

openmeter/notification/deliverystatus.go (2)

• EventDeliveryStatusStateSending (16-16)
• UpdateEventDeliveryStatusInput (175-188)

pkg/models/id.go (1)

• NamespacedID (7-10)

pkg/models/annotation.go (1)

• Annotations (3-3)

⏰ 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: Analyze (go)
• GitHub Check: Artifacts / Benthos Collector Container image
• GitHub Check: Artifacts / Container image
• GitHub Check: Migration Checks
• GitHub Check: Test
• GitHub Check: Code Generators
• GitHub Check: Lint
• GitHub Check: Build
• GitHub Check: Repository Scan

🔇 Additional comments (1)

openmeter/notification/eventhandler/reconcile.go (1)

117-118: Logging key alignment looks good

Love the tweak to log under notification.event.id—makes the logs line up with the span attrs without any surprises.