Open
Conversation
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
from
ac1f4a1 to
5df3910
Compare
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
2 times, most recently
from
6a91c29 to
ed69e7f
Compare
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
from
1d59e19 to
2cd379e
Compare
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
from
2cd379e to
3b23a8f
Compare
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
WalkthroughAdds a watchdog feature: Notification CRD gains optional Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Notification Reconciler
participant DB as Database
participant Scheduler as Cron Scheduler
participant Cache as Watchdog Jobs Cache
participant Sender as Notification Sender
Reconciler->>DB: PersistNotificationFromCRD(obj with watchdog.schedule)
DB->>DB: Trim & parse cron schedule (cron.ParseStandard)
DB-->>Reconciler: persisted object (with WatchdogSchedule) or error
Reconciler->>Scheduler: SyncWatchdogJob(notificationID, schedule)
Scheduler->>Cache: Lookup existing job for notificationID
alt Job exists and schedule changed/cleared
Scheduler->>Scheduler: Unschedule & remove old job
end
alt Schedule non-nil
Scheduler->>Scheduler: Schedule new cron job
Scheduler->>Cache: Store job for notificationID
end
Note over Scheduler,Cache: At scheduled times...
Scheduler->>DB: Query notification record & GetNotificationStats(notificationID)
DB-->>Scheduler: statistics
Scheduler->>Sender: SendWatchdogNotification(notificationID, stats)
Sender->>Sender: Build message payload (title, summary, attributes)
Sender->>Sender: Post to primary/fallback recipients and record history
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
4 times, most recently
from
ac586a2 to
eb413de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
notification/job.go (1)
610-610: Consider usingduty.Now()for consistency.The
eventTimevariable usestime.Now()directly. While this is for an event payload rather than a database timestamp, usingduty.Now()would be more consistent with the codebase patterns.Suggested change
- eventTime := time.Now() + eventTime := duty.Now()This would require adding
"github.com/flanksource/duty"to the imports.As per coding guidelines: "Use
duty.Now() time.Timeinstead oftime.Now()for database timestamps and soft deletes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/job.go` at line 610, The code sets eventTime using time.Now() (eventTime := time.Now()); replace this with duty.Now() for consistency with project conventions and to use the shared time helper. Update the import list to include "github.com/flanksource/duty" and change the assignment to use duty.Now() wherever eventTime is created (reference: variable eventTime in notification/job.go) so the event payload uses the standardized timestamp provider.notification/events.go (1)
790-790: Avoid a magic key for watchdog notification ID.Line 790 uses a hard-coded
"id"property key. Please use a shared constant (used by both event producer and consumer) to prevent silent contract drift.♻️ Suggested refactor
+const watchdogNotificationIDProperty = "id" + if event.Name == api.EventWatchdog { - notificationID := event.Properties["id"] + notificationID := event.Properties[watchdogNotificationIDProperty] if notificationID == "" { return nil, fmt.Errorf("watchdog event is missing notification id") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/events.go` at line 790, Replace the hard-coded property key "id" used to read the watchdog notification ID from event.Properties (where notificationID := event.Properties["id"]) with a shared constant; define a exported constant (e.g., NotificationIDKey) in the notification package (or a shared constants file) and use that constant in the consumer code here and in the producer that sets the property so both sides read/write event.Properties[notification.NotificationIDKey] to avoid silent contract drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/notifications.go`:
- Around line 79-85: The current persistence path parses
obj.Spec.WatchdogInterval via text.ParseDuration and assigns it to
dbObj.WatchdogInterval without rejecting zero or negative durations; change the
block that handles obj.Spec.WatchdogInterval so that after parsing
(text.ParseDuration) you validate parsed > 0 and return an error if parsed <= 0
(e.g., "invalid watchdogInterval: must be > 0"), otherwise assign
dbObj.WatchdogInterval = parsed; reference obj.Spec.WatchdogInterval,
text.ParseDuration, and dbObj.WatchdogInterval when making the change.
---
Nitpick comments:
In `@notification/events.go`:
- Line 790: Replace the hard-coded property key "id" used to read the watchdog
notification ID from event.Properties (where notificationID :=
event.Properties["id"]) with a shared constant; define a exported constant
(e.g., NotificationIDKey) in the notification package (or a shared constants
file) and use that constant in the consumer code here and in the producer that
sets the property so both sides read/write
event.Properties[notification.NotificationIDKey] to avoid silent contract drift.
In `@notification/job.go`:
- Line 610: The code sets eventTime using time.Now() (eventTime := time.Now());
replace this with duty.Now() for consistency with project conventions and to use
the shared time helper. Update the import list to include
"github.com/flanksource/duty" and change the assignment to use duty.Now()
wherever eventTime is created (reference: variable eventTime in
notification/job.go) so the event payload uses the standardized timestamp
provider.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3abb418-fbb5-49f7-b373-dd661f956a45
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
api/event.goapi/v1/notification_types.goapi/v1/zz_generated.deepcopy.gocmd/server.goconfig/crds/mission-control.flanksource.com_notifications.yamlconfig/schemas/notification.schema.jsondb/notifications.gogo.modjobs/jobs.gonotification/cel.gonotification/events.gonotification/job.gonotification/message.go
💤 Files with no reviewable changes (1)
- go.mod
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
3 times, most recently
from
054db90 to
1c2eedb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notification/job.go`:
- Around line 610-647: The watchdog fallback is always executed even when the
primary send succeeded; modify the logic in the primary watchdog block (the
sendNotification call for the primary recipient) to capture its error (e.g.,
errPrimary := sendNotification(ctx, payload)) and then only run the fallback
branch (the sendNotification call that uses notification.FallbackPersonID /
FallbackCustomNotification) if errPrimary != nil (i.e., primary delivery
failed). Ensure you propagate errors appropriately: if primary fails and
fallback succeeds return nil, and if fallback also fails return a wrapped error
(use the existing fmt.Errorf pattern), keeping references to sendNotification,
NotificationEventPayload, HasRecipients, and HasFallbackSet.
- Around line 538-571: Concurrent SyncWatchdogJob calls can race because
watchdogJobs (sync.Map) only protects individual operations; serialize the whole
load/unschedule/store + schedule sequence per notification ID by introducing a
per-key lock (e.g., a map/sync.Map of *sync.Mutex or singleflight-like guard)
and acquire that lock at the start of SyncWatchdogJob and release it at the end;
keep using WatchdogNotificationJob and scheduleWatchdogJob but ensure
scheduleWatchdogJob is invoked while holding the per-ID lock so that only one
goroutine can create/unschedule/store jobs for a given notificationID and avoid
orphaned cron entries in watchdogJobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3159484d-1a1c-4317-9cb6-43e43e05eaa4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
api/event.goapi/v1/notification_types.goapi/v1/zz_generated.deepcopy.gocmd/server.goconfig/crds/mission-control.flanksource.com_notifications.yamlconfig/schemas/notification.schema.jsondb/notifications.gofixtures/notifications/health-check.yamlgo.modjobs/jobs.gonotification/cel.gonotification/events.gonotification/job.gonotification/message.gonotification/watchdog_test.go
💤 Files with no reviewable changes (1)
- go.mod
✅ Files skipped from review due to trivial changes (1)
- fixtures/notifications/health-check.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- notification/cel.go
- api/event.go
- notification/events.go
- jobs/jobs.go
- api/v1/notification_types.go
- notification/message.go
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
4 times, most recently
from
6e803a7 to
d52c1d1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notification/job.go`:
- Around line 640-642: In SendWatchdogNotification, don't short-circuit on
failure of the primary send; call sendNotification(ctx, payload) for the primary
and then independently call sendNotification(ctx, fallbackPayload) for the
fallback (or the existing fallback branch) regardless of the first call's
result, capture both errors, and return a combined error (or a wrapped error
that includes both messages) after attempting both sends; update the conditional
branches around sendNotification to execute both paths and aggregate errors
instead of returning immediately on the first failure.
In `@notification/message.go`:
- Around line 53-66: The EventWatchdog branch (case icapi.EventWatchdog) is
using cumulative env.Summary values and an absolute LastFailedAt timestamp;
change it to compute and display interval-scoped/relative stats: build msg.Title
and msg.Summary to indicate the watchdog interval, replace Sent/Failed/Pending
attribute values with interval counts derived from env.SummaryInterval (or
compute deltas from prior snapshot if available) and append relative time
strings (e.g., "2w ago") next to counts, set msg.Description to include the
interval-scoped error text (env.Summary.Error) if present, and format
LastFailedAt as a relative time instead of only RFC3339; update code that
appends attributes (msg.Attributes, keyValue) and the LastFailedAt handling to
use the interval counts and a helper that converts time.Time to relative labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f2a57b9-dcf7-425a-8ee7-3964667b2bc0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
api/event.goapi/v1/notification_types.goapi/v1/zz_generated.deepcopy.gocmd/server.goconfig/crds/mission-control.flanksource.com_notifications.yamlconfig/schemas/notification.schema.jsondb/notifications.gofixtures/notifications/health-check.yamlgo.modjobs/jobs.gonotification/cel.gonotification/events.gonotification/job.gonotification/message.gonotification/watchdog_test.go
💤 Files with no reviewable changes (1)
- go.mod
✅ Files skipped from review due to trivial changes (2)
- notification/events.go
- api/event.go
🚧 Files skipped from review as they are similar to previous changes (5)
- fixtures/notifications/health-check.yaml
- notification/cel.go
- config/schemas/notification.schema.json
- api/v1/notification_types.go
- notification/watchdog_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
notification/message.go (1)
53-66:⚠️ Potential issue | 🟠 MajorFormat watchdog output from interval-scoped, relative stats.
This branch still hard-codes cumulative-history wording on Line 55 and renders
Last Failed Atas RFC3339 on Line 65, so the watchdog message does not match the feature requirement to summarize the current watchdog interval with relative times like2w ago. Also,notification/events.go:789-803only assignsstats[0]intoenv.Summary, so this path cannot produce interval-specific values unless that data is carried into the env/message layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/message.go` around lines 53 - 66, The watchdog branch in the case for icapi.EventWatchdog currently uses cumulative-history wording and RFC3339 timestamps; update the text and time formatting to reflect interval-scoped, relative stats: change msg.Summary to describe the current watchdog interval (remove "cumulative across the full notification history" phrasing), and render "Last Failed At" using a relative formatter (e.g., call an existing helper like RelativeTime(env.Summary.LastFailedAt) or add a small humanizeTime function to produce "2w ago"/"3h ago") instead of time.RFC3339; also ensure the interval-specific data reaches this code by having the caller populate env.Summary with the correct interval stats (update the code that prepares env.Summary to pass the intended stats slice element rather than always stats[0], or add a new field conveying the interval stats into env that this branch reads).
🧹 Nitpick comments (1)
notification/job.go (1)
510-526: Usectx.Oops().Wrapfon the new watchdog error paths.The new watchdog helpers are returning raw
fmt.Errorfwrappers for DB, scheduler, and send failures, which skips the repo-standard error context/categorization. Please switch these internal-error paths toctx.Oops().Wrapf(...).As per coding guidelines: "For wrapping internal errors, use
ctx.Oops().Wrap(err)orctx.Oops().Wrapf(err, "context")."Also applies to: 576-580, 606-614, 641-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/job.go` around lines 510 - 526, Replace raw fmt.Errorf error returns in the watchdog helper paths with the repo-standard ctx.Oops().Wrapf(...) calls: in InitWatchdogNotifications replace the DB fetch error return and the error returned after calling SyncWatchdogJob with ctx.Oops().Wrapf(err, "failed to ...") using the same contextual messages; similarly update error returns inside SyncWatchdogJob and its downstream helpers (the scheduler registration and send/failure paths referenced around lines 576-580, 606-614, 641-659) to use ctx.Oops().Wrapf(err, "...") instead of fmt.Errorf so all internal errors are wrapped and categorized by the context's Oops wrapper. Ensure you preserve the original context string (e.g., "failed to get notifications with watchdog schedule", "failed to schedule watchdog job for notification(%s)", etc.) while passing the underlying err into ctx.Oops().Wrapf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notification/job.go`:
- Around line 562-570: The code deletes the existing watchdog job
(existingJob.Unschedule() and watchdogJobs.Delete(notificationID)) before
ensuring the new job registers, which can leave a notification without a
watchdog if scheduleWatchdogJob/AddToScheduler fails; instead, first attempt to
create/register the replacement by calling scheduleWatchdogJob (or the internal
AddToScheduler) and only unschedule/delete the old job after the new job is
confirmed; if scheduleWatchdogJob returns an error, ensure you keep the existing
job intact or re-register it (restore watchdogJobs entry and call AddToScheduler
on existingJob) so the previous watchdog remains active on failure.
---
Duplicate comments:
In `@notification/message.go`:
- Around line 53-66: The watchdog branch in the case for icapi.EventWatchdog
currently uses cumulative-history wording and RFC3339 timestamps; update the
text and time formatting to reflect interval-scoped, relative stats: change
msg.Summary to describe the current watchdog interval (remove "cumulative across
the full notification history" phrasing), and render "Last Failed At" using a
relative formatter (e.g., call an existing helper like
RelativeTime(env.Summary.LastFailedAt) or add a small humanizeTime function to
produce "2w ago"/"3h ago") instead of time.RFC3339; also ensure the
interval-specific data reaches this code by having the caller populate
env.Summary with the correct interval stats (update the code that prepares
env.Summary to pass the intended stats slice element rather than always
stats[0], or add a new field conveying the interval stats into env that this
branch reads).
---
Nitpick comments:
In `@notification/job.go`:
- Around line 510-526: Replace raw fmt.Errorf error returns in the watchdog
helper paths with the repo-standard ctx.Oops().Wrapf(...) calls: in
InitWatchdogNotifications replace the DB fetch error return and the error
returned after calling SyncWatchdogJob with ctx.Oops().Wrapf(err, "failed to
...") using the same contextual messages; similarly update error returns inside
SyncWatchdogJob and its downstream helpers (the scheduler registration and
send/failure paths referenced around lines 576-580, 606-614, 641-659) to use
ctx.Oops().Wrapf(err, "...") instead of fmt.Errorf so all internal errors are
wrapped and categorized by the context's Oops wrapper. Ensure you preserve the
original context string (e.g., "failed to get notifications with watchdog
schedule", "failed to schedule watchdog job for notification(%s)", etc.) while
passing the underlying err into ctx.Oops().Wrapf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87df2623-36ce-4348-8bf3-4465eab67330
📒 Files selected for processing (2)
notification/job.gonotification/message.go
adityathebe
force-pushed
the
feat/notification-watch-dog
branch
from
01dcedb to
48dba55
Compare
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
1 participant
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.
resolves: #1985
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores