Repository navigation
Make Grafana config class configurable in base toolset - #1732
Conversation
The base class prerequisites_callable() always instantiated GrafanaConfig, ignoring subclass config_classes (e.g. GrafanaTempoConfig which adds 'labels'). The cast() in the Tempo toolset didn't convert the object at runtime, causing AttributeError when accessing .labels. Now uses the subclass's config_classes[0]. https://claude.ai/code/session_01DBzG3kmGffrwmGxrWTqT84 Signed-off-by: Claude <noreply@anthropic.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…allable Verifies that prerequisites_callable creates GrafanaTempoConfig (with labels) rather than plain GrafanaConfig, preventing the AttributeError regression. https://claude.ai/code/session_01DBzG3kmGffrwmGxrWTqT84 Signed-off-by: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces hardcoded GrafanaConfig instantiation in BaseGrafanaToolset with selection of a configurable class (uses 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/plugins/toolsets/grafana/test_grafana_tempo_unit.py (1)
459-481: Well-structured regression test with clear documentation.The docstring clearly explains the bug being prevented, and the assertions properly verify that:
- The config is the correct subclass type (
GrafanaTempoConfig)- The
labelsattribute exists- The
labelsfield is properly instantiated asGrafanaTempoLabelsConfigConsider adding an assertion for the return value of
prerequisites_callable()to verify the health check completes successfully (returns(True, ...)). This would make the test more comprehensive.💡 Optional enhancement
with patch( "holmes.plugins.toolsets.grafana.toolset_grafana_tempo.GrafanaTempoAPI" ): - toolset.prerequisites_callable(config) + result = toolset.prerequisites_callable(config) + assert result[0] is True, f"prerequisites_callable failed: {result[1]}" assert isinstance(toolset._grafana_config, GrafanaTempoConfig) assert hasattr(toolset._grafana_config, "labels") assert isinstance(toolset._grafana_config.labels, GrafanaTempoLabelsConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/grafana/test_grafana_tempo_unit.py` around lines 459 - 481, Add an assertion that prerequisites_callable returns a successful health-check tuple: call GrafanaTempoToolset.prerequisites_callable(config) (within the same patched GrafanaTempoAPI context) and assert the returned value's first element is True (e.g., returned_tuple[0] is True) to ensure the health check completed successfully in addition to the existing type/attribute assertions on toolset._grafana_config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/plugins/toolsets/grafana/test_grafana_tempo_unit.py`:
- Around line 459-481: Add an assertion that prerequisites_callable returns a
successful health-check tuple: call
GrafanaTempoToolset.prerequisites_callable(config) (within the same patched
GrafanaTempoAPI context) and assert the returned value's first element is True
(e.g., returned_tuple[0] is True) to ensure the health check completed
successfully in addition to the existing type/attribute assertions on
toolset._grafana_config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc2e216e-e249-43fd-8932-35896465de2e
📒 Files selected for processing (2)
holmes/plugins/toolsets/grafana/base_grafana_toolset.pytests/plugins/toolsets/grafana/test_grafana_tempo_unit.py
…quisites_callable
Tests the actual user-facing behavior (building trace filters) rather than
checking internal types. Verified the test reproduces the exact original error
('GrafanaConfig' object has no attribute 'labels') when the fix is reverted.
https://claude.ai/code/session_01DBzG3kmGffrwmGxrWTqT84
Signed-off-by: Claude <noreply@anthropic.com>
📂 Previous Runs📜 Run @ 6444c79 (#22911128333)✅ Results of HolmesGPT evalsAutomatically triggered by commit 6444c79 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
⏳ HolmesGPT evals running...Automatically triggered by commit 769beb6 on branch Progress:
📋 Evals to run🔄 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 in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
|
✅ 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:5ccef260
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:5ccef260 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:5ccef260
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:5ccef260
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:5ccef260
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:5ccef260 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:5ccef260
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:5ccef260Patch 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:5ccef260 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:5ccef260Robusta 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:5ccef260 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:5ccef260 |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
3 participants
Summary
Updated the
BaseGrafanaToolsetto support configurable config classes instead of hardcodingGrafanaConfig. This allows subclasses to use their own config class implementations while maintaining backward compatibility.Key Changes
prerequisites_callable()to dynamically select the config class fromself.config_classesif available, falling back toGrafanaConfigas defaultImplementation Details
self.config_classesexists and uses the first element as the config classGrafanaConfigwhenconfig_classesis not defined or emptyhttps://claude.ai/code/session_01DBzG3kmGffrwmGxrWTqT84
Summary by CodeRabbit
New Features
Tests