Fix/alerter scheduled retries - #1259
Draft
Mike Keesey (mkeesey) wants to merge 2 commits into
Draft
Mike Keesey (mkeesey) wants to merge 2 commits into
Mike Keesey (mkeesey) wants to merge 2 commits into
Conversation
added 2 commits
September 16, 2026 21:08
Schedule bounded retries for transient Kusto remote entity resolution failures while preserving query windows and absolute execution cadence. Delay deadline creation until a concurrency slot is acquired and keep query health unhealthy when failure notification delivery fails. Add coverage for retry scheduling, scheduled windows, owning-team alert routing, and exhausted-retry health behavior.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces at least one reliability regression risk (unbounded context during throttling-alert send) and a misleading duration log due to the new scheduled executionTime semantics.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the alerter worker scheduling/execution loop to support scheduled retries for narrowly-scoped transient Kusto SEM0056 remote entity resolution failures, while preserving consistent query windows and improving testability via injected clocks.
Changes:
- Replaces the worker ticker with a clock-backed timer loop that supports rescheduling (including retry delays) while keeping the normal cadence anchored to fixed deadlines.
- Adds targeted retry handling for transient SEM0056 remote entity resolution failures, reusing the original query window and reserving time for failure notification.
- Introduces injectable clocks across executor/worker/evaluation and expands unit tests to validate scheduling, retry behavior, and cancellation.
File summaries
| File | Description |
|---|---|
| alerter/engine/worker.go | Implements timer-based scheduling, retry state machine, notification time reservation, and clock injection in the worker loop. |
| alerter/engine/worker_test.go | Adds extensive tests for retry scheduling, fixed-window semantics, cancellation behavior, and clock-defaulting. |
| alerter/engine/status_test.go | Updates evaluation timing assertions using a fake clock and cached elapsed behavior. |
| alerter/engine/executor.go | Adds executor-level clock injection and passes it through to workers. |
| alerter/engine/executor_test.go | Tests executor clock propagation and default clock behavior. |
| alerter/engine/evaluation.go | Makes evaluation timing clock-driven and supports scheduled execution times distinct from start time. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+441
to
+452
| err := e.alertCli.Create(ctx, e.alertAddr, alert.Alert{ | ||
| Destination: e.rule.Destination, | ||
| Title: fmt.Sprintf("Alert %s/%s has too many notifications in %s", e.rule.Namespace, e.rule.Name, e.region), | ||
| Summary: summary, | ||
| Severity: 3, | ||
| Source: fmt.Sprintf("notification-failure/%s/%s", e.rule.Namespace, e.rule.Name), | ||
| CorrelationID: fmt.Sprintf("notification-failure/%s/%s", e.rule.Namespace, e.rule.Name), | ||
| }) | ||
| if err != nil { | ||
| logger.Errorf("Failed to send alert for throttled notification for %s/%s: %s", e.rule.Namespace, e.rule.Name, err) | ||
| } | ||
| e.updateAlertRuleStatus(ctx, evaluation, "Throttled", "Too many notifications sent") |
Comment on lines
+354
to
+369
| if isTransientRemoteEntityResolutionError(err) { | ||
| if retry.initialErr == nil { | ||
| retry.initialErr = err | ||
| } | ||
| if retry.attempt < remoteEntityResolutionMaxAttempts && e.hasRetryBudget(retry.deadline) { | ||
| retry.attempt++ | ||
| logger.Warnf("Query %s/%s failed with a transient remote entity resolution error; scheduling retry attempt %d/%d after %s", e.rule.Namespace, e.rule.Name, retry.attempt, remoteEntityResolutionMaxAttempts, e.retryDelay) | ||
| return queryAttemptResult{retry: retry, retryable: true} | ||
| } | ||
| return queryAttemptResult{ | ||
| retry: retry, | ||
| err: &remoteEntityResolutionRetryError{ | ||
| initialErr: retry.initialErr, | ||
| retryErr: err, | ||
| }, | ||
| } |
Comment on lines
+414
to
+418
| if err == nil && !result.retry.notificationsThrottled { | ||
| metrics.QueryHealth.WithLabelValues(e.rule.Namespace, e.rule.Name).Set(1) | ||
| metrics.QueriesRunTotal.WithLabelValues().Inc() | ||
| logger.Infof("Completed %s/%s in %s", e.rule.Namespace, e.rule.Name, e.clock.Since(evaluation.executionTime)) | ||
| logger.Infof("Query for %s/%s completed with %d entries found", e.rule.Namespace, e.rule.Name, evaluation.rows) |
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
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.
This is intended to help implement #1235 in a way where we use the main worker loop to handle reschedules.
The main changes here are to change the ticker to a timer to allow for customizing the reset periods. For retry cases, we set a time a short period of time in the future, otherwise we set to a normal cadence based on the end of the window we are processing.
In retry cases, we retain the same window configuration to ensure we retry the same query with the same parameters.
This only retries a given failure case now, but the intention is to make it easier to add additional error types that tend to be ephemeral in practice.