feat: enhance suppression summary handling and event processing#5
feat: enhance suppression summary handling and event processing#5osamaalsahafi wants to merge 1 commit into
Conversation
- Introduced `collect_unreported_summaries` to retrieve and mark unreported suppressions. - Updated `check_event` methods to return suppression summaries when events are allowed. - Added support for episode-end summaries in rate limiting policies. - Enhanced `SuppressionCounter` to track reported suppressions and manage unreported claims. - Improved tests for summary emissions and suppression behavior.
nootr
left a comment
nootr
left a comment
There was a problem hiding this comment.
@osamaalsahafi First of all, thank you a lot for putting this together! I really appreciate the time and thought that went into this, especially the analysis around policy-specific summary semantics. The added tests and the claim_unreported direction show a lot of care.
After reviewing it, I think this PR is trying to solve a bit too much at once. The core bug in #4 is that active emission repeatedly emits stale cumulative summaries. For that, I think the safest first step is to make the active emitter delta/unreported-based.
The episode-end summary behavior is interesting, but it feels like a separate feature/design change. It changes when summaries are emitted, affects should_allow, interacts with formatter execution, and changes behavior even outside active emission. I would prefer to split that out from the #4 fix.
I’d also like to see regression coverage for the main behavior boundaries before merging:
- A test that
enable_active_emission(false)does not emit summaries. - A storage/Redis-oriented test or design adjustment showing that claimed/reported state is actually persisted and stale summaries are not re-emitted with non-in-memory storage.
My preferred path would be to narrow this PR to the active-emitter fix only, then handle episode-end summaries and Redis persistence either in follow-ups or with a more explicit design. Again, thank you for the work here! I know this may be more involved than expected, and I’m happy to help split or pick up parts of it if that makes it easier.
| /// that have not yet been reported is emitted through the summary formatter. | ||
| pub fn should_allow(&self, signature: EventSignature) -> bool { | ||
| matches!(self.limiter.check_event(signature), LimitDecision::Allow) | ||
| let (decision, episode) = self.limiter.check_event_with_summary(signature); |
There was a problem hiding this comment.
This now emits summaries even when enable_active_emission is false, which looks like a breaking behavior change: users who did not opt into active summaries may now get WARN summary events.
Also, putting this in should_allow changes it from an allow/suppress predicate into a reporting path with side effects: it can claim summaries and call the formatter. That feels like the wrong layer for this concern.
For fixing #4, I think we should keep should_allow* focused on rate-limit decisions and limit this PR to making the active emitter delta/unreported-based. If episode-end summaries are desirable, they should probably be a separate explicit opt-in (e.g. a dedicated builder option) and not implicitly tied to every Allow.
What do you think, @osamaalsahafi ?
| return; | ||
| } | ||
|
|
||
| if let Some(unreported) = state.counter.claim_unreported() { |
There was a problem hiding this comment.
I think this does not work correctly with RedisStorage. collect_unreported_summaries() now mutates reporting state through state.counter.claim_unreported() while iterating with Storage::for_each(&EventState).
That works for in-memory storage because the EventState reference points at the live state. But RedisStorage::for_each deserializes each entry into a temporary EventState, passes a reference to the callback, and then drops it without writing it back. So the reported mark is never persisted, and active emission with Redis would likely keep re-emitting the same summaries.
The new reported_count and reported boundary state also are not included in the Redis serialization format, so deserialized counters always start with nothing reported.
I think this needs either a storage-level way to claim unreported summaries and persist that mutation, or the reported state has to be serialized and updated through a mutable storage path. Otherwise #4 remains unfixed for Redis-backed users.
Sorry if this is a bigger thing to ask than expected. If this makes the PR too large, let me know. I can also pick up the Redis/persistence part separately later, or we can split this PR so the in-memory active-emission fix lands first and Redis is handled in a follow-up.
| /// Advances the reported mark to the current count and returns the newly | ||
| /// claimed range, or `None` if there is nothing new to report. | ||
| /// | ||
| /// # Thread Safety |
There was a problem hiding this comment.
Nit, and assuming I understand this correctly: I think the thread-safety guarantee here is a bit stronger than what the code actually guarantees.
reported_count.fetch_max(count) makes the count claim mostly safe, but the timestamp boundary is updated separately through reported_boundary_nanos. If two reporters race, or if a suppression is recorded while a claim is happening, the count and boundary can observe different moments in time.
For example, record_suppression() increments the count before storing last_suppressed_nanos. A claim can see the new count but still read the old last timestamp, mark that suppression as reported, and produce a summary with an inaccurate range. With two concurrent claimers, the boundary can also be overwritten out of order.
This may be acceptable if the only intended reporter is a single active emitter task, but then I’d avoid documenting this as “exactly one reporter claims each suppression” with a fully correct range. Either the docs should describe the range as best-effort under concurrency, or the claim needs to be guarded by the same mutable state path so count and boundary are updated consistently.
What do you think?
| matches!(self.limiter.check_event(signature), LimitDecision::Allow) | ||
| let (decision, episode) = self.limiter.check_event_with_summary(signature); | ||
| if let Some(summary) = episode { | ||
| (self.summary_formatter)(&summary); |
There was a problem hiding this comment.
One more concern with emitting this from should_allow: the formatter now runs synchronously in the tracing filter path.
The periodic emitter catches panics from the emit function and runs outside the event decision path. This route does neither. A custom formatter that panics, blocks, takes a lock, or emits more tracing events can now directly affect whether normal log events are processed.
If we keep episode-end summaries, I think this path should at least have the same panic isolation as the active emitter. But my preference would be to keep formatter execution out of should_allow entirely and let the active emitter own summary emission.
| let min_count = self.config.min_count; | ||
|
|
||
| self.registry.for_each(|signature, state| { | ||
| if state.counter.count() < min_count { |
There was a problem hiding this comment.
Nitpick / semantics question: min_count is checked against the all-time counter, but the summary emitted from this method contains only the unreported delta.
That means once a signature has crossed the threshold, future single suppressions can be emitted as summaries with count = 1. For a setting documented as “minimum suppression count to include in summary”, I would probably expect the threshold to apply to the emitted count/delta instead.
Maybe the current behavior is intentional to match collect_summaries(), but then I think it should be documented very explicitly because it is easy to misread.
|
Hi @osamaalsahafi, hope you're doing well. Thank you again for your help, I really appreciate it! This bug needed to be fixed ASAP, so I decided to make a new PR to fix it: #6 Is it OK if I close this PR? |
2 participants
Close #4
collect_unreported_summariesto retrieve and mark unreported suppressions.check_eventmethods to return suppression summaries when events are allowed.SuppressionCounterto track reported suppressions and manage unreported claims.