ROB-2555 slack bot stream render graphs #1962
Conversation
WalkthroughAdds SSE parsing/creation utilities and a streaming path in Holmes chat that converts supported graph/tool results into PNG bytes and emits them as SSE image events when streaming with render_graph_images enabled. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HolmesChat as "Holmes Chat"
participant RemoteAI as "Remote AI (SSE)"
participant StreamUtils as "Stream Utils"
Client->>HolmesChat: POST /holmes_chat (stream=true, render_graph_images=true)
HolmesChat->>RemoteAI: proxied streaming request
RemoteAI-->>HolmesChat: SSE lines (event:, data:)
HolmesChat->>StreamUtils: parse_sse_event_type / parse_sse_data
alt TOOL_RESULT containing graph/tool output
HolmesChat->>HolmesChat: get_png_from_graph_tool(data) → PNG bytes + name
HolmesChat->>Client: create_sse_message(TOOL_RESULT, {png_base64, name})
else other events
HolmesChat->>Client: pass-through/reformatted SSE event
end
RemoteAI-->>HolmesChat: ANSWER_END
HolmesChat->>Client: SSE ANSWER_END
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/robusta/core/stream/utils.py (1)
14-22: Consider simplifying the exception handling.The explicit
return Noneon line 21 inside the except block is redundant since the function returns None at the end anyway.Apply this diff to simplify:
def parse_sse_data(line: str): """Parse SSE data line and return parsed JSON or None""" if line.startswith("data: "): try: data = json.loads(line[6:].strip()) return data except json.JSONDecodeError: - return None + pass return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/robusta/core/playbooks/internal/ai_integration.py(18 hunks)src/robusta/core/stream/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
src/robusta/core/stream/utils.py
19-19: Consider moving this statement to an else block
(TRY300)
36-36: Possible hardcoded password assigned to: "TOKEN_COUNT"
(S105)
src/robusta/core/playbooks/internal/ai_integration.py
129-129: Probable use of requests call without timeout
(S113)
188-188: Probable use of requests call without timeout
(S113)
434-434: Probable use of requests call without timeout
(S113)
519-519: Probable use of requests call without timeout
(S113)
559-559: Probable use of requests call without timeout
(S113)
590-590: Unpacked variable name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: run_tests
🔇 Additional comments (8)
src/robusta/core/stream/utils.py (3)
5-11: LGTM!The SSE event type parsing logic is correct and handles the case where the prefix is not found.
25-26: LGTM!The SSE message creation follows the correct format with event type and JSON-serialized data.
29-37: LGTM!The enum correctly defines the streaming event types used throughout the Holmes integration. The static analysis warning about "TOKEN_COUNT" being a hardcoded password is a false positive—it's clearly an event type identifier.
src/robusta/core/playbooks/internal/ai_integration.py (5)
1-1: LGTM!The new imports support the streaming graph rendering feature:
base64for PNG encoding and the SSE utilities fromstream.utils. The reformatting improves readability.Also applies to: 21-23, 41-46, 48-50
64-83: LGTM!The reformatting of error handling to multi-line improves readability and maintains consistency across the file.
452-475: LGTM!The non-streaming graph rendering logic correctly filters Prometheus and Datadog metrics tools, converts them to PNGs using the new helper function, and handles exceptions gracefully.
603-619: LGTM!The function correctly converts graph tool results to PNG with appropriate fallback handling for invalid output types. The use of
.get()with defaults prevents KeyError issues.
430-446: Missing timeout parameter on Holmes API requests.The review comment is accurate. The
requests.postcalls at lines 434, 559, and throughout the file lack explicit timeout values, creating a risk of indefinite hangs if the Holmes service becomes unresponsive. This is a pre-existing issue not introduced by the PR, but present across:
- Line 114, 129, 188, 280, 371, 434, 448, 519, 559: All requests.post calls in this file
- Other integrations in the codebase (Prometheus, Supabase) properly configure timeouts using environment variables
Consider adding timeout configuration to Holmes requests. You may follow the pattern used elsewhere:
from robusta.core.model.env_vars import HOLMES_REQUEST_TIMEOUT_SECONDS # (if defined or create) requests.post(url, data=..., timeout=HOLMES_REQUEST_TIMEOUT_SECONDS)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_ai_integration.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_ai_integration.py (3)
src/robusta/core/model/base_params.py (1)
HolmesChatParams(192-203)src/robusta/core/model/events.py (3)
ExecutionBaseEvent(49-175)get_context(67-68)add_finding(129-138)src/robusta/core/stream/utils.py (2)
create_sse_message(25-26)StreamEvents(29-37)
🪛 Flake8 (7.3.0)
tests/test_ai_integration.py
[error] 4-4: 'unittest.mock.MagicMock' imported but unused
(F401)
🪛 Ruff (0.14.5)
tests/test_ai_integration.py
35-35: Unused method argument: chunk_size
(ARG002)
35-35: Unused method argument: decode_unicode
(ARG002)
182-182: Unpacked variable event_type is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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: run_tests
- GitHub Check: run_tests
🔇 Additional comments (1)
tests/test_ai_integration.py (1)
47-71: Well-structured test fixtures.Both
mock_eventandholmes_chat_paramsfixtures are properly configured for testing the streaming functionality with graph rendering enabled.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/robusta/core/playbooks/internal/ai_integration.py (1)
570-580: Fix potential IndexError from unsafe array access.Lines 571 and 580 access
event_lines[0]andevent_lines[1]without verifying the list length, which will cause an IndexError if the stream event is malformed or empty.Apply this diff to add bounds checking:
if stream_event: event_lines = stream_event.splitlines() + if len(event_lines) < 2: + event.ws(data=stream_event) + continue + event_type = parse_sse_event_type(event_lines[0]) if event_type == StreamEvents.ANSWER_END.value: stream_event = re.sub(r"<<.*?>>", "", stream_event) if event_type != StreamEvents.TOOL_RESULT.value: event.ws(data=stream_event) continue tool_res = parse_sse_data(event_lines[1])As flagged in past review comments.
🧹 Nitpick comments (1)
src/robusta/core/playbooks/internal/ai_integration.py (1)
590-590: Remove unused variable to clean up the code.The
namevariable is unpacked but never used. Replace it with_to indicate it's intentionally ignored.Apply this diff:
try: tool_data = json.loads(tool_res["result"]["data"]) - content, name = get_png_from_graph_tool(tool_data) + content, _ = get_png_from_graph_tool(tool_data) tool_res["result"]["data"] = base64.b64encode(content).decode(Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/robusta/core/playbooks/internal/ai_integration.py(18 hunks)tests/test_ai_integration.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/robusta/core/playbooks/internal/ai_integration.py (6)
src/robusta/core/playbooks/prometheus_enrichment_utils.py (1)
build_chart_from_prometheus_result(404-536)src/robusta/core/stream/utils.py (4)
create_sse_message(25-26)parse_sse_event_type(5-11)parse_sse_data(14-22)StreamEvents(29-37)src/robusta/utils/error_codes.py (2)
ActionException(42-47)ErrorCodes(5-40)src/robusta/core/model/base_params.py (3)
AIInvestigateParams(94-113)ResourceInfo(74-80)ChartValuesFormat(11-22)src/robusta/core/reporting/consts.py (2)
FindingSubjectType(42-72)from_kind(54-72)src/robusta/core/reporting/base.py (2)
add_enrichment(343-360)EnrichmentType(115-126)
tests/test_ai_integration.py (3)
src/robusta/core/model/base_params.py (1)
HolmesChatParams(192-203)src/robusta/core/playbooks/internal/ai_integration.py (1)
holmes_chat(408-497)src/robusta/core/stream/utils.py (2)
create_sse_message(25-26)StreamEvents(29-37)
🪛 Ruff (0.14.5)
src/robusta/core/playbooks/internal/ai_integration.py
129-129: Probable use of requests call without timeout
(S113)
188-188: Probable use of requests call without timeout
(S113)
434-434: Probable use of requests call without timeout
(S113)
519-519: Probable use of requests call without timeout
(S113)
559-559: Probable use of requests call without timeout
(S113)
590-590: Unpacked variable name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_ai_integration.py
35-35: Unused method argument: chunk_size
(ARG002)
35-35: Unused method argument: decode_unicode
(ARG002)
⏰ 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: run_tests
- GitHub Check: run_tests
🔇 Additional comments (10)
src/robusta/core/playbooks/internal/ai_integration.py (4)
1-52: LGTM on import changes.The new imports (base64, streaming utilities) support the streaming graph rendering feature, and the multiline formatting aligns with the ruff formatting mentioned in the PR description.
62-83: LGTM on error handling formatting.The multiline formatting for ActionException calls improves readability without changing logic.
320-349: LGTM on formatting improvements.The multiline formatting enhances readability without changing logic.
603-619: LGTM on graph-to-PNG conversion helper.The function safely extracts values with fallbacks, handles invalid output_type gracefully, and leverages existing chart-building utilities.
tests/test_ai_integration.py (6)
1-8: LGTM on imports.All imports are used appropriately in the test file.
11-21: LGTM on SSE message parser.The helper correctly extracts event type and JSON data from SSE-formatted strings.
24-44: LGTM on mock response class.The MockResponse class correctly implements the streaming interface. The unused
chunk_sizeanddecode_unicodeparameters initer_contentare intentional to match the realrequests.Responsesignature for proper mocking.
47-71: LGTM on test fixtures.Both fixtures appropriately configure the test environment, with
holmes_chat_paramsenabling streaming and graph rendering to exercise the new code paths.
74-137: LGTM on test data constants.The tool result fixtures provide realistic data structures for testing SSE streaming and graph rendering.
140-191: LGTM on the test implementation.The test correctly validates the streaming SSE workflow, including:
- Event count verification
- Pass-through of non-graph tool events
- PNG conversion of graph tool results
- Analysis string cleaning (regex removal of embedded tool references)
All past review concerns (assertions instead of assignments, unused variables) have been properly addressed.
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
looks good, small refactoring and comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/robusta/core/playbooks/internal/ai_integration.py (3)
131-133: Add timeout to requests call.This non-streaming
requests.postcall lacks a timeout parameter and can hang indefinitely.- result = requests.post( - f"{holmes_url}/api/investigate", data=holmes_req.json() - ) + result = requests.post( + f"{holmes_url}/api/investigate", data=holmes_req.json(), timeout=30 + )
559-565: Add timeout to prevent indefinite hanging.The
requests.postcall lacks a timeout parameter. For streaming requests, consider using a tuple timeout liketimeout=(connect_timeout, read_timeout)where read_timeout can be longer to accommodate streaming.with requests.post( url, data=holmes_req.json(), stream=True, + timeout=(10, 300), # 10s connect, 300s read for streaming headers={"Connection": "keep-alive"}, ) as resp:
436-441: Add timeout to streaming request.This
requests.postcall withstream=Truelacks a timeout parameter.with requests.post( url, data=holmes_req.json(), stream=True, + timeout=(10, 300), headers={"Connection": "keep-alive"}, ) as resp:
🧹 Nitpick comments (1)
src/robusta/core/playbooks/internal/ai_integration.py (1)
579-580: Consider adding.strip()for consistency with non-streaming path.The non-streaming path at line 463 calls
.strip()afterre.sub()to remove embedded tool calls, but this streaming path does not. This could leave trailing whitespace in the stream event.# removes all embedded tool calls in this case as its not supported. if event_type == StreamEvents.ANSWER_END.value: - stream_event = re.sub(r"<<.*?>>", "", stream_event) + stream_event = re.sub(r"<<.*?>>", "", stream_event).strip()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/core/playbooks/internal/ai_integration.py(18 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
src/robusta/core/playbooks/internal/ai_integration.py
131-131: Probable use of requests call without timeout
(S113)
190-190: Probable use of requests call without timeout
(S113)
436-436: Probable use of requests call without timeout
(S113)
520-520: Probable use of requests call without timeout
(S113)
560-560: Probable use of requests call without timeout
(S113)
⏰ 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: run_tests
- GitHub Check: run_tests
🔇 Additional comments (4)
src/robusta/core/playbooks/internal/ai_integration.py (4)
54-55: LGTM! GRAPH_TOOLS constant addresses reviewer feedback.The constant consolidates the supported graph tool names for reuse across the file, as suggested in previous review comments.
606-622: LGTM! Good defensive handling of output_type.The KeyError fallback to
ChartValuesFormat.Plainensures robustness when an invalid output_type string is received. The function is well-structured.
64-85: LGTM!The multi-line formatting for
ActionExceptionraises improves readability while maintaining the same error handling behavior.
591-603: LGTM! Exception handling provides adequate safety net.The try/except block appropriately catches any parsing failures (including missing keys) and falls back to sending the original stream event, ensuring the stream isn't interrupted by malformed data.
Theres some ruff formating on the file as well.