Conversation
… retention sweeper in tests
johnsimons
left a comment
johnsimons
left a comment
There was a problem hiding this comment.
From a coding perspective this PR is fine, no issues.
But displaying a custom check without an action plan for the customer to address it is not going to help, we may as well tell them to raise a support case.
This may need more thought.
| catch (Exception ex) when (ex is not OperationCanceledException) | ||
| { | ||
| logger.LogError(ex, "Error during the {RetentionEntity} retention pass", entity); | ||
| RecordFailure(entity, ex.Message); |
There was a problem hiding this comment.
We are in essence displaying ex.Message to customers, is that going to be useful for them to solve the problem?
Will it prevent them from raising a support case with us?
What action should they take if they see such error in ServicePulse?
There was a problem hiding this comment.
Do you have any suggestions to what action we could give them?
The main thing that I'd see this failing on would either be deadlocks or timeouts from an overloaded DB, a completely unreachable DB takes ServicePulse offline equivalent to the SC instance disappearing.
| if (failedMessagesSucceeded && eventLogSucceeded && groupCommentsSucceeded) | ||
| { | ||
| lock (failures) | ||
| { | ||
| failures.Clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we be clearing individual failures as they become successful rather than waiting for all 3? If 1 is fixed, it'l still show all 3 failing until all 3 are resolved.
Take a look at RentetionMetrics which is also tracking retention failures. Could we re-use at all?
| @@ -129,6 +131,32 @@ async Task SweepWithoutAcquiringLock() | |||
|
|
|||
| public RetentionSweepCurrentStatus GetStatus() => new(isRunning, lastStartedAt, lastFinishedAt, lastErrorCutoff, lastEventsCutoff); | |||
There was a problem hiding this comment.
Will the custom check pass if the retention never starts? Could we also raise a failure for this as well?
| catch (Exception ex) when (ex is not OperationCanceledException) | ||
| { | ||
| logger.LogError(ex, "Error during the {RetentionEntity} retention pass", entity); | ||
| RecordFailure(entity, ex.Message); |
There was a problem hiding this comment.
Take a look at the pattern used in the below. Could we use this to move some of the failure logic out of the retention sweeper?
3 participants
This pull request introduces a new custom check for monitoring retention sweep failures in the EF Core persistence layer. It tracks and reports failures in the retention process, ensuring that any issues are surfaced via the custom checks infrastructure and are only cleared after a fully successful sweep. The changes include the implementation of the custom check, supporting logic in the sweeper, updates to configuration, and comprehensive tests.
Retention Sweep Monitoring and Reporting:
RetentionSweepCustomCheckclass that reports failures in the retention sweep process as a custom check, failing after any retention pass fails and recovering after the next fully successful sweep. This check summarizes failures per entity and integrates with the custom check infrastructure. [1] [2]RetentionSweeperto track active failures per entity, provide methods for recording and retrieving failures, and clear failures only after all passes succeed in a sweep. The sweeper now returns pass/fail status for each retention entity and records reasons for failures. [1] [2] [3]Testing and Verification:
RetentionSweepCustomCheckTeststo verify the behavior of the custom check, including its response to failures, recovery, time persistence, and deduplication of state change events.Configuration and Integration:
Test Artifacts: