Conversation
nabisobhi
marked this pull request as draft
August 30, 2026 18:56
nabisobhi
force-pushed
the
nabisobhi-dlq-observability
branch
from
September 10, 2026 12:22
ce37df9 to
47df637
Compare
Consumer now accepts an optional ILogger<Consumer> (defaulting to NullLogger) and logs an error with the exception type, partition key and source topic when a registered bypass exception is about to propagate and crash the consumer. The real logger is resolved in both AddConsumer overloads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nabisobhi
force-pushed
the
nabisobhi-dlq-observability
branch
from
September 17, 2026 11:48
47df637 to
c17b8ba
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Moving the bypass predicate out of the exception filter can replace the original failure when the predicate throws.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds contextual error logging when an exception bypasses the dead-letter queue.
Changes:
- Injects an optional consumer logger.
- Logs bypassed exceptions before rethrowing.
- Adds logging tests and a logger spy.
File summaries
| File | Description |
|---|---|
src/Dafda/Consuming/Consumer.cs |
Logs bypassed exceptions. |
src/Dafda/Configuration/ConsumerServiceCollectionExtensions.cs |
Resolves consumer loggers. |
src/Dafda.Tests/TestDoubles/LoggerSpy.cs |
Adds a logging test double. |
src/Dafda.Tests/Consuming/TestConsumer.cs |
Tests bypass logging behavior. |
src/Dafda.Tests/Builders/ConsumerBuilder.cs |
Supports logger injection in tests. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Evaluating the bypass predicate outside an exception filter meant a throwing predicate replaced the original handler exception. ShouldBypassDeadLetterQueue now treats a failing predicate as a bypass, so the bare rethrow propagates the original exception instead of dead-lettering it or losing it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nabisobhi
marked this pull request as ready for review
September 17, 2026 12:19
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.
Why
Dafda lets you register exception types that bypass the dead letter queue and propagate instead, crashing the consumer — the escape hatch for systemic failures like a database outage, where dead-lettering would silently drain a whole topic.
When that bypass fires, nothing is logged from the
Consumer's point of view. The consumer just dies, with no record of which message triggered it or which exception was responsible.Change
Consumernow takes an optionalILogger<Consumer>(defaulting toNullLogger<Consumer>.Instance, so existing call sites and tests are unaffected) and logs before rethrowing:Logged at Error level with the exception attached. The real logger is resolved in both
AddConsumeroverloads.To log at that point, the bypass check moved out of the exception filter into the catch body, which now rethrows with a bare
throw;. That preserves the original stack trace. The check still runs before the retry/backoff branch, so a bypassed exception fails fast without waiting out a backoff delay.Keeping the semantics of a throwing predicate
The bypass predicate is user-supplied, and moving it out of the exception filter changed one edge case. C# treats an exception thrown by a filter as a non-match, so previously a predicate that threw meant the original handler exception simply escaped — it was never dead-lettered. Evaluated in a catch body, a throwing predicate would instead replace the original exception and hide the real failure.
ShouldBypassDeadLetterQueuenow absorbs that, keepingDispatchclean:Returning
trueroutes into the bypass branch, whose barethrow;propagates the original exception — matching the old behavior. (falsewould not: that dead-letters the message, which the old code never did here.)Overlap worth a reviewer's opinion
ConsumerHostedService.ConsumeAllalready logs any escaping exception as"Unhandled error occurred while consuming messaging", so a bypassed exception will now produce two Error entries: this context-rich one, then the generic boundary one. That's a common pattern (detail at the site, generic at the boundary), but if you'd rather avoid the duplication, dropping this to Warning is an easy alternative.Notes
Microsoft.Extensions.Logging.Abstractionsis already referenced.Consumerisinternal.LoggerSpy<T>test double rather thanMock<ILogger<Consumer>>, because Moq can't proxy a generic closed over an internal type withoutInternalsVisibleTo("DynamicProxyGenAssembly2").Tests
Three new tests: the bypass logs the expected level/exception/message, dead-lettered messages do not log the bypass error, and a throwing
BypassWhenpredicate still surfaces the original exception without dead-lettering.TestConsumer42/42, full suite 185/185, build clean with 0 warnings.