Repository navigation
Conversation
Signed-off-by: Codex <codex@openai.com>
WalkthroughThis pull request adds three optional parameters— Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant Tool as Grafana Tool (Get/Search)
participant API as Grafana API
participant Filter as Filter Engine (jsonpath/jq)
participant Trunc as Depth Truncator
rect rgb(230, 248, 255)
User->>Tool: invoke tool(max_depth?, jsonpath?, jq?)
Tool->>API: request dashboard(s)
API-->>Tool: return JSON payload
alt jsonpath or jq provided
Tool->>Filter: apply jsonpath/jq filters
Filter-->>Tool: filtered JSON (or error)
end
alt max_depth provided
Tool->>Trunc: truncate to max_depth
Trunc-->>Tool: truncated JSON
end
Tool-->>User: return resulting JSON (or error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (5)
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 (4)
docs/data-sources/builtin-toolsets/grafanadashboards.md (1)
61-69: LGTM! Clear and helpful documentation.The section clearly explains the three filtering controls and their execution order, which aligns with the implementation. The examples help users understand when to use each control.
Optional: Minor style refinement
Consider replacing "very big" with "large" for a more concise tone:
-Large dashboards can produce very big JSON payloads. To keep tool outputs readable you can: +Large dashboards can produce large JSON payloads. To keep tool outputs readable you can:This is a minor stylistic suggestion and entirely optional.
holmes/plugins/toolsets/grafana/toolset_grafana.py (2)
155-165: Consider including the filter expression in error messages.The error handling is solid, but per coding guidelines, toolsets should return detailed error messages to enable LLM self-correction. Include the actual expression that failed to help the LLM adjust its parameters.
As per coding guidelines, "All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including the exact query/command executed."
Proposed enhancement
def _apply_jsonpath_filter(data: Any, expression: str) -> Tuple[Optional[Any], Optional[str]]: try: jsonpath_expr = jsonpath_parse(expression) matches = [match.value for match in jsonpath_expr.find(data)] if len(matches) == 1: return matches[0], None return matches, None except Exception as exc: # pragma: no cover - defensive logger.debug("Failed to apply jsonpath filter", exc_info=exc) - return None, f"Invalid jsonpath expression: {exc}" + return None, f"Invalid jsonpath expression '{expression}': {exc}"
167-177: Consider including the filter expression in error messages.Same recommendation as for
_apply_jsonpath_filter: include the actual jq expression in the error message to help LLM self-correction.As per coding guidelines.
Proposed enhancement
def _apply_jq_filter(data: Any, expression: str) -> Tuple[Optional[Any], Optional[str]]: try: compiled = jq.compile(expression) results = compiled.input(data).all() if len(results) == 1: return results[0], None return results, None except Exception as exc: # pragma: no cover - defensive logger.debug("Failed to apply jq filter", exc_info=exc) - return None, f"Invalid jq filter: {exc}" + return None, f"Invalid jq filter '{expression}': {exc}"tests/plugins/toolsets/grafana/test_dashboard_filters.py (1)
12-22: Consider prefixing unused parameter with underscore.The helper correctly mocks
_make_grafana_request, but theendpointparameter is unused. Prefix it with an underscore to indicate it's intentionally unused and suppress the linter warning.Proposed refinement
- tool._make_grafana_request = lambda endpoint, params: StructuredToolResult( + tool._make_grafana_request = lambda _endpoint, params: StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, data=data, params=params, url="http://api", )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
docs/data-sources/builtin-toolsets/grafanadashboards.md(1 hunks)holmes/plugins/toolsets/grafana/toolset_grafana.py(5 hunks)pyproject.toml(1 hunks)tests/plugins/toolsets/grafana/test_dashboard_filters.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/grafana/toolset_grafana.pytests/plugins/toolsets/grafana/test_dashboard_filters.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**: Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/ directory structure
All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including the exact query/command executed, time ranges, parameters, filters used, and full API error response
Files:
holmes/plugins/toolsets/grafana/toolset_grafana.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Tests: Match source structure under tests/
Files:
tests/plugins/toolsets/grafana/test_dashboard_filters.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When writing documentation in the docs/ directory, always add a blank line between headers/bold text and lists for proper MkDocs rendering
Files:
docs/data-sources/builtin-toolsets/grafanadashboards.md
🧬 Code graph analysis (2)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
holmes/core/tools.py (3)
ToolParameter(157-163)StructuredToolResult(79-105)StructuredToolResultStatus(52-76)
tests/plugins/toolsets/grafana/test_dashboard_filters.py (2)
holmes/core/tools.py (2)
StructuredToolResult(79-105)StructuredToolResultStatus(52-76)holmes/plugins/toolsets/grafana/toolset_grafana.py (7)
GetDashboardByUID(292-360)_truncate_to_depth(132-152)_make_grafana_request(95-129)_invoke(234-286)_invoke(322-357)_invoke(388-424)_invoke(439-450)
🪛 LanguageTool
docs/data-sources/builtin-toolsets/grafanadashboards.md
[style] ~63-~63: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...onse size Large dashboards can produce very big JSON payloads. To keep tool outputs rea...
(EN_WEAK_ADJECTIVE)
🪛 Ruff (0.14.8)
holmes/plugins/toolsets/grafana/toolset_grafana.py
161-161: Consider moving this statement to an else block
(TRY300)
162-162: Do not catch blind exception: Exception
(BLE001)
173-173: Consider moving this statement to an else block
(TRY300)
174-174: Do not catch blind exception: Exception
(BLE001)
322-322: Unused method argument: context
(ARG002)
388-388: Unused method argument: context
(ARG002)
tests/plugins/toolsets/grafana/test_dashboard_filters.py
16-16: Unused lambda argument: endpoint
(ARG005)
⏰ 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: build
🔇 Additional comments (10)
holmes/plugins/toolsets/grafana/toolset_grafana.py (5)
21-25: LGTM! Imports properly placed.The new dependencies are correctly imported at the top of the file, and the logger follows standard Python conventions.
132-152: LGTM! Truncation logic is correct.The depth truncation helper properly handles edge cases including
max_depth=0, empty collections, and deeply nested structures. The recursive approach cleanly separates concerns by incrementing depth at each level.
304-318: LGTM! Well-defined optional parameters.The three new parameters are clearly described, appropriately typed, and optional to maintain backward compatibility. The descriptions align with the user documentation.
322-357: LGTM! Filtering and truncation logic correctly implemented.The implementation properly applies filters in the documented order (jsonpath → jq → depth), handles errors gracefully by returning ERROR status, and maintains the filtered data through the pipeline. The unused
contextparameter flagged by Ruff is required by theToolinterface.
369-424: LGTM! Consistent implementation with GetDashboardByUID.The filtering and truncation logic mirrors
GetDashboardByUID, maintaining consistency across the toolset. Line 414 correctly uses the originalresult.datato extract the UID for URL building, since filtered data might not contain this field.tests/plugins/toolsets/grafana/test_dashboard_filters.py (4)
25-29: LGTM! Truncation test covers key scenarios.The test validates truncation for both dictionaries and lists, ensuring the function handles mixed nested structures correctly.
32-41: LGTM! JSONPath filter test validates extraction.The test correctly verifies that JSONPath filtering extracts specific fields before returning data, and properly expects a single value when only one match is found.
44-51: LGTM! JQ filter test validates extraction.The test correctly verifies jq filtering functionality and the single-value unwrapping behavior, maintaining consistency with the JSONPath test.
54-61: LGTM! Error handling test validates invalid input.The test ensures malformed JSONPath expressions result in an ERROR status with an appropriate message, validating the defensive error handling.
pyproject.toml (1)
58-59: Bothjqandjsonpath-ngare current and secure. jq version 1.10.0 was released July 14, 2025, making the^1.8.0constraint compatible with the latest version. jsonpath-ng 1.7.0 was released October 11, 2024, compatible with^1.6.1. The jq package was scanned for known vulnerabilities and missing license, and no issues were found. The jsonpath-ng package was scanned for known vulnerabilities and missing license, and no issues were found. Both packages are actively maintained with healthy release cadences.
| def test_depth_applies_after_filters(): | ||
| data = {"dashboard": {"panels": [{"id": 1, "title": "CPU"}]}} | ||
| tool = _build_tool(data) | ||
|
|
||
| result = tool._invoke({"uid": "abc", "max_depth": 1}, context=None) | ||
|
|
||
| assert result.data["dashboard"] == "...truncated at depth 1" |
There was a problem hiding this comment.
Test name doesn't match implementation.
The test name suggests it verifies that depth truncation happens after filters, but it only tests depth truncation without any filters. To properly test the execution order, the test should apply both a filter and depth truncation.
Proposed fix
Either rename the test to match what it actually tests:
-def test_depth_applies_after_filters():
+def test_depth_truncates_nested_structures():
data = {"dashboard": {"panels": [{"id": 1, "title": "CPU"}]}}
tool = _build_tool(data)
result = tool._invoke({"uid": "abc", "max_depth": 1}, context=None)
assert result.data["dashboard"] == "...truncated at depth 1"Or enhance the test to actually verify filter→depth order:
def test_depth_applies_after_filters():
data = {"dashboard": {"panels": [{"id": 1, "title": "CPU"}]}}
tool = _build_tool(data)
- result = tool._invoke({"uid": "abc", "max_depth": 1}, context=None)
+ # Apply jsonpath filter first to extract dashboard, then truncate to depth 1
+ result = tool._invoke({"uid": "abc", "jsonpath": "$.dashboard", "max_depth": 1}, context=None)
- assert result.data["dashboard"] == "...truncated at depth 1"
+ # Verify the filtered result (just dashboard) is then truncated at depth 1
+ assert result.data["panels"] == "...truncated at depth 1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_depth_applies_after_filters(): | |
| data = {"dashboard": {"panels": [{"id": 1, "title": "CPU"}]}} | |
| tool = _build_tool(data) | |
| result = tool._invoke({"uid": "abc", "max_depth": 1}, context=None) | |
| assert result.data["dashboard"] == "...truncated at depth 1" | |
| def test_depth_truncates_nested_structures(): | |
| data = {"dashboard": {"panels": [{"id": 1, "title": "CPU"}]}} | |
| tool = _build_tool(data) | |
| result = tool._invoke({"uid": "abc", "max_depth": 1}, context=None) | |
| assert result.data["dashboard"] == "...truncated at depth 1" |
🤖 Prompt for AI Agents
In tests/plugins/toolsets/grafana/test_dashboard_filters.py around lines 64 to
70, the test name claims it verifies that depth truncation occurs after filters,
but the implementation only exercises depth truncation with no filters; either
rename the test to reflect it only checks depth truncation, or modify the test
to apply a filter and a max_depth and assert the filter is applied first and
then the result is truncated—specifically: build tool with data that would be
affected by a filter, invoke tool._invoke with both filter criteria and
max_depth, and assert that the filtered content appears and then the dashboard
is "...truncated at depth 1" (or equivalent) to prove filter→depth ordering.
…-tool-to-handle-large
Results of HolmesGPT evalsDuration: 4m 3s | View workflow logs Results of HolmesGPT evals
Legend
🔄 Re-run evals manuallyOption 1: Comment on this PR with Or with options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d569495
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d569495 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d569495
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d569495Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:d569495Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:d569495 |
|
/eval |
🧪 Manual Eval Results
|
|
/eval |
🧪 Manual Eval Results
|
|
/eval |
…-tool-to-handle-large
|
/eval |
✅ Results of HolmesGPT evalsAutomatically triggered by commit f5af1d4 Results of HolmesGPT evals
Legend
📖 Legend🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
🧪 Manual Eval Results
Results of HolmesGPT evals
Legend
|
|
@aantn Your eval run has finished. ✅ Completed successfully 👆 See updated results above. |
…-tool-to-handle-large
✅ Results of HolmesGPT evalsAutomatically triggered by commit 9e32f73 Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
|
/eval |
🧪 Manual Eval Results
Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
|
@aantn Your eval run has finished. ✅ Completed successfully 👆 See updated results above. |
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.