Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| ErrorMessage: err.Error(), | ||
| }) | ||
|
|
||
| continue |
There was a problem hiding this comment.
afict we were inserting decision twice here for the same filterId
There was a problem hiding this comment.
good catch, this looks like a bug to me 👍 thanks
There was a problem hiding this comment.
Pull request overview
Reduces noisy error reporting for expected/benign failure modes in scheduling and repository flows (shutdown cancellations, CEL evaluation errors, and “no rows” cases).
Changes:
- Suppress queue refill error logging when the queuer context is canceled (shutdown path).
- Downgrade CEL evaluation failures from error to warning and stop further processing of that filter after an evaluation error.
- Treat
pgx.ErrNoRowsas a non-error when checking if tenant alerting is active.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/scheduling/v1/queuer.go | Skips logging refill errors during shutdown cancellation. |
| pkg/repository/trigger.go | Lowers log severity for expected CEL evaluation failures and prevents post-error decision processing. |
| pkg/repository/ticker.go | Returns (false, zeroTime, nil) on ErrNoRows for tenant alerting settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -201,6 +202,8 @@ func (r *sharedRepository) makeTriggerDecisions(ctx context.Context, filters []* | |||
| Source: sqlcv1.V1CelEvaluationFailureSourceFILTER, | |||
| ErrorMessage: err.Error(), | |||
| }) | |||
|
|
|||
| continue | |||
| } | |||
There was a problem hiding this comment.
This change (adding continue on expression evaluation error) alters runtime behavior (it prevents the subsequent decisions = append(...shouldTrigger...) from running in the error case). The PR description/type currently claims a refactor with no behavior change; it should be updated to reflect that this is a functional bug fix.
There was a problem hiding this comment.
i dont think its a functional bug... i think we probably dedupe this somewhere higher, but i did not trace this
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| if err != nil { | ||
| r.l.Error(). | ||
| // This is notified to the user via an olap event. |
There was a problem hiding this comment.
it is? I'm pretty sure we write this to the db but don't show it on the frontend or anything, at least right now
2 participants
Description
Some errors are expected behavior and should be suppresed
Type of change