ROB-2005: improve error handling for streaming (slackbot) - #935
Conversation
WalkthroughAdds logging and a catch-all Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as stream_chat_formatter
participant P as LLM Provider
C->>S: Start stream
S->>P: Send request / stream tokens
alt Success
P-->>S: Token chunks
S-->>C: SSE data chunks
else RateLimitError
P--xS: Raise RateLimitError
S-->>C: SSE error (rate_limit)
else Other Exception
P--xS: Raise Exception
S->>S: log.exception(e)
alt Message contains "Model is getting throttled"
S-->>C: SSE error (rate_limit via Bedrock throttle)
else Any other exception
S-->>C: SSE error (description=msg=exception, error_code=1)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nherment
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/utils/stream.py (2)
72-75: Add return type for the generator (coding guideline: type hints required).Both formatters yield SSE strings; annotate the return type to keep mypy happy.
def stream_chat_formatter( call_stream: Generator[StreamMessage, None, None], followups: Optional[List[dict]] = None, -): +) -> Generator[str, None, None]:
48-51: Add return type to stream_investigate_formatter as well.Matches behavior (yields SSE strings).
def stream_investigate_formatter( call_stream: Generator[StreamMessage, None, None], runbooks -): +) -> Generator[str, None, None]:
🧹 Nitpick comments (2)
holmes/utils/stream.py (2)
89-100: Consider logging the exception with traceback for observability.Today the error is only surfaced to the client; add structured logging to aid triage.
Add near the imports:
import logging logger = logging.getLogger(__name__)Then inside the catch-all block before yielding:
logger.exception("stream_chat_formatter failed")
48-70: Consistency: mirror the generic error handling in investigate formatter (optional).Only
stream_chat_formatterhas the catch-all path;stream_investigate_formatterwill still drop non-rate-limit errors. Consider mirroring the same behavior to provide consistent SSE error semantics.If you want, I can draft the mirrored try/except for
stream_investigate_formatter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
holmes/utils/stream.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/utils/stream.py
🪛 Ruff (0.12.2)
holmes/utils/stream.py
91-91: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: Build and test HolmesGPT
holmes/utils/stream.py
[error] 89-100: ruff-format failed: 1 file reformatted by this hook during pre-commit. 1 file reformatted, 326 files left unchanged. Command: 'pre-commit run --show-diff-on-failure --color=always --all-files'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
holmes/utils/stream.py (2)
91-100: Run pre-commit (ruff‐format) locally to fix formatting
CI indicates formatting errors in holmes/utils/stream.py (likely inline comment spacing or trailing whitespace). Install pre-commit if needed and runpre-commit run --all-files, then commit the fixes.
95-99: Standardize error_code usage: Generic errors use1(holmes/utils/stream.py:97) while rate-limit errors use5204(holmes/utils/stream.py:43), and there’s no centralErrorCodeenum or constants. Please verify any existing conventions or introduce a shared enumeration/constants for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/utils/stream.py (1)
49-51: Add return type hints to the streaming formatters.Mypy is enabled; these should declare they yield strings.
-def stream_investigate_formatter( - call_stream: Generator[StreamMessage, None, None], runbooks -): +def stream_investigate_formatter( + call_stream: Generator[StreamMessage, None, None], runbooks +) -> Generator[str, None, None]: @@ -def stream_chat_formatter( - call_stream: Generator[StreamMessage, None, None], - followups: Optional[List[dict]] = None, -): +def stream_chat_formatter( + call_stream: Generator[StreamMessage, None, None], + followups: Optional[List[dict]] = None, +) -> Generator[str, None, None]:Also applies to: 73-76
♻️ Duplicate comments (2)
holmes/utils/stream.py (2)
92-97: Harden catch-all: noqa BLE001, log traceback, normalize Bedrock check, and avoid exposing internals.At a streaming boundary a broad
exceptis fine; annotate it, log with traceback, make the Bedrock check case-insensitive and broader, and don’t send raw exception text in the user-facingmsg.- except Exception as e: - logging.error(e) - if "Model is getting throttled" in str(e): # happens for bedrock - yield create_rate_limit_error_message(str(e)) - else: - yield create_sse_error_message(description=str(e), error_code=1, msg=str(e)) + except Exception as e: # noqa: BLE001 - streaming boundary: convert any failure to SSE + err = str(e) + # include traceback for diagnostics + logger.exception("stream_chat_formatter failed") + # Bedrock throttling (make robust to provider/case variants) + if any(s in err.lower() for s in ( + "model is getting throttled", + "throttlingexception", + "too many requests", + "rate exceeded", + )): + yield create_rate_limit_error_message(err) + else: + yield create_sse_error_message( + description=err, + error_code=1, + msg="Unexpected error" + )
90-95: Uselogger.exceptioninstead oflogging.errorto keep traceback.Also satisfies TRY400. This is covered in the diff above.
🧹 Nitpick comments (1)
holmes/utils/stream.py (1)
8-8: Prefer a module-level logger over the root logger.Define
logger = logging.getLogger(__name__)and use it (keeps logs scoped and configurable).-import logging +import logging +logger = logging.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
holmes/utils/stream.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/utils/stream.py
🪛 Ruff (0.12.2)
holmes/utils/stream.py
92-92: Do not catch blind exception: Exception
(BLE001)
93-93: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/utils/stream.py (1)
94-99: Catch-all: justify BLE001, log with traceback, and normalize Bedrock throttling check; avoid leaking raw errors to clients.Add
# noqa: BLE001with rationale, uselogger.exceptionfor stack, do a case-insensitive throttling match, and return a genericmsgto the client to prevent information leakage.- except Exception as e: - logging.error(e) - if "Model is getting throttled" in str(e): # happens for bedrock - yield create_rate_limit_error_message(str(e)) - else: - yield create_sse_error_message(description=str(e), error_code=1, msg=str(e)) + except Exception as e: # noqa: BLE001 - streaming boundary: convert any failure to SSE + err = str(e) + logger.exception("stream_chat_formatter failed") + if "model is getting throttled" in err.lower(): # bedrock throttling + yield create_rate_limit_error_message(err) + else: + yield create_sse_error_message(description=err, error_code=1, msg="Unexpected error")
🧹 Nitpick comments (1)
holmes/utils/stream.py (1)
8-8: Define a module-level logger; prefer stack-trace logging.Add a named logger and use
logger.exception(...)in the catch-all below to capture traceback (TRY400).import logging +logger = logging.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/utils/stream.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting
Type hints are required (checked by mypy)
Always place Python imports at the top of the file, not inside functions or methods
Files:
holmes/utils/stream.py
🪛 Ruff (0.12.2)
holmes/utils/stream.py
94-94: Do not catch blind exception: Exception
(BLE001)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
2 participants
No description provided.