Rename Datadog config fields for consistency and clarity - #1493
Conversation
|
📂 Previous Runs📜 Run @ 8e35762 (#21820520806)✅ Results of HolmesGPT evalsAutomatically triggered by commit 8e35762 on branch Results of HolmesGPT evals
📜 Run @ 1fd6e29 (#21802733119)✅ Results of HolmesGPT evalsAutomatically triggered by commit 1fd6e29 on branch Results of HolmesGPT evals
📜 Run @ 5fc54df (#21799860839)✅ Results of HolmesGPT evalsAutomatically triggered by commit 5fc54df on branch Results of HolmesGPT evals
📜 Run @ c6769e9 (#21799683279)✅ Results of HolmesGPT evalsAutomatically triggered by commit c6769e9 on branch Results of HolmesGPT evals
📜 Run @ abb3f89 (#21797906504)✅ Results of HolmesGPT evalsAutomatically triggered by commit abb3f89 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit e7e196a on branch Results of HolmesGPT evals
📖 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: |
|
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:
WalkthroughThe PR renames Datadog configuration fields from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. No actionable comments were generated in the recent review. 🎉 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:607d961
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:607d961 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:607d961
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:607d961Patch 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:607d961Robusta 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:607d961 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/datadog/toolset_datadog_logs.py (1)
245-251:⚠️ Potential issue | 🔴 CriticalBug: Still using deprecated field name
request_timeoutinstead oftimeout_seconds.Line 249 references
self.toolset.dd_config.request_timeoutbut the field was renamed totimeout_seconds. This will cause anAttributeErrorat runtime when fetching logs.🐛 Fix: Use the new field name
response = execute_datadog_http_request( url=url, headers=headers, payload_or_params=payload, - timeout=self.toolset.dd_config.request_timeout, + timeout=self.toolset.dd_config.timeout_seconds, method="POST", )
🧹 Nitpick comments (3)
docs/data-sources/builtin-toolsets/datadog.md (1)
36-59: Documentation correctly updated with new field names.All configuration examples consistently use the new field names (
api_key,app_key,api_url,timeout_seconds).Consider adding a brief migration note for users with existing configurations using the old field names (
dd_api_key,dd_app_key,site_api_url,request_timeout). While backward compatibility is maintained in code, documenting this helps users understand:
- Old field names still work but are deprecated
- They should update to new names when convenient
This could be a small note in the Quick Start section or a dedicated "Migration" subsection.
tests/utils/test_toolset_config.py (1)
122-183: Add type hints to new pytest tests; rephrase the precedence comment.These new tests should carry type hints to satisfy mypy, and the Line 179 comment can explain why the precedence is expected.
Proposed update
- def test_deprecated_datadog_fields(self, caplog): + def test_deprecated_datadog_fields(self, caplog: pytest.LogCaptureFixture) -> None: """Test that deprecated Datadog config fields are migrated.""" from holmes.plugins.toolsets.datadog.datadog_api import DatadogBaseConfig @@ - def test_new_datadog_fields_no_warning(self, caplog): + def test_new_datadog_fields_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: """Test that new Datadog field names don't trigger warnings.""" from holmes.plugins.toolsets.datadog.datadog_api import DatadogBaseConfig @@ - def test_old_and_new_datadog_fields_new_takes_precedence(self, caplog): + def test_old_and_new_datadog_fields_new_takes_precedence( + self, caplog: pytest.LogCaptureFixture + ) -> None: """Test that new Datadog field names take precedence over deprecated ones.""" from holmes.plugins.toolsets.datadog.datadog_api import DatadogBaseConfig @@ - # New fields should take precedence + # New fields override deprecated ones to avoid ambiguity assert config.api_key == "new-api-key"As per coding guidelines, use mypy for type checking with type hints required in all code, and write clear, concise comments that explain 'why' rather than 'what'.
tests/plugins/toolsets/datadog/logs/test_check_prerequisites.py (1)
43-45: Reword the comment to explain the intent (why).The Line 44 comment can explain that the omission is deliberate to exercise validation.
Proposed update
- # Missing app_key and api_url + # Intentionally omit app_key/api_url to exercise validationAs per coding guidelines, write clear, concise comments that explain 'why' rather than 'what'.
66fbef3 to
d3ee1a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/data-sources/builtin-toolsets/datadog.md`:
- Around line 36-38: Replace the incorrect Datadog web UI URL with the Datadog
API base URL across all configuration examples: change any occurrence of
"https://app.datadoghq.com" (found as values for api_url in the config blocks)
to "https://api.datadoghq.com" so the api_url setting points to the API base for
US accounts (requests will still use /api/v1/... or /api/v2/... as needed);
update every api_url instance in the document (including all quick-start/config
blocks that reference api_key/app_key and api_url) so examples use the API
endpoint instead of the web UI URL.
d3ee1a7 to
60e3f88
Compare
Renamed Datadog toolset config fields to more intuitive names: - dd_api_key → api_key - dd_app_key → app_key - site_api_url → api_url - request_timeout → timeout_seconds The old field names continue to work with a deprecation warning via the _deprecated_mappings pattern (matching Prometheus approach). Changes: - Update DatadogBaseConfig with new field names and deprecated mappings - Update all toolset implementations to use new field names - Update documentation to use new field names - Update all tests and eval fixtures to use new field names - Add backward compatibility tests for deprecated field names https://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb Signed-off-by: Claude <noreply@anthropic.com>
60e3f88 to
d1d2268
Compare
Revert api_url back to https://api.datadoghq.com for the four toolset detail configuration sections (logs, metrics, traces, general) while keeping https://app.datadoghq.com in the Quick Start sections. https://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb Signed-off-by: Claude <noreply@anthropic.com>
|
/eval |
Move DatadogBaseConfig and ServiceNowTablesConfig imports from inside test methods to the module level, following Python best practices. https://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/utils/test_toolset_config.py`:
- Around line 126-205: The test methods in
TestDatadogConfigBackwardCompatibility are missing type hints; add typing.Any to
the file imports and annotate three methods that accept caplog with caplog: Any
and annotate all four test methods with -> None. Specifically, update the
signatures of test_deprecated_datadog_fields(caplog: Any) -> None,
test_new_datadog_fields_no_warning(caplog: Any) -> None,
test_old_and_new_datadog_fields_new_takes_precedence(caplog: Any) -> None, and
test_old_fields_produce_same_config_as_new_fields() -> None, and add "from
typing import Any" to the module imports.
Add Any to typing imports and annotate the four test methods with proper type hints for caplog parameter and return types. https://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb Signed-off-by: Claude <noreply@anthropic.com>
## Summary Standardizes Datadog toolset configuration field names to be more concise and consistent with naming conventions. All `dd_*` prefixed fields and verbose names are renamed to shorter, clearer alternatives while maintaining full backward compatibility. ## Key Changes - **Configuration field renames:** - `dd_api_key` → `api_key` - `dd_app_key` → `app_key` - `site_api_url` → `api_url` - `request_timeout` → `timeout_seconds` - **Backward compatibility:** Implemented automatic field migration using `_deprecated_mappings` in `DatadogBaseConfig` that: - Accepts both old and new field names - Logs deprecation warnings when old names are used - Gives precedence to new field names if both are provided - Ensures existing configurations continue to work without changes - **Documentation updates:** Updated all examples in the Datadog data source documentation to use the new field names across all deployment methods (Holmes CLI, Holmes Helm Chart, Robusta Helm Chart) - **Test updates:** Updated all test fixtures and test code to use the new field names while adding comprehensive backward compatibility tests ## Implementation Details - Added `ClassVar[Dict[str, Optional[str]]]` type hint for the deprecated mappings dictionary - Leveraged Pydantic's field validation to handle the migration transparently - All internal code references updated to use new field names - Deprecation warnings help users identify and update their configurations https://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated Datadog docs and examples to use standardized key names. * **Configuration Changes** * Renamed Datadog config keys: dd_api_key → api_key, dd_app_key → app_key, site_api_url → api_url, request_timeout → timeout_seconds. * Backward compatibility: old keys are automatically supported during migration. * **Tests** * Updated and added tests to cover new keys and backward-compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
3 participants
Summary
Standardizes Datadog toolset configuration field names to be more concise and consistent with naming conventions. All
dd_*prefixed fields and verbose names are renamed to shorter, clearer alternatives while maintaining full backward compatibility.Key Changes
Configuration field renames:
dd_api_key→api_keydd_app_key→app_keysite_api_url→api_urlrequest_timeout→timeout_secondsBackward compatibility: Implemented automatic field migration using
_deprecated_mappingsinDatadogBaseConfigthat:Documentation updates: Updated all examples in the Datadog data source documentation to use the new field names across all deployment methods (Holmes CLI, Holmes Helm Chart, Robusta Helm Chart)
Test updates: Updated all test fixtures and test code to use the new field names while adding comprehensive backward compatibility tests
Implementation Details
ClassVar[Dict[str, Optional[str]]]type hint for the deprecated mappings dictionaryhttps://claude.ai/code/session_01XhFkchzMzZu6v39uT8ofTb
Summary by CodeRabbit
Documentation
Configuration Changes
Tests