ROB-2323 loki free logql tool - #1106
Conversation
WalkthroughMoves Grafana Loki functionality into a nested grafana/loki package, adds a LokiQuery tool and instructions, updates imports to the new module path, and deletes the previous flat grafana/toolset_grafana_loki implementation and some related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toolset as GrafanaLokiToolset
participant LokiTool as LokiQuery
participant GrafanaAPI as Grafana/Loki API
Note over Toolset,LokiTool: New flow — Toolset provides config and wiring for LokiQuery
User->>Toolset: invoke Loki query (params)
Toolset->>LokiTool: forward (query, start, end, limit)
LokiTool->>GrafanaAPI: HTTP request (base_url + api_key) with LogQL & RFC3339 timestamps
GrafanaAPI-->>LokiTool: 200 + data / 200 empty / error
alt data returned
LokiTool-->>Toolset: StructuredToolResult(SUCCESS, JSON data)
else no data
LokiTool-->>Toolset: StructuredToolResult(NO_DATA)
else error
LokiTool-->>Toolset: StructuredToolResult(ERROR, message, diagnostic_url)
end
Toolset-->>User: return StructuredToolResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
84-107: Consider using ClassVar for class-level parameters dict.The
parametersdictionary is shared across all instances and should be annotated withtyping.ClassVarto clarify intent and satisfy type checkers.Apply this diff:
+from typing import Any, cast, Dict, ClassVar -from typing import Any, cast, Dict # ... existing code ... class LokiQuery(Tool): toolset: GrafanaLokiToolset name: str = "grafana_loki_query" description: str = "Run a query against Grafana Loki using LogQL query language." - parameters: Dict[str, ToolParameter] = { + parameters: ClassVar[Dict[str, ToolParameter]] = { "query": ToolParameter(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
holmes/plugins/toolsets/__init__.py(1 hunks)holmes/plugins/toolsets/grafana/loki/instructions.jinja2(1 hunks)holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py(1 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py(0 hunks)tests/plugins/toolsets/grafana/test_grafana_loki.py(1 hunks)
💤 Files with no reviewable changes (1)
- holmes/plugins/toolsets/grafana/toolset_grafana_loki.py
🧰 Additional context used
📓 Path-based instructions (4)
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/grafana/loki/instructions.jinja2holmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/plugins/toolsets/grafana/test_grafana_loki.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/plugins/toolsets/grafana/test_grafana_loki.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/plugins/toolsets/grafana/test_grafana_loki.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: robusta-dev/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T13:01:12.288Z
Learning: Applies to holmes/plugins/toolsets/**/* : Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
📚 Learning: 2025-05-15T05:13:43.169Z
Learnt from: nherment
Repo: robusta-dev/holmesgpt PR: 408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Applied to files:
holmes/plugins/toolsets/grafana/loki/instructions.jinja2
📚 Learning: 2025-10-01T06:01:13.215Z
Learnt from: mainred
Repo: robusta-dev/holmesgpt PR: 992
File: holmes/plugins/toolsets/__init__.py:106-109
Timestamp: 2025-10-01T06:01:13.215Z
Learning: In the holmesgpt repository (Python), function-scoped imports are acceptable when dealing with optional dependencies that may be missing on some platforms. For example, PrometheusToolset is imported inside the load_python_toolsets function to avoid import-time failures when DISABLE_PROMETHEUS_TOOLSET is true.
Applied to files:
tests/plugins/toolsets/grafana/test_grafana_loki.pyholmes/plugins/toolsets/__init__.py
📚 Learning: 2025-10-05T13:01:12.288Z
Learnt from: CR
Repo: robusta-dev/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T13:01:12.288Z
Learning: Applies to holmes/plugins/toolsets/**/* : Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Applied to files:
holmes/plugins/toolsets/__init__.py
🧬 Code graph analysis (3)
tests/plugins/toolsets/grafana/test_grafana_loki.py (2)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
GrafanaLokiConfig(39-40)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
FetchPodLogsParams(51-66)
holmes/plugins/toolsets/__init__.py (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
GrafanaLokiToolset(43-77)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (5)
holmes/core/tools.py (8)
CallablePrerequisite(521-522)Tool(173-365)ToolInvokeContext(164-170)ToolParameter(155-161)Toolset(534-778)StructuredToolResult(79-103)StructuredToolResultStatus(52-76)_load_llm_instructions(763-768)holmes/plugins/toolsets/grafana/common.py (2)
GrafanaConfig(9-22)get_base_url(52-56)holmes/plugins/toolsets/grafana/grafana_api.py (1)
grafana_health_check(20-42)holmes/plugins/toolsets/utils.py (3)
process_timestamps_to_rfc3339(75-87)standard_start_datetime_tool_param_description(10-11)toolset_name_for_one_liner(232-236)holmes/plugins/toolsets/grafana/loki_api.py (1)
execute_loki_query(37-62)
🪛 Ruff (0.14.3)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
84-107: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
112-112: Unused method argument: context
(ARG002)
141-141: Do not catch blind exception: Exception
(BLE001)
⏰ 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: build
🔇 Additional comments (3)
holmes/plugins/toolsets/__init__.py (1)
37-37: LGTM! Import path correctly updated.The import path change aligns with the toolset relocation to
holmes/plugins/toolsets/grafana/loki/and follows the coding guideline for toolset structure.tests/plugins/toolsets/grafana/test_grafana_loki.py (1)
7-11: LGTM! Import paths correctly updated.The import paths have been properly updated to reflect the new module location at
holmes.plugins.toolsets.grafana.loki.toolset_grafana_loki.holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
112-147: LGTM! Query execution logic is well-structured.The implementation correctly processes timestamps, executes the Loki query, and returns appropriate structured results with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
47-47: Fix typo in description.The word "quereis" should be "queries" as previously noted.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
84-107: Consider adding ClassVar annotation for class-level dict.The
parametersdictionary is a class attribute that's shared across instances. While the current implementation works, addingClassVarannotation would make the intent clearer and satisfy static analysis.Apply this diff:
+from typing import Any, cast, ClassVar, Dict -from typing import Any, cast, Dict class LokiQuery(Tool): toolset: GrafanaLokiToolset name: str = "grafana_loki_query" description: str = "Run a query against Grafana Loki using LogQL query language." - parameters: Dict[str, ToolParameter] = { + parameters: ClassVar[Dict[str, ToolParameter]] = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/grafana/loki/instructions.jinja2(1 hunks)holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/grafana/loki/instructions.jinja2
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: robusta-dev/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T13:01:12.288Z
Learning: Applies to holmes/plugins/toolsets/**/* : Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
🧬 Code graph analysis (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (5)
holmes/core/tools.py (6)
CallablePrerequisite(521-522)Tool(173-365)ToolInvokeContext(164-170)ToolParameter(155-161)StructuredToolResult(79-103)StructuredToolResultStatus(52-76)holmes/plugins/toolsets/grafana/common.py (2)
GrafanaConfig(9-22)get_base_url(52-56)holmes/plugins/toolsets/grafana/grafana_api.py (1)
grafana_health_check(20-42)holmes/plugins/toolsets/utils.py (3)
process_timestamps_to_rfc3339(75-87)standard_start_datetime_tool_param_description(10-11)toolset_name_for_one_liner(232-236)holmes/plugins/toolsets/grafana/loki_api.py (1)
execute_loki_query(37-62)
🪛 Ruff (0.14.3)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
84-107: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
112-112: Unused method argument: context
(ARG002)
141-141: Do not catch blind exception: Exception
(BLE001)
⏰ 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: build
🔇 Additional comments (2)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (2)
75-77: Respond to earlier question: Yes, this property is used.The
grafana_configproperty is accessed on line 119 within theLokiQuery._invokemethod:config = self.toolset.grafana_config. This property provides type-safe access to the properly typed config.
34-41: Verify whetherGrafanaLokiLabelsConfigis used or should be removed.The
labelsfield defined on line 40 is not accessed anywhere in the loki toolset implementation. While similar label configurations are actively used in other toolsets (datadog, opensearch, coralogix) to specify field names in queries, there is no evidence of this field being used within the loki toolset.Determine whether this configuration should be:
- Removed as unused code, or
- Retained and integrated into the Loki query construction (if LogQL query building needs customizable field names)
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
small changes needed, ping me after and ill re-approve
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
82-97: Reject missingqueryinstead of running a fake fallback.
queryis marked required, yet the code silently swaps in{query="no_query_fallback"}if it’s absent. That hides real user errors and can return misleading “no data” responses. Validate the parameter and return an explicit error when it’s missing or blank, then pass the validated value intoexecute_loki_query.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
52-75: MakeparametersaClassVar.As written, Pydantic treats
parametersas a mutable model field; any runtime mutation would bleed across instances and triggers RUF012. Annotate it as aClassVar(and keep the dict literal) so it’s excluded from instance state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py(1 hunks)tests/plugins/toolsets/grafana/test_grafana_loki.py(3 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) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.pytests/plugins/toolsets/grafana/test_grafana_loki.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/plugins/toolsets/grafana/test_grafana_loki.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/plugins/toolsets/grafana/test_grafana_loki.py
🧠 Learnings (2)
📚 Learning: 2025-10-05T13:01:12.288Z
Learnt from: CR
Repo: robusta-dev/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T13:01:12.288Z
Learning: Applies to holmes/plugins/toolsets/**/*.{yaml,yml} : All toolsets MUST return detailed error messages from underlying APIs, including the exact query/command, time ranges, parameters/filters, and full API error response; for "no data" responses, specify what was searched and where
Applied to files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
📚 Learning: 2025-10-01T06:01:13.215Z
Learnt from: mainred
Repo: robusta-dev/holmesgpt PR: 992
File: holmes/plugins/toolsets/__init__.py:106-109
Timestamp: 2025-10-01T06:01:13.215Z
Learning: In the holmesgpt repository (Python), function-scoped imports are acceptable when dealing with optional dependencies that may be missing on some platforms. For example, PrometheusToolset is imported inside the load_python_toolsets function to avoid import-time failures when DISABLE_PROMETHEUS_TOOLSET is true.
Applied to files:
tests/plugins/toolsets/grafana/test_grafana_loki.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (5)
holmes/core/tools.py (5)
Tool(173-365)ToolInvokeContext(164-170)ToolParameter(155-161)StructuredToolResult(79-103)StructuredToolResultStatus(52-76)holmes/plugins/toolsets/grafana/common.py (1)
get_base_url(52-56)holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
BaseGrafanaToolset(11-54)holmes/plugins/toolsets/utils.py (3)
process_timestamps_to_rfc3339(75-87)standard_start_datetime_tool_param_description(10-11)toolset_name_for_one_liner(232-236)holmes/plugins/toolsets/grafana/loki_api.py (1)
execute_loki_query(37-62)
tests/plugins/toolsets/grafana/test_grafana_loki.py (5)
holmes/core/tools.py (1)
ToolsetStatusEnum(137-140)holmes/plugins/toolsets/grafana/grafana_api.py (1)
grafana_health_check(20-42)holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
GrafanaLokiToolset(31-45)tests/plugins/toolsets/grafana/conftest.py (1)
check_grafana_connectivity(5-30)holmes/plugins/toolsets/grafana/common.py (1)
GrafanaConfig(9-22)
🪛 Ruff (0.14.3)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
52-75: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
80-80: Unused method argument: context
(ARG002)
109-109: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
LGTM, just verify evals 100-102, and do the small changes I wrote
|
@Avi-Robusta I will do the 100-102 evals on a differet PR with new evals for the tool |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
107-110: Return full query context in NO_DATA response.The NO_DATA response should include the complete query context (query string, time range, limit) so the LLM understands what was searched. Currently it only returns
params.Based on learnings
Apply this diff:
else: return StructuredToolResult( status=StructuredToolResultStatus.NO_DATA, params=params, + data={ + "message": "No data returned from Loki query", + "query": params.get("query", '{query="no_query_fallback"}'), + "time_range": {"start": start, "end": end}, + "limit": params.get("limit") or DEFAULT_LOG_LIMIT, + }, )
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (1)
80-80: Remove unusedcontextparameter or add type ignore.The
contextparameter is not used in the method body. Either remove it if not needed, or if it's required by the Tool interface, add# noqa: ARG002to suppress the linter warning.If the parameter is required by the interface but unused:
- def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: + def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: # noqa: ARG002
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:01:12.288Z
Learnt from: CR
Repo: robusta-dev/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T13:01:12.288Z
Learning: Applies to holmes/plugins/toolsets/**/*.{yaml,yml} : All toolsets MUST return detailed error messages from underlying APIs, including the exact query/command, time ranges, parameters/filters, and full API error response; for "no data" responses, specify what was searched and where
Applied to files:
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
🪛 Ruff (0.14.3)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py
52-75: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
80-80: Unused method argument: context
(ARG002)
111-111: Do not catch blind exception: Exception
(BLE001)
⏰ 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: build
🔇 Additional comments (4)
holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py (4)
1-28: LGTM!Imports are properly organized at the top of the file with appropriate type hints, following the coding guidelines.
31-45: LGTM!The toolset initialization is correct, properly extending
BaseGrafanaToolsetwith appropriate metadata and loading LLM instructions from the jinja template file.
77-78: LGTM!The one-liner formatting method is concise and correctly structured.
93-95: Consider type ignore for params.get("query") to fix build/test failures.A past review comment noted that build and tests are failing without a type ignore annotation here. Consider adding it as suggested.
Apply this diff if the type checker is complaining:
query=params.get( - "query", '{query="no_query_fallback"}' + "query", '{query="no_query_fallback"}' # type: ignore ), # make sure a string returns. fall back to query that will return nothing.
2 participants
added some minimal instructions