Repository navigation
urljoin removes path after base url - #1342
Conversation
urljoin("robusta.dev/grafana", "/api/dashboards/tags") -> "robusta.dev/api/dashboards/tags"
Signed-off-by: Arik Alon <alon.arik@gmail.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
✅ Results of HolmesGPT evalsAutomatically triggered by commit 0f73038 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'grafana-dashboards-urljoin' Status: Success - 45 test/model combinations loaded Experiments compared (30):
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" 🏷️ Valid markers
Commands: CLI: |
WalkthroughThe PR modifies URL construction in the Grafana toolset by removing leading slashes from API endpoints and ensuring the base URL includes a trailing slash before concatenation, normalizing endpoint formatting across health checks and dashboard tools without changing public APIs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:a48f981
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:a48f981 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:a48f981
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:a48f981Patch 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:a48f981Robusta 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:a48f981 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @holmes/plugins/toolsets/grafana/toolset_grafana.py:
- Around line 109-112: The _make_grafana_request method currently lets
response.raise_for_status() bubble up; catch requests.exceptions.HTTPError (and
general RequestException) around the requests call in _make_grafana_request and
return a StructuredToolResult with status=StructuredToolResultStatus.ERROR
(include the attempted full URL, the query_params/request payload, the HTTP
status code and response text from e.response if present, and any other request
details) and keep the success path returning
StructuredToolResult(status=StructuredToolResultStatus.SUCCESS, data=data,
url=url, params=query_params); ensure you reference the same variable names used
in the function (url, query_params, params, response) and include fallback text
if e.response is None.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
102-102: Update docstring example to match new endpoint format.The docstring example still shows
"/api/search"with a leading slash, but the actual usage now requires relative paths without leading slashes (e.g.,"api/search").📝 Proposed fix
- endpoint: API endpoint path (e.g., "/api/search") + endpoint: API endpoint path (e.g., "api/search")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/plugins/toolsets/grafana/toolset_grafana.py
holmes/plugins/toolsets/**/*.{py,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.{py,yaml}: All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
For 'no data' responses in toolsets, specify what was searched and where
Never return unbounded data from APIs - always include filter parameters on tools that query collections
Files:
holmes/plugins/toolsets/grafana/toolset_grafana.py
holmes/plugins/toolsets/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.py: Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Implement simple Pydantic config class with validation for Python toolsets
Include health check in prerequisites_callable() method for Python toolsets
Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Use @model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Bash toolset validates commands for safety
Files:
holmes/plugins/toolsets/grafana/toolset_grafana.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
holmes/plugins/toolsets/grafana/common.py (1)
get_base_url(35-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (1)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
75-75: Endpoint paths in toolset_grafana.py are correctly updated, but the refactoring is incomplete.The endpoints at lines 75, 222, 264, 288, and 314 are correctly changed to relative paths for use with
urljoin. However,grafana_tempo_api.pystill uses leading-slash endpoints (/api/searchat line 218,/api/v2/traces/{trace_id}at line 182, etc.) with simple string concatenation, which is inconsistent with this refactoring.Update
grafana_tempo_api.pyto use relative paths (without leading slashes) and implement the sameurljoinpattern astoolset_grafana.pyfor consistency.Likely an incorrect or invalid review comment.
2 participants
urljoin("robusta.dev/grafana", "/api/dashboards/tags") -> "robusta.dev/api/dashboards/tags"
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.