Repository navigation
[ROB-3973] Multi instance grafana dashboard support - #2080
Merged
Merged
Conversation
`grafana/dashboards`, `grafana/loki`, and `grafana/tempo` now accept an `instances` list and per-instance `username`/`password` basic-auth, in addition to the existing single-instance + `api_key` shape (legacy configs keep working unchanged). Top-level credentials act as global defaults inherited by any instance that doesn't override them. Tools take an optional `grafana_instance` parameter that the LLM uses to pick a target; with a single instance configured it's auto-selected. Health checks are tolerant — partial failures don't disable the toolset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Loki and Tempo revert to the master single-instance shape; the multi-instance `instances` list and HTTP Basic auth fields now live on a new MultiInstanceGrafanaConfig + BaseMultiInstanceGrafanaToolset pair used only by grafana/dashboards. Signed-off-by: avi@robusta.dev <avi@robusta.dev> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Avi-Robusta
changed the title
Contributor
📂 Previous Runs📜 #2 · Run @ __7c042b4__ (#26402881053) — May 25, 13:35 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 7c042b4 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsMaster baseline: latest master-* experiment (post-merge regression eval)
Benchmark baseline: latest ci-benchmark experiment on master
Time comparison (seconds):
Cost comparison:
Total tokens comparison:
Cached tokens comparison:
Turns comparison:
Tool calls comparison:
Comparison indicators:
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 35.9s | 5 | 10 | $0.2668 | 105,477 | 103,341 | 23,460 | 2,136 | 974 | 79,307 | 24,034 | 243 | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 80.2s | 8 | 20 | $0.4325 | 197,118 | 192,160 | 29,018 | 4,958 | 929 | 161,737 | 30,423 | 864 | — | src |
| ✅ | 112_find_pvcs_by_uuid | 20.3s | 3 | 4 | $0.2057 | 61,174 | 59,976 | 21,898 | 1,198 | 622 | 37,823 | 22,153 | 295 | — | src |
| ✅ | 12_job_crashing | 62.7s | 8 | 17 | $0.3684 | 187,366 | 183,958 | 26,534 | 3,408 | 858 | 156,283 | 27,675 | 500 | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 53.8s | 5 | 14 | $0.3264 | 115,061 | 111,782 | 26,593 | 3,279 | 1,299 | 83,876 | 27,906 | 640 | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 20.2s | 4 | 9 | $0.2079 | 76,701 | 75,573 | 20,660 | 1,128 | 594 | 53,979 | 21,594 | 53 | — | src |
| ✅ | 243_pod_names_contain_service | 34.4s | 5 | 9 | $0.2541 | 101,866 | 99,939 | 22,351 | 1,927 | 890 | 76,633 | 23,306 | 138 | — | src |
| ✅ | 24_misconfigured_pvc | 51.7s | 6 | 15 | $0.3337 | 134,687 | 131,409 | 25,263 | 3,278 | 1,061 | 103,829 | 27,580 | 550 | — | src |
| ✅ | 43_current_datetime_from_prompt | 4.2s | 1 | — | $0.0121 | 16,999 | 16,899 | 16,899 | 100 | 100 | 16,896 | 3 | 60 | — | src |
| ✅ | 51_logs_summarize_errors | 22.8s | 4 | 5 | $0.2032 | 76,611 | 75,480 | 20,639 | 1,131 | 476 | 54,836 | 20,644 | 32 | — | src |
| ✅ | 61_exact_match_counting | 11.3s | 3 | 3 | $0.1518 | 52,728 | 52,366 | 17,876 | 362 | 215 | 34,486 | 17,880 | 31 | — | src |
| Total | 36.1s avg | 4.7 avg | 10.6 avg | $2.7624 | 1,125,788 | 1,102,883 | 29,018 | 22,905 | 1,299 | 859,685 | 243,198 | 3,406 | — |
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 11 test/model combinations loaded
- master-26436808081 (created: 2026-05-26)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 187 test/model combinations loaded
- ci-benchmark-26358788343 (created: 2026-05-24)
Time comparison (seconds):
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 35.9s | 32.7s | ±0% | 37.6s | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 80.2s | 78.3s | ±0% | 73.0s | ±0% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 20.3s | 17.6s | ↑16% | 20.9s | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 62.7s | 47.4s | ↑32% | 45.3s | ↑38% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 53.8s | 62.7s | ↓14% | 45.8s | ↑18% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 20.2s | 19.8s | ±0% | 20.0s | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 34.4s | 39.5s | ↓13% | 41.1s | ↓16% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 51.7s | 43.8s | ↑18% | 38.4s | ↑35% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 4.2s | 3.8s | ±0% | 3.5s | ↑18% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 22.8s | 21.7s | ±0% | 22.3s | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 11.3s | 10.6s | ±0% | 10.3s | ±0% |
| Total (all, n=11) | 36.1s | 34.3s | — | 32.6s | — |
| Comparable (m=11, b=11) | 36.1s | 34.3s | ±0% | 32.6s | ↑11% |
Cost comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2668 | $0.2583 | ±0% | $0.2799 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.4325 | $0.4138 | ±0% | $0.4212 | ±0% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.2057 | $0.1900 | ±0% | $0.2055 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | $0.3684 | $0.3142 | ↑17% | $0.3155 | ↑17% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.3264 | $0.3862 | ↓15% | $0.3301 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.2079 | $0.2031 | ±0% | $0.2055 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.2541 | $0.2729 | ±0% | $0.2887 | ↓12% |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.3337 | $0.3010 | ↑11% | $0.2854 | ↑17% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.0121 | $0.1189 | ↓90% | $0.1182 | ↓90% |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.2032 | $0.2040 | ±0% | $0.2052 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1518 | $0.1518 | ±0% | $0.1511 | ±0% |
| Total (all, n=11) | $0.2511 | $0.2558 | — | $0.2551 | — |
| Comparable (m=11, b=11) | $0.2511 | $0.2558 | ±0% | $0.2551 | ±0% |
Total tokens comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 105,477 | 84,961 | ↑24% | 106,269 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 197,118 | 171,846 | ↑15% | 171,838 | ↑15% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 61,174 | 58,072 | ±0% | 60,885 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 187,366 | 154,948 | ↑21% | 134,896 | ↑39% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 115,061 | 172,978 | ↓33% | 138,789 | ↓17% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 76,701 | 76,661 | ±0% | 76,302 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 101,866 | 105,543 | ±0% | 126,664 | ↓20% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 134,687 | 130,101 | ±0% | 107,150 | ↑26% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 16,999 | 16,999 | ±0% | 16,898 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 76,611 | 76,989 | ±0% | 77,029 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 52,728 | 52,725 | ±0% | 52,431 | ±0% |
| Total (all, n=11) | 102,344 | 100,166 | — | 97,196 | — |
| Comparable (m=11, b=11) | 102,344 | 100,166 | ±0% | 97,196 | ±0% |
Cached tokens comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 79,307 | 58,055 | ↑37% | 78,682 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 161,737 | 135,478 | ↑19% | 134,429 | ↑20% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 37,823 | 36,442 | ±0% | 37,612 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 156,283 | 126,891 | ↑23% | 105,026 | ↑49% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 83,876 | 139,279 | ↓40% | 108,400 | ↓23% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 53,979 | 54,887 | ±0% | 53,987 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 76,633 | 79,165 | ±0% | 100,135 | ↓23% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 103,829 | 101,761 | ±0% | 79,251 | ↑31% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 16,896 | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 54,836 | 55,033 | ±0% | 54,949 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 34,486 | 34,484 | ±0% | 34,287 | ±0% |
| Total (all, n=11) | 78,153 | 74,680 | — | 71,523 | — |
| Comparable (m=10, b=10) | 84,279 | 82,148 | ±0% | 78,676 | ±0% |
Turns comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 5 | 4 | ↑25% | 5 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 8 | 7 | ↑14% | 7 | ↑14% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 8 | 7 | ↑14% | 6 | ↑33% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 7 | ↓29% | 6 | ↓17% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 5 | 5 | ±0% | 6 | ↓17% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 6 | 6 | ±0% | 5 | ↑20% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | 1 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| Total (all, n=11) | 4.7 | 4.6 | — | 4.5 | — |
| Comparable (m=11, b=11) | 4.7 | 4.6 | ±0% | 4.5 | ±0% |
Tool calls comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 10 | 10 | ±0% | 11 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 20 | 17 | ↑18% | 17 | ↑18% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 17 | 13 | ↑31% | 14 | ↑21% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 14 | 16 | ↓12% | 15 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 9 | 9 | ±0% | 9 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 9 | 11 | ↓18% | 11 | ↓18% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 15 | 14 | ±0% | 14 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 5 | 5 | ±0% | 5 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| Total (all, n=11) | 9.6 | 10.2 | — | 10.3 | — |
| Comparable (m=10, b=10) | 10.6 | 10.2 | ±0% | 10.3 | ±0% |
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref multi-instance-grafana -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, conversation_worker, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, manual, mcp, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, skills, slackbot, storage, token-limit, toolset-limitation, traces, transparency, victorialogs
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref multi-instance-grafana -f markers=regression -f filter=
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:1390304c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:1390304c me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:1390304c
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:1390304c
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:1390304c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:1390304c me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:1390304c
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:1390304cPatch 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:1390304c \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:1390304cRobusta 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:1390304c \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:1390304c |
Contributor
WalkthroughThis PR extends the Grafana toolset to support configuring and routing requests to multiple Grafana instances, with per-instance authentication/overrides, aggregated health checks, instance-aware request/render helpers, tool parameter threading, tests, and documentation. ChangesGrafana multi-instance support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/plugins/toolsets/grafana/test_grafana_multi_instance.py (2)
101-103: ⚡ Quick winMove function-local imports to module scope.
These imports should be hoisted to the top-level import block to keep import behavior consistent and deterministic.
Proposed diff
import base64 +import logging from unittest.mock import MagicMock import pytest import requests import responses from pydantic import ValidationError from requests.auth import HTTPBasicAuth @@ -from holmes.plugins.toolsets.grafana.toolset_grafana import GrafanaToolset +from holmes.plugins.toolsets.grafana.toolset_grafana import ( + GrafanaDashboardConfig, + GrafanaToolset, +) @@ def test_top_level_api_url_with_instances_logs_warning(self, caplog): - import logging with caplog.at_level(logging.WARNING): GrafanaConfig( api_url="http://ignored", instances=[{"name": "real", "api_url": "http://real"}], ) @@ def _toolset_with(**config) -> GrafanaToolset: """Build a GrafanaToolset with state populated directly (no network probe).""" - from holmes.plugins.toolsets.grafana.toolset_grafana import GrafanaDashboardConfig - ts = GrafanaToolset() ts._grafana_config = GrafanaDashboardConfig(**config)As per coding guidelines:
**/*.py: Always place Python imports at the top of the file, not inside functions or methods.Also applies to: 174-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugins/toolsets/grafana/test_grafana_multi_instance.py` around lines 101 - 103, The test contains function-local imports (logging and possibly others) in test_top_level_api_url_with_instances_logs_warning and similarly at lines around 174-177; move those imports to module scope by hoisting "import logging" (and any other imports used only inside tests at those locations, e.g., caplog-related imports if present) to the top of the file's import block so imports are consistent and deterministic—update the top-of-file imports and remove the in-function import statements in the test functions (reference functions: test_top_level_api_url_with_instances_logs_warning and the other test at ~174-177).
174-174: ⚡ Quick winAnnotate
_toolset_withkwargs for typing compliance.Please type
**configto satisfy the repository typing requirement for Python code.Proposed diff
+from typing import Any @@ -def _toolset_with(**config) -> GrafanaToolset: +def _toolset_with(**config: Any) -> GrafanaToolset:As per coding guidelines:
**/*.py: Type hints required (mypy configuration in pyproject.toml).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugins/toolsets/grafana/test_grafana_multi_instance.py` at line 174, Annotate the var-kwargs on the helper by adding a typing for **config (e.g., **config: Any) and import Any from typing; update the signature of _toolset_with to use that annotated **config so it satisfies mypy/type-hint rules while preserving the GrafanaToolset return type and existing behavior.tests/plugins/toolsets/test_verify_tool_urls.py (1)
421-421: ⚡ Quick winAdd type hints to the updated mock helper signature.
The new multi-instance-aware mock function should be annotated to match the repository typing rule.
Proposed diff
+from typing import Any @@ - def mock_make_request(instance, endpoint, params, query_params=None, timeout=30): + def mock_make_request( + instance: Any, + endpoint: str, + params: dict[str, Any], + query_params: dict[str, Any] | None = None, + timeout: int = 30, + ) -> MagicMock:As per coding guidelines:
**/*.py: Type hints required (mypy configuration in pyproject.toml).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugins/toolsets/test_verify_tool_urls.py` at line 421, The new multi-instance-aware mock function mock_make_request lacks type annotations; update its signature to include typing (e.g., import and use Optional, Dict, Any, and int) so parameters like instance, endpoint, params, query_params: Optional[Dict[str, Any]] = None, timeout: int = 30 are annotated and the return type is specified (e.g., -> Any or -> Dict[str, Any]) to satisfy the repository mypy rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/data-sources/builtin-toolsets/grafanadashboards.md`:
- Around line 196-204: The fenced YAML code block under the Grafana dashboards
subsection triggers markdownlint rule MD046; replace the triple-backtick fenced
block with an indented code block by removing the backticks and prefixing each
YAML line with the correct indentation for the subsection so the snippet
beginning with the toolsets: grafana/dashboards lines becomes an indented code
block; ensure the block content (toolsets:, grafana/dashboards:, enabled:,
config:, api_url:, username:, password:) retains relative indentation so
markdown renders the same while satisfying MD046.
In `@holmes/plugins/toolsets/grafana/common.py`:
- Around line 372-376: The top-level auth fields (api_key, username, password)
must be validated the same way as per-instance auth to avoid silently preferring
api_key; update _normalize_and_resolve_globals() to detect and reject mixed
top-level credentials when instances is set by ensuring exactly one auth method
is provided: either api_key alone OR username and password together (not api_key
plus username/password, and not incomplete basic auth), and raise a clear
validation/error when the combination is invalid; apply the same validation
logic you use for per-instance entries to the top-level api_key, username,
password handling.
In `@holmes/plugins/toolsets/grafana/toolset_grafana.py`:
- Around line 196-205: health_check currently calls tool._make_grafana_request
and only catches raw exceptions; update health_check to use the structured
response returned by _make_grafana_request (inspect result.status and
result.body/response) instead of relying on exceptions, and when building
failures include detailed context: instance.name, the resolved request URL,
query params, HTTP status code and response body/message; use those fields to
construct the failure strings passed to _aggregate_health_results; apply the
same pattern to the other dashboard tools that call _make_grafana_request in the
225-270 region so all callers consume result.status and include full API error
details for LLM self-correction.
- Around line 120-130: The probe currently only checks the first instance
(first_instance) when grafana_config.enable_rendering, so render tools may be
skipped if a later instance supports the image renderer; update the logic in the
block that calls get_base_url(first_instance) and
self._try_add_render_tools(first_instance) to iterate over all entries in
self._instances (e.g., for instance in self._instances.values()), probe each
instance (log with get_base_url(instance)), call
self._try_add_render_tools(instance) for each, and stop early if the tools
(self.tools or specific grafana_render_panel/grafana_render_dashboard) have been
successfully added to avoid redundant probes.
---
Nitpick comments:
In `@tests/plugins/toolsets/grafana/test_grafana_multi_instance.py`:
- Around line 101-103: The test contains function-local imports (logging and
possibly others) in test_top_level_api_url_with_instances_logs_warning and
similarly at lines around 174-177; move those imports to module scope by
hoisting "import logging" (and any other imports used only inside tests at those
locations, e.g., caplog-related imports if present) to the top of the file's
import block so imports are consistent and deterministic—update the top-of-file
imports and remove the in-function import statements in the test functions
(reference functions: test_top_level_api_url_with_instances_logs_warning and the
other test at ~174-177).
- Line 174: Annotate the var-kwargs on the helper by adding a typing for
**config (e.g., **config: Any) and import Any from typing; update the signature
of _toolset_with to use that annotated **config so it satisfies mypy/type-hint
rules while preserving the GrafanaToolset return type and existing behavior.
In `@tests/plugins/toolsets/test_verify_tool_urls.py`:
- Line 421: The new multi-instance-aware mock function mock_make_request lacks
type annotations; update its signature to include typing (e.g., import and use
Optional, Dict, Any, and int) so parameters like instance, endpoint, params,
query_params: Optional[Dict[str, Any]] = None, timeout: int = 30 are annotated
and the return type is specified (e.g., -> Any or -> Dict[str, Any]) to satisfy
the repository mypy rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c905529-f0eb-44d0-84ae-a3adcf3c99bf
📒 Files selected for processing (7)
docs/data-sources/builtin-toolsets/grafanadashboards.mdholmes/plugins/toolsets/grafana/base_grafana_toolset.pyholmes/plugins/toolsets/grafana/common.pyholmes/plugins/toolsets/grafana/toolset_grafana.pytests/plugins/toolsets/grafana/test_grafana_multi_instance.pytests/plugins/toolsets/test_json_filter_mixin.pytests/plugins/toolsets/test_verify_tool_urls.py
The LLM only sees `StructuredToolResult.data`, not `.url`, so dashboard links were invisible to it. Inject `grafana_url` into the data payload in the four dashboard tools (search, get-by-uid, get-home, get-tags) so the model can cite the correct instance link in its responses. Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
642-667: ⚡ Quick winInclude instance name and response body in render error messages.
The error messages don't identify which Grafana instance failed, which is important in multi-instance setups. Additionally, the
HTTPErrorhandler omits the response body that often contains actionable error details.♻️ Suggested improvement
except requests.HTTPError as e: status_code = ( e.response.status_code if e.response is not None else "unknown" ) + response_text = e.response.text[:500] if e.response is not None else "" query_string = urlencode(query_params, doseq=True) return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=f"Grafana render API returned HTTP {status_code}: {e}. " + error=f"[{instance.name}] Grafana render API returned HTTP {status_code}. " f"Render path: {render_path}?{query_string}. " - f"Ensure the grafana-image-renderer plugin is installed and running.", + f"Response: {response_text}. " + f"Ensure the grafana-image-renderer plugin is installed and running." if not response_text else "", params=params, ) except requests.ConnectionError as e: return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=f"Failed to connect to Grafana render API at {render_path}: {e}", + error=f"[{instance.name}] Failed to connect to Grafana render API at {render_path}: {e}", params=params, ) except requests.Timeout: query_string = urlencode(query_params, doseq=True) return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=f"Grafana render request timed out for {render_path}?{query_string}. " + error=f"[{instance.name}] Grafana render request timed out for {render_path}?{query_string}. " f"The panel may be too complex or the renderer is overloaded.", params=params, )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, time ranges/parameters/filters used, and full API error response (status code and message)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holmes/plugins/toolsets/grafana/toolset_grafana.py` around lines 642 - 667, Include the Grafana instance identifier and the full HTTP response body in the render error messages: in the requests.HTTPError handler (where render_path, query_params, params and StructuredToolResult are used) append the instance identifier that was used to build render_path (e.g., grafana_url or self.instance_name) and, when e.response is not None, include e.response.status_code and the response body text (e.response.text) in the error string; do the same for the ConnectionError and Timeout handlers (include the same instance identifier and the exception message/response body when available) so all StructuredToolResult.error messages contain instance, exact render path+query, and API response details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@holmes/plugins/toolsets/grafana/toolset_grafana.py`:
- Around line 642-667: Include the Grafana instance identifier and the full HTTP
response body in the render error messages: in the requests.HTTPError handler
(where render_path, query_params, params and StructuredToolResult are used)
append the instance identifier that was used to build render_path (e.g.,
grafana_url or self.instance_name) and, when e.response is not None, include
e.response.status_code and the response body text (e.response.text) in the error
string; do the same for the ConnectionError and Timeout handlers (include the
same instance identifier and the exception message/response body when available)
so all StructuredToolResult.error messages contain instance, exact render
path+query, and API response details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2eeda0e-9a11-466d-a379-13a48dc8952c
📒 Files selected for processing (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py
In a heterogeneous multi-instance setup, only the first configured Grafana was probed for the image-renderer plugin. If that instance lacked the plugin, `grafana_render_panel` and `grafana_render_dashboard` were never registered even when a later instance had it. Loop through all instances and stop as soon as one exposes the renderer. Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Two assertions in test_json_filter_mixin still expected the raw filter output, but the GetDashboardByUID tool now wraps results with the Grafana UI URL (added in 7c1d19b "Expose Grafana UI URL in tool result data for LLM visibility"). Update the expected payloads to match the wrapped shape: dicts get `grafana_url` merged in; non-dict jq outputs become `{grafana_url, results}`. Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- common.py: Apply XOR auth validation to top-level credentials. Rejects mixed `api_key` + `username/password` and incomplete basic auth so config errors surface up front instead of api_key silently winning during the per-instance fall-through. - toolset_grafana.py: `_make_grafana_request` now returns structured ERROR results for HTTPError/Timeout/ConnectionError, prefixed with `[instance.name]` and including the resolved URL+query and HTTP response body — required for LLM self-correction per CLAUDE.md. `health_check` consumes the structured `result.status` instead of catching raw exceptions, so per-instance failures propagate with full context. - toolset_grafana.py: Render-tool errors (HTTPError/ConnectionError/ Timeout) gain the same `[instance.name]` prefix and include response body (first 500 chars) on HTTP failures. - tests: hoist function-local `import logging` / `GrafanaDashboardConfig` in test_grafana_multi_instance.py to module scope. Annotate `_toolset_with(**config: Any)` and the `mock_make_request` helper in test_verify_tool_urls.py for mypy. - grafanadashboards.md: convert the username/password top-level fenced YAML block to an indented code block to satisfy markdownlint MD046. Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/data-sources/builtin-toolsets/grafanadashboards.md (2)
107-111:⚠️ Potential issue | 🟠 MajorFix invalid
grafana_list_instancesreference in Grafana multi-instance docs
docs/data-sources/builtin-toolsets/grafanadashboards.md(lines 109 and 190) tells users to rungrafana_list_instancesto see configured instance names, but the Grafana toolset does not implement anygrafana_list_instancestool/function. Instead, instance names come fromtoolsets.grafana/dashboards.config.instances[].name(and unknowngrafana_instanceerrors list configured names); update the doc to remove/replace thegrafana_list_instancesguidance, and rely on the toolset status string for the health-check note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/data-sources/builtin-toolsets/grafanadashboards.md` around lines 107 - 111, Replace the invalid grafana_list_instances reference in the Grafana multi-instance docs: remove the instruction to run grafana_list_instances and instead tell users to read instance names from toolsets.grafana/dashboards.config.instances[].name (and note that unknown grafana_instance errors will list configured names), and update the health-check sentence to refer to the toolset status string rather than the nonexistent function.
190-190:⚠️ Potential issue | 🟠 MajorFix “toolset status string” wording (and the
grafana_list_instancesreference) in Grafana dashboards docs
- There is no user-facing concept named “toolset status string” in the code; toolset health is surfaced via
pretty_print_toolset_status()(status/error table) and cached intoolsets_status.json(especially theerrorfield).- For the Grafana multi-instance tolerant health check, when at least one instance is reachable
health_check()returns(True, ""), so unreachable-instance details won’t appear in the status/error table (they’re emitted in the warning log instead). The joined failure details populateToolset.erroronly when all instances fail.grafana_list_instancesis only referenced in the docs and isn’t present in the repo as an actual tool/command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/data-sources/builtin-toolsets/grafanadashboards.md` at line 190, Update the Grafana dashboards doc wording to reference actual code artifacts: replace “toolset status string” with the status/error table produced by pretty_print_toolset_status() and the cached status in toolsets_status.json (not a generic string); clarify that health_check() for the multi-instance tolerant check returns (True, "") when at least one instance is reachable so unreachable-instance details are emitted to warnings (and only set Toolset.error when all instances fail); and remove or mark as inaccurate the reference to grafana_list_instances since that command/tool does not exist in the repo.
🧹 Nitpick comments (2)
docs/data-sources/builtin-toolsets/grafanadashboards.md (2)
192-202: 💤 Low valueConsider adding deployment tabs for consistency.
This subsection shows a single code snippet without the tab structure (Holmes CLI / Holmes Helm Chart / Robusta Helm Chart) used throughout the rest of the documentation. For consistency with the rest of the file and to help users across different deployment methods, consider restructuring this subsection to use the same tab pattern.
Additionally, this subsection is nested under "Multiple Grafana Instances" but describes single-instance usage. Consider whether it should be:
- A sibling section to "Multiple Grafana Instances" (e.g., "## Alternative Authentication Methods"), or
- Moved to the main Configuration section as an authentication option
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/data-sources/builtin-toolsets/grafanadashboards.md` around lines 192 - 202, The "Username/password authentication (single instance)" subsection currently lacks the deployment-tabs pattern and sits under "Multiple Grafana Instances"; update it to match other docs by adding the Holmes CLI / Holmes Helm Chart / Robusta Helm Chart tab structure around the example (same tab names and ordering used elsewhere) and either move this subsection out from under "Multiple Grafana Instances" into a sibling section (e.g., "Alternative Authentication Methods") or relocate it into the main Configuration section as an authentication option so the heading and placement reflect single-instance usage; target the "Username/password authentication (single instance)" heading and the surrounding "Multiple Grafana Instances" section when making the change.
167-188: ⚡ Quick winAdd staging instance example to Robusta tab for consistency.
The Holmes CLI and Helm Chart examples show three instances including a
staginginstance with anapi_keyoverride (lines 130-133, 162-164), but the Robusta example only shows two instances. For consistency and to demonstrate per-instance credential overrides across all deployment methods, add the staging instance to the Robusta example.📝 Proposed addition
instances: - name: prod-eu api_url: https://grafana.eu-west-1.internal - name: prod-us api_url: https://grafana.us-east-1.internal + - name: staging + api_url: https://grafana.staging.internal + api_key: "{{ env.STAGING_GRAFANA_API_KEY }}"You'll also need to add the corresponding environment variable at the top:
holmes: additionalEnvVars: - name: GRAFANA_PASSWORD valueFrom: secretKeyRef: name: grafana-credentials key: password + - name: STAGING_GRAFANA_API_KEY + valueFrom: + secretKeyRef: + name: grafana-credentials + key: staging-api-key toolsets:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/data-sources/builtin-toolsets/grafanadashboards.md` around lines 167 - 188, Add the missing "staging" instance to the Robusta Helm Chart example under toolsets.grafana/dashboards.config.instances (matching the Holmes CLI and Helm examples) and include a per-instance credential override (e.g., an api_key field for the staging instance); also add the corresponding environment variable entry under holmes.additionalEnvVars (so the template value like "{{ env.GRAFANA_STAGING_API_KEY }}" resolves) to demonstrate per-instance credential overrides consistently across examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/data-sources/builtin-toolsets/grafanadashboards.md`:
- Around line 107-111: Replace the invalid grafana_list_instances reference in
the Grafana multi-instance docs: remove the instruction to run
grafana_list_instances and instead tell users to read instance names from
toolsets.grafana/dashboards.config.instances[].name (and note that unknown
grafana_instance errors will list configured names), and update the health-check
sentence to refer to the toolset status string rather than the nonexistent
function.
- Line 190: Update the Grafana dashboards doc wording to reference actual code
artifacts: replace “toolset status string” with the status/error table produced
by pretty_print_toolset_status() and the cached status in toolsets_status.json
(not a generic string); clarify that health_check() for the multi-instance
tolerant check returns (True, "") when at least one instance is reachable so
unreachable-instance details are emitted to warnings (and only set Toolset.error
when all instances fail); and remove or mark as inaccurate the reference to
grafana_list_instances since that command/tool does not exist in the repo.
---
Nitpick comments:
In `@docs/data-sources/builtin-toolsets/grafanadashboards.md`:
- Around line 192-202: The "Username/password authentication (single instance)"
subsection currently lacks the deployment-tabs pattern and sits under "Multiple
Grafana Instances"; update it to match other docs by adding the Holmes CLI /
Holmes Helm Chart / Robusta Helm Chart tab structure around the example (same
tab names and ordering used elsewhere) and either move this subsection out from
under "Multiple Grafana Instances" into a sibling section (e.g., "Alternative
Authentication Methods") or relocate it into the main Configuration section as
an authentication option so the heading and placement reflect single-instance
usage; target the "Username/password authentication (single instance)" heading
and the surrounding "Multiple Grafana Instances" section when making the change.
- Around line 167-188: Add the missing "staging" instance to the Robusta Helm
Chart example under toolsets.grafana/dashboards.config.instances (matching the
Holmes CLI and Helm examples) and include a per-instance credential override
(e.g., an api_key field for the staging instance); also add the corresponding
environment variable entry under holmes.additionalEnvVars (so the template value
like "{{ env.GRAFANA_STAGING_API_KEY }}" resolves) to demonstrate per-instance
credential overrides consistently across examples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 624da601-b896-41ff-972e-f6331c214d1d
📒 Files selected for processing (6)
docs/data-sources/builtin-toolsets/grafanadashboards.mdholmes/plugins/toolsets/grafana/common.pyholmes/plugins/toolsets/grafana/toolset_grafana.pytests/plugins/toolsets/grafana/test_grafana_multi_instance.pytests/plugins/toolsets/test_json_filter_mixin.pytests/plugins/toolsets/test_verify_tool_urls.py
Avi-Robusta
changed the title
naomi-robusta
requested changes
May 26, 2026
naomi-robusta
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests