Improves chat service logging and error handling - #55
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves observability and resilience of the chat flow by enhancing startup logging of configured chat models and broadening the set of transient API failures that trigger fallback from the primary to the secondary model.
Changes:
- Add an informational log during
ChatServiceconstruction to report configured primary and fallback model names. - Expand fallback-triggering
ApiExceptionstatus codes to include 502/503/504 and include theErrorCodein warning logs. - Add explicit warning + rethrow behavior when transient failures occur but no fallback model is configured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Pacos/Services/ChatCommandHandlers/MentionHandler.cs | Broaden fallback conditions and improve warning logs to include ErrorCode and “no fallback configured” path. |
| Pacos/Program.cs | Log configured primary/fallback model names when ChatService is created via DI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (ApiException ex) when (ex.ErrorCode is 502 or 503 or 504) | ||
| { |
There was a problem hiding this comment.
The transient/unavailable error code list (502/503/504) is now duplicated between the retry policy and this catch filter. To avoid the two drifting over time, consider extracting a single helper (e.g., IsModelUnavailable(ApiException)) or a shared static readonly int[]/pattern so both the retry handler and fallback switch use the same source of truth.
| _logger.LogWarning(ex, "Primary chat model unavailable after retry, switching to fallback model"); | ||
| if (!_chatService.HasFallback) | ||
| { | ||
| _logger.LogWarning(ex, "Primary chat model returned {ErrorCode} but no fallback model is configured", ex.ErrorCode); |
There was a problem hiding this comment.
When no fallback is configured, this method logs a warning and then rethrows; the caller (HandleMentionAsync) also logs the same exception as an error. This can produce duplicated stack traces/noisy logs for the same failure. Consider either: (1) removing this warning in the no-fallback path and letting the caller handle logging, or (2) having the caller suppress/log differently for ApiException when this warning is emitted.
| _logger.LogWarning(ex, "Primary chat model returned {ErrorCode} but no fallback model is configured", ex.ErrorCode); |
| logger.LogInformation("Chat models configured — Primary: {PrimaryModel}, Fallback: {FallbackModel}", | ||
| options.ChatModel, | ||
| string.IsNullOrWhiteSpace(options.ChatModelFallback) ? "(none)" : options.ChatModelFallback); | ||
| return new ChatService( |
There was a problem hiding this comment.
This log is inside the AddSingleton factory, so it runs when ChatService is first resolved (singleton creation), not when the service is registered. If the intent is a guaranteed startup log regardless of whether ChatService is ever resolved, consider moving this to host startup (e.g., after building the host, or via an IHostedService that logs the configured models once).
Enhances the application's resilience and diagnosability for chat services:
ApiExceptionerror codes (502, 503, 504) that trigger a fallback to an alternative chat model, making the system more robust against various service unavailability issues.ErrorCodein warnings when the primary chat model fails and a fallback is attempted or when no fallback is configured.