Repository navigation
Add max retries & timeout to grafana config - #1927
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
📂 Previous Runs📜 #5 · Run @ __37c40b3__ (#24832704231) — Apr 23, 11:35 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 37c40b3 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #4 · Run @ __a385529__ (#24732188585) — Apr 21, 16:10 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit a385529 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #3 · Run @ __9fadbb1__ (#24722963655) — Apr 21, 12:49 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 9fadbb1 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #2 · Run @ __42adcd1__ (#24718095787) — Apr 21, 10:51 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 42adcd1 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #1 · Run @ __483d9e3__ (#24708821129) — Apr 21, 07:16 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 483d9e3 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 18688e2 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
|
✅ 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:b3edcf40
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:b3edcf40 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b3edcf40
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b3edcf40
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b3edcf40
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b3edcf40 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b3edcf40
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b3edcf40Patch 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:b3edcf40 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:b3edcf40Robusta 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:b3edcf40 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:b3edcf40 |
3455126 to
960c455
Compare
✅ 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds timeout and retry configuration to Grafana configs and threads them into Tempo, Loki, and core Grafana request paths; replaces module-level retry decorators with per-request backoff, updates method signatures to accept optional timeout/retries, adds tests, and documents HTTP mocking guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as BaseGrafanaTool / Toolset
participant API as Grafana API wrapper (Tempo / Loki)
participant HTTP as HTTP client (requests)
participant Remote as Grafana / Loki service
Tool->>API: invoke request (timeout/retries omitted)
API->>API: resolve timeout := config.timeout_seconds\nretries := config.max_retries
API->>HTTP: perform HTTP call with timeout
alt transient network error / 5xx
HTTP-->>API: raise RequestException / 5xx response
API->>HTTP: retry per backoff (up to retries)
else 2xx response
HTTP-->>API: return response (JSON)
end
API-->>Tool: return parsed result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
|
@claude review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
holmes/plugins/toolsets/grafana/grafana_tempo_api.py (1)
146-152:⚠️ Potential issue | 🟠 MajorApply
max_retriesto the echo endpoint too.This request now honors
timeout_seconds, but transient connection failures still get only one attempt even whenconfig.max_retriesis set.🔁 Proposed retry wrapper
try: - response = requests.get( - url, - headers=self.headers, - timeout=self.config.timeout_seconds, - verify=self.config.verify_ssl, - ) + `@backoff.on_exception`( + backoff.expo, + requests.exceptions.RequestException, + max_tries=self.config.max_retries, + giveup=lambda e: isinstance(e, requests.exceptions.HTTPError) + and getattr(e, "response", None) is not None + and e.response.status_code < 500, + ) + def make_request(): + response = requests.get( + url, + headers=self.headers, + timeout=self.config.timeout_seconds, + verify=self.config.verify_ssl, + ) + if response.status_code >= 500: + response.raise_for_status() + return response + + response = make_request()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/grafana/grafana_tempo_api.py` around lines 146 - 152, The echo endpoint currently calls requests.get directly and only honors timeout_seconds and verify_ssl but not self.config.max_retries; wrap the call in a requests.Session configured with a urllib3 Retry using total=self.config.max_retries and mount an HTTPAdapter (or reuse the existing session/adapter if one exists) so transient connection errors are retried; ensure you use the same headers, timeout (self.config.timeout_seconds), and verify (self.config.verify_ssl) when calling session.get instead of requests.get to apply the retry behavior.holmes/plugins/toolsets/grafana/loki_api.py (1)
42-71:⚠️ Potential issue | 🟠 MajorPreserve Loki query context and API response in retry failures.
After retries are exhausted, the raised error still omits the LogQL query, time range, limit, URL, and response body. That makes Loki failures hard for the LLM to self-correct.
🧩 Proposed error-detail fix
params = {"query": query, "limit": limit, "start": start, "end": end} + url = f"{base_url}/loki/api/v1/query_range" `@backoff.on_exception`( backoff.expo, requests.exceptions.RequestException, max_tries=max_retries, giveup=lambda e: isinstance(e, requests.exceptions.HTTPError) - and e.response.status_code < 500, + and getattr(e, "response", None) is not None + and e.response.status_code < 500, ) def _make_request(): - url = f"{base_url}/loki/api/v1/query_range" response = requests.get( url, headers=build_headers(api_key=api_key, additional_headers=headers), params=params, # type: ignore verify=verify_ssl, @@ - except requests.exceptions.RequestException as e: - raise Exception(f"Failed to query Loki logs: {str(e)}") + except requests.exceptions.HTTPError as e: + response = e.response + status_code = response.status_code if response is not None else "unknown" + response_text = response.text if response is not None else "" + raise Exception( + f"Failed to query Loki logs. URL: {url}. Query: {query}. " + f"Start: {start}. End: {end}. Limit: {limit}. " + f"HTTP status: {status_code}. Response: {response_text}. Error: {e}" + ) + except requests.exceptions.RequestException as e: + raise Exception( + f"Failed to query Loki logs. URL: {url}. Query: {query}. " + f"Start: {start}. End: {end}. Limit: {limit}. Error: {e}" + )As per coding guidelines,
holmes/plugins/toolsets/**/*.{py,yaml}: “All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction: Include the exact query/command executed, time ranges/parameters/filters used, and full API error response”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/grafana/loki_api.py` around lines 42 - 71, The except block after _make_request must include the full Loki request context and API response details when raising the final error: update the exception handling around the _make_request call to capture and include params (query, start, end, limit), the resolved url (built from base_url + "/loki/api/v1/query_range"), any request headers (from build_headers with api_key and headers), and the HTTP response details (status_code and response.text or response.content when available) in the raised Exception message; reference the existing symbols _make_request, params, base_url, api_key, headers, response (from _make_request), and parse_loki_response so you add this contextual information to the Exception(f"...") that currently raises "Failed to query Loki logs: {str(e)}".holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
202-237:⚠️ Potential issue | 🟠 MajorHonor
max_retriesfor dashboard API requests.
_make_grafana_requestnow uses configured timeouts, but dashboard tools still make a single attempt. This leavesGrafanaDashboardConfig.max_retriesineffective for search/get/tag calls.🔁 Proposed retry support
+import backoff import requests # type: ignore- response = requests.get( - url, - headers=headers, - params=query_params, - timeout=timeout, - verify=config.verify_ssl, - ) - response.raise_for_status() + `@backoff.on_exception`( + backoff.expo, + requests.exceptions.RequestException, + max_tries=config.max_retries, + giveup=lambda e: isinstance(e, requests.exceptions.HTTPError) + and getattr(e, "response", None) is not None + and e.response.status_code < 500, + ) + def make_request(): + response = requests.get( + url, + headers=headers, + params=query_params, + timeout=timeout, + verify=config.verify_ssl, + ) + response.raise_for_status() + return response + + response = make_request() data = response.json()As per coding guidelines,
holmes/plugins/toolsets/**/*.py: “When adding new config fields, methods, or behavior, check the class hierarchy and place changes at the most general level that applies, not scoped to specific subclasses”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/grafana/toolset_grafana.py` around lines 202 - 237, The _make_grafana_request currently issues a single requests.get and ignores GrafanaDashboardConfig.max_retries; update it to honor max_retries by creating or reusing a requests.Session, mounting a HTTPAdapter configured with urllib3.util.retry.Retry(total=config.max_retries, backoff_factor=..., status_forcelist=[429,500,502,503,504], allowed_methods=["GET","HEAD","OPTIONS"]) and then perform session.get(...) with the same headers, params, timeout, and verify; ensure the session is properly reused or closed and that the change is applied in the _make_grafana_request function (referencing config = self._toolset.grafana_config and the max_retries field) so all dashboard search/get/tag calls inherit the retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/grafana/common.py`:
- Around line 54-63: The timeout_seconds and max_retries fields need strict
Pydantic validation so invalid YAML is rejected at parse time: in the Grafana
config model in holmes/plugins/toolsets/grafana/common.py, change
timeout_seconds Field to include gt=0 (timeout_seconds > 0) and change
max_retries Field to include ge=1 (max_retries >= 1); if the model currently
lacks typed constraints or uses custom parsing, add `@validator` methods for
"timeout_seconds" and "max_retries" that raise ValueError for values <=0 and <1
respectively, and keep prerequisites_callable() unchanged except rely on the
model validation to prevent invalid configs from reaching runtime.
In `@tests/plugins/toolsets/grafana/test_grafana_config.py`:
- Around line 83-124: Update the tests that instantiate
GrafanaTempoConfig/GrafanaTempoAPI to assert that the configured timeout is
actually passed to requests by adding
responses.matchers.request_kwargs_matcher({"timeout": <timeout>}) to the
rsps.add(...) calls; specifically, in
test_make_request_with_custom_config_returns_data (uses
GrafanaTempoConfig(timeout_seconds=90) and calls
GrafanaTempoAPI.search_traces_by_query) add
match=[matchers.request_kwargs_matcher({"timeout": 90})], and in
test_echo_endpoint_with_custom_config (uses timeout_seconds=120 and calls
GrafanaTempoAPI.query_echo_endpoint) add
match=[matchers.request_kwargs_matcher({"timeout": 120})]; apply the same
pattern to the other related test referenced (the one around lines 173-195) so
each rsps.add verifies the expected timeout value from the GrafanaTempoConfig.
---
Outside diff comments:
In `@holmes/plugins/toolsets/grafana/grafana_tempo_api.py`:
- Around line 146-152: The echo endpoint currently calls requests.get directly
and only honors timeout_seconds and verify_ssl but not self.config.max_retries;
wrap the call in a requests.Session configured with a urllib3 Retry using
total=self.config.max_retries and mount an HTTPAdapter (or reuse the existing
session/adapter if one exists) so transient connection errors are retried;
ensure you use the same headers, timeout (self.config.timeout_seconds), and
verify (self.config.verify_ssl) when calling session.get instead of requests.get
to apply the retry behavior.
In `@holmes/plugins/toolsets/grafana/loki_api.py`:
- Around line 42-71: The except block after _make_request must include the full
Loki request context and API response details when raising the final error:
update the exception handling around the _make_request call to capture and
include params (query, start, end, limit), the resolved url (built from base_url
+ "/loki/api/v1/query_range"), any request headers (from build_headers with
api_key and headers), and the HTTP response details (status_code and
response.text or response.content when available) in the raised Exception
message; reference the existing symbols _make_request, params, base_url,
api_key, headers, response (from _make_request), and parse_loki_response so you
add this contextual information to the Exception(f"...") that currently raises
"Failed to query Loki logs: {str(e)}".
In `@holmes/plugins/toolsets/grafana/toolset_grafana.py`:
- Around line 202-237: The _make_grafana_request currently issues a single
requests.get and ignores GrafanaDashboardConfig.max_retries; update it to honor
max_retries by creating or reusing a requests.Session, mounting a HTTPAdapter
configured with urllib3.util.retry.Retry(total=config.max_retries,
backoff_factor=..., status_forcelist=[429,500,502,503,504],
allowed_methods=["GET","HEAD","OPTIONS"]) and then perform session.get(...) with
the same headers, params, timeout, and verify; ensure the session is properly
reused or closed and that the change is applied in the _make_grafana_request
function (referencing config = self._toolset.grafana_config and the max_retries
field) so all dashboard search/get/tag calls inherit the retry behavior.
🪄 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: 6b0eaad3-9261-4bf8-b264-4105773490cb
📒 Files selected for processing (7)
CLAUDE.mdholmes/plugins/toolsets/grafana/common.pyholmes/plugins/toolsets/grafana/grafana_tempo_api.pyholmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.pyholmes/plugins/toolsets/grafana/loki_api.pyholmes/plugins/toolsets/grafana/toolset_grafana.pytests/plugins/toolsets/grafana/test_grafana_config.py
|
@claude fix the comments. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
582-588:⚠️ Potential issue | 🟡 MinorRender requests still ignore
max_retries.
grafana_render_panelandgrafana_render_dashboardnow honortimeout_seconds, but transient renderer failures still make only one HTTP attempt. Ifmax_retriesis meant to cover Grafana API calls broadly, route render requests through the same retry behavior or document render calls as timeout-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/grafana/toolset_grafana.py` around lines 582 - 588, The render requests in grafana_render_panel and grafana_render_dashboard call requests.get directly (the response = requests.get(...) call) and therefore ignore the configured max_retries; update these render code paths to use the same retry-enabled HTTP helper used elsewhere (e.g., the module's existing retry/session helper or a shared function like requests_retry_session/_do_request) so render calls honor config.max_retries and timeout_seconds, or alternatively pass a session configured with HTTPAdapter(retries=...) into requests.get; change both grafana_render_panel and grafana_render_dashboard to route their GET to that helper/session instead of calling requests.get directly so transient renderer failures are retried.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/grafana/toolset_grafana.py`:
- Around line 252-253: The call to response = _do_request() can raise
HTTPError/Timeout/ConnectionError and currently escapes without returning a
structured error; wrap the _do_request() invocation in a try/except that catches
requests.exceptions.HTTPError, requests.exceptions.Timeout,
requests.exceptions.ConnectionError (and a generic Exception fallback), and
return a structured Grafana API error object/string containing the request URL,
query params/filters, retry and timeout settings used, and the full response
body/status_code when available (use e.response or response.text/status_code for
HTTPError). Update the logic around response = _do_request() / data =
response.json() to only parse JSON when the call succeeds and to include these
detailed fields in the tool result on failure so LLMs can self-correct.
- Around line 223-237: The backoff decorator currently uses config.max_retries
as max_tries which undercounts because max_tries is total calls; update the
backoff.on_exception call to pass max_tries=config.max_retries + 1 (i.e.,
convert retry attempts to total calls) where the decorator is applied (the
backoff.on_exception block that wraps the request call and references retries,
config.max_retries, and requests.exceptions.RequestException).
---
Outside diff comments:
In `@holmes/plugins/toolsets/grafana/toolset_grafana.py`:
- Around line 582-588: The render requests in grafana_render_panel and
grafana_render_dashboard call requests.get directly (the response =
requests.get(...) call) and therefore ignore the configured max_retries; update
these render code paths to use the same retry-enabled HTTP helper used elsewhere
(e.g., the module's existing retry/session helper or a shared function like
requests_retry_session/_do_request) so render calls honor config.max_retries and
timeout_seconds, or alternatively pass a session configured with
HTTPAdapter(retries=...) into requests.get; change both grafana_render_panel and
grafana_render_dashboard to route their GET to that helper/session instead of
calling requests.get directly so transient renderer failures are retried.
🪄 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: 5438a269-6426-47c8-8c8a-80b6af7e73fa
📒 Files selected for processing (5)
holmes/plugins/toolsets/grafana/common.pyholmes/plugins/toolsets/grafana/grafana_tempo_api.pyholmes/plugins/toolsets/grafana/loki_api.pyholmes/plugins/toolsets/grafana/toolset_grafana.pytests/plugins/toolsets/grafana/test_grafana_config.py
✅ Files skipped from review due to trivial changes (1)
- tests/plugins/toolsets/grafana/test_grafana_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/toolsets/grafana/common.py
- holmes/plugins/toolsets/grafana/grafana_tempo_api.py
- holmes/plugins/toolsets/grafana/loki_api.py
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed: backoff retry logic added to all request paths, Pydantic bounds validated, giveup lambda None guards consistent, and tests updated with sleep mocking and timeout matchers.
Extended reasoning...
Overview
This PR adds timeout_seconds and max_retries to GrafanaConfig and threads them through all Grafana toolsets (Tempo, Loki, Dashboards, Render). The implementation is now consistent across every HTTP code path.
Security risks
None. This is purely a configuration and resilience change — no auth, no crypto, no permissions changes. The new Pydantic gt=0/ge=1 constraints prevent invalid config from reaching runtime.
Level of scrutiny
Medium. This touches several HTTP request paths and adds backoff retry logic, but the changes are well-patterned and follow the existing Tempo/Loki implementations. All evals pass (11/11 across 5 runs), and the test suite now verifies timeout propagation via request_kwargs_matcher.
Prior review resolution
Every issue I raised in prior rounds was addressed: backoff added to _make_grafana_request, _make_render_request, and query_echo_endpoint; Loki giveup lambda now has a None guard matching the Tempo pattern; timeout_seconds and max_retries have Pydantic gt=0/ge=1 constraints; retry tests mock time.sleep; and request_kwargs_matcher verifies actual timeout propagation in HTTP calls. The max_retries-as-total-attempts semantic was explicitly accepted by the maintainer.
There was a problem hiding this comment.
My prior inline comment about the render timeout regression at _make_render_request (line 571) was incorrect — GrafanaDashboardConfig correctly overrides timeout_seconds to 60s, which preserves the pre-PR behavior. Please disregard that concern; the actual outstanding issue is the renderer probe timeout noted in the new inline comment.
Extended reasoning...
Overview
This PR adds configurable timeout_seconds and max_retries to GrafanaConfig and propagates those settings through Grafana Tempo, Loki, and dashboard toolsets, replacing hardcoded values. It spans 7 files including config models, API wrappers, toolset classes, and a new test file.
Correction of prior review comment
My April 21 inline comment on toolset_grafana.py:571 claimed the render timeout had regressed from 60s to 30s. That was wrong. The PR adds GrafanaDashboardConfig.timeout_seconds = Field(default=60), which overrides the base class 30s default specifically for the dashboard toolset. _make_render_request now resolves to config.timeout_seconds = 60, which is identical to the old hardcoded value. No regression occurred, and no render_timeout_seconds field is needed.
Newly identified issue (probe timeout)
The new bug report correctly identifies that the renderer probe requests in _try_add_render_tools (lines 134 and 157) previously used a hardcoded timeout=10 and now use config.timeout_seconds = 60. The 6x increase is meaningful for the misconfiguration case (enable_rendering=True, no renderer installed): startup can now wait up to 120s (2×60s) instead of 20s (2×10s). The inline comment on this PR describes a concrete fix (separate probe_timeout_seconds field or capping with min()).
Security risks
No security-sensitive code is touched. This is configuration and HTTP retry/timeout logic.
Level of scrutiny
Medium-to-high: this PR touches startup behavior and all Grafana HTTP call paths. The probe timeout change is a behavioral regression that affects startup time under a common misconfiguration scenario.
Other factors
All 11 LLM eval tests pass. CodeRabbit's pydantic bounds suggestion (gt=0, ge=1) was incorporated. The time.sleep mock was added to retry tests. The outstanding issue is limited in scope (2 lines in one method).
Changed timeout value for rendering version API request.
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests