fix(logmq): filter unsubscribed operator events before suppression - #1050
Conversation
alexluong
left a comment
There was a problem hiding this comment.
Thanks for opening the PR. Sorry for the delayed review.
Overall LGTM. Great catch as well!
One small question here. Once that is settled we can merge and include in the next release!
| // Filter before entering a suppression window. Emit also filters as a | ||
| // boundary safeguard, but an unsubscribed event must not touch Redis or | ||
| // turn a suppression failure into a delivery failure for an event that | ||
| // would never be sent. | ||
| if !bp.alerts.Emitter.Enabled(de.event.Topic) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I wonder if we should move this logic to plan step instead of send?
There was a problem hiding this comment.
Thank you for taking a look!
Good question. I placed the check in send intentionally because topic filtering and suppression are both delivery concerns, while plan handles domain decisions and actions. In particular, disabling a destination must still happen even when the corresponding notification topic is not subscribed.
Also, not every event passes through plan. For example, attempt.success and the alerts-disabled attempt.failed path construct deliveryEvents directly. Keeping the check at the final delivery boundary guarantees that no unsubscribed event can enter suppression, regardless of where it was produced.
There was a problem hiding this comment.
Makes sense! Merging now.
Also from your error report, I discovered the root cause of the context deadline exceeded issue here #1063
Summary
Filter unsubscribed operator events before entering their delivery suppression window.
This prevents an event that will never be emitted from:
opevent delivery failedlog;Problem
Operator-event topic filtering currently happens inside
Emitter.Emit.For events with a suppression key,
logmq.BatchProcessor.sendenters the Redis-backed suppression window before callingEmitter.Emit. This means the topic filter is applied too late.For example:
When an attempt exhausts its retries, the alert evaluator can still plan an
alert.attempt.exhausted_retriesevent. Although that topic is not subscribed, the previous delivery path was:Emitter.Emit.Emitter.Emitsees that the topic is disabled and discards it.As a result, an unsubscribed event could fail before reaching the component responsible for filtering it:
The log message was then nacked even though no exhausted-retries event was configured for delivery.
Root cause
Topic eligibility was checked after optional delivery-layer behavior:
Filtering needs to happen before any behavior specific to delivering the event:
Fix
BatchProcessor.sendnow checksEmitter.Enabledbefore entering the suppression window:Emitter.Emitretains its existing topic check as a boundary safeguard.Subscribed events continue through the same suppression and delivery path as before.
Tests
Added a regression test covering the reported configuration:
alert.destination.disabledis subscribed;context.DeadlineExceeded;alert.destination.disabledis delivered;alert.attempt.exhausted_retriesis not delivered;The existing delivery-suppression tests continue to cover subscribed exhausted-retry events, including:
Impact
Test command
go test ./internal/logmqFor maintainers
Please review if the change makes sense in a broader scope that I may be missing here as I have found this issue in my use case where I am only interested in
alert.destination.disabledevents running 3 replicas of outpost-log service specifically.