Conversation
|
|
|
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 Datadog multi-account support: new DatadogAccount model and config normalization, refactors URL/header helpers to use per-account credentials, adds optional account tool parameter across Datadog tools, performs per-account healthchecks, injects accounts into Jinja instructions, and adds tests and docs. ChangesMulti-account Datadog support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
tests/plugins/toolsets/datadog/test_datadog_accounts.py (1)
35-41: ⚡ Quick winAdd a regression test for blank credential values.
Coverage is strong, but this suite should also assert that empty/whitespace
api_key/app_keyare rejected (not just missing fields). That prevents silent auth misconfiguration regressions.🤖 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/datadog/test_datadog_accounts.py` around lines 35 - 41, Add a regression test that asserts blank or whitespace credential values are rejected: create a new test (e.g., test_blank_credentials_raises) that calls _base_config with api_key="" and api_key=" " and similarly with app_key empty/whitespace, and verify each invocation raises ValueError (use pytest.raises with an appropriate match like "missing credentials" or "blank"). Place this alongside test_shorthand_missing_field_raises and test_no_credentials_raises so the suite covers missing, blank, and whitespace-only credential cases.holmes/plugins/toolsets/datadog/toolset_datadog_traces.py (1)
155-162: ⚡ Quick winHoist
load_and_render_promptimport to file scope.Line 161 currently imports inside
_reload_instructions; move it to top-level imports.As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/toolset_datadog_traces.py` around lines 155 - 162, Move the local import of load_and_render_prompt out of the _reload_instructions method and into the module-level imports: remove "from holmes.plugins.prompts import load_and_render_prompt" from inside the _reload_instructions function and add it to the top of the file alongside other imports so the symbol load_and_render_prompt (from holmes.plugins.prompts) is imported at file scope and can be referenced directly within _reload_instructions.holmes/plugins/toolsets/datadog/toolset_datadog_general.py (1)
309-316: ⚡ Quick winMove
_reload_instructionsimport to module scope.Line 315 imports
load_and_render_promptinside the method. Please move it to top-level imports to match repo Python conventions.As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/toolset_datadog_general.py` around lines 309 - 316, The method _reload_instructions currently imports load_and_render_prompt from holmes.plugins.prompts inside the function; move that import to module top-level imports so load_and_render_prompt is imported once when the module loads and to follow project conventions. Update the file-level imports to include "from holmes.plugins.prompts import load_and_render_prompt" and remove the inline import inside _reload_instructions, ensuring any references to load_and_render_prompt inside _reload_instructions remain unchanged.holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
163-170: ⚡ Quick winMove
load_and_render_promptimport to top-level.Line 169 imports inside
_reload_instructions; please move it to module imports.As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/toolset_datadog_logs.py` around lines 163 - 170, The import of load_and_render_prompt is currently inside the _reload_instructions method; move the import statement to the module top-level imports so it follows the project's import guidelines and reference load_and_render_prompt from the module scope. Update the file imports section to include "from holmes.plugins.prompts import load_and_render_prompt" and remove the in-method import in _reload_instructions; if a circular-import error appears when moving it, add a short comment explaining the circular dependency and keep a single localized import with that explanation instead of duplicating imports.tests/plugins/toolsets/datadog/test_toolset_datadog_general.py (2)
181-185: ⚡ Quick winAvoid function-local imports in
setup_method.Lines 182-185 add imports inside the test method. Move them to module scope for consistency with the repo Python style.
As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/test_toolset_datadog_general.py` around lines 181 - 185, The test's setup_method currently contains function-local imports for DatadogGeneralConfig and DatadogGeneralToolset; move these two imports out of setup_method to the top-level of the test module so they are imported at module scope (referencing DatadogGeneralConfig and DatadogGeneralToolset) and remove the in-method import statements in setup_method to comply with the project's import style.
207-224: ⚡ Quick winUse
responsesinstead of patchingrequests.getin tests.These tests patch
holmes.plugins.toolsets.datadog.datadog_api.requests.get; please switch toresponsesso HTTP behavior is mocked at the transport layer.As per coding guidelines: "
tests/**/*.py: Use theresponseslibrary for HTTP mocking in tests, not@patch('requests.get'), as it intercepts at the transport/adapter level for more realistic test behavior".🤖 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/datadog/test_toolset_datadog_general.py` around lines 207 - 224, Replace the `@patch`("holmes.plugins.toolsets.datadog.datadog_api.requests.get") usage in the tests test_account_param_routes_to_target and test_omitted_account_uses_default by using the responses library: call responses.activate (or the responses fixture), register a GET response for the expected base URL with responses.add(...) returning status=200 and the JSON body {"data": []}, then invoke get_tool._invoke (DatadogAPIGet via self.toolset.tools[0]) with the same context and params; finally assert against responses.calls to verify the requested URL starts with "https://api.stg.datadoghq.eu" and that the outgoing request headers include "DD-API-KEY" == "stg-key" (instead of inspecting mock_get.call_args). Ensure both tests follow this pattern and remove the `@patch` decorator and mock_get usage.holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
859-868: ⚡ Quick winMove
_reload_instructionsimport to module scope.Line 867 uses a function-local import; please hoist it to top-level imports.
As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/toolset_datadog_metrics.py` around lines 859 - 868, The method _reload_instructions currently does a function-local import of load_and_render_prompt; hoist that import to module scope by adding "from holmes.plugins.prompts import load_and_render_prompt" to the file's top-level imports and remove the in-function import inside _reload_instructions so the method simply calls load_and_render_prompt directly; ensure no other references rely on the local import and run tests/lint after the change.tests/plugins/toolsets/datadog/traces/test_datadog_traces.py (1)
166-169: ⚡ Quick winMove setup imports to module top.
Lines 167-169 import modules inside
setup_method; please move these imports to module scope.As per coding guidelines: "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods".🤖 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/datadog/traces/test_datadog_traces.py` around lines 166 - 169, The imports for DatadogAccount and DatadogTracesConfig are currently inside setup_method; move "from holmes.plugins.toolsets.datadog.datadog_api import DatadogAccount" and "from holmes.plugins.toolsets.datadog.datadog_models import DatadogTracesConfig" to the top of the module (module scope) and remove them from inside the setup_method so the test file follows the guideline of placing imports at file top and setup_method just references those symbols.tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py (1)
353-385: ⚡ Quick winPrefer
responsesover patchingrequests.getin routing tests.The new account-routing tests should mock transport using
responsesinstead of patchingholmes.plugins.toolsets.datadog.datadog_api.requests.get.As per coding guidelines: "
tests/**/*.py: Use theresponseslibrary for HTTP mocking in tests, not@patch('requests.get'), as it intercepts at the transport/adapter level for more realistic test behavior".🤖 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/datadog/metrics/test_datadog_metrics.py` around lines 353 - 385, The tests test_account_param_routes_to_target_url_and_keys and test_omitted_account_uses_default are patching holmes.plugins.toolsets.datadog.datadog_api.requests.get which violates the testing guideline; replace the requests.get patch with the responses library by registering the expected Datadog endpoint(s) and response bodies (matching the URL patterns used by the ListActiveMetrics tool and its tool._invoke call), assert the call was made by inspecting responses.calls (and verify the request URL and DD-API-KEY / DD-APPLICATION-KEY headers), and remove the `@patch` decorator so the HTTP interaction is intercepted at the transport/adapter level instead of mocking requests.get directly.
🤖 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 `@holmes/plugins/toolsets/datadog/datadog_api.py`:
- Around line 118-127: The config model currently accepts empty or
whitespace-only strings for api_key and app_key (the Field declarations named
api_key and app_key), causing opaque auth failures later; add validation to
reject blank credentials during model validation by trimming and checking
non-empty (e.g. a Pydantic validator or use constr with
min_length/strip_whitespace) and raise a clear ValueError when api_key or
app_key are empty after strip; apply the same non-blank validation to the other
shorthand/alternate credential fields referenced around the second occurrence
(lines 257-271) so both accounts and shorthand credential paths fail fast with a
clear error.
In `@holmes/plugins/toolsets/datadog/datadog_url_utils.py`:
- Around line 119-120: The code currently sets url_params["index"] when indexes
!= ["*"], which still applies a filter if "*" appears alongside other entries;
update the logic in datadog_url_utils (the block using the indexes variable and
url_params dict) to treat any indexes list containing "*" as unfiltered—i.e.,
only set url_params["index"] when "*" is not present in indexes (use a
membership check like '*' in indexes to decide), so that lists containing "*" do
not produce an index query parameter.
---
Nitpick comments:
In `@holmes/plugins/toolsets/datadog/toolset_datadog_general.py`:
- Around line 309-316: The method _reload_instructions currently imports
load_and_render_prompt from holmes.plugins.prompts inside the function; move
that import to module top-level imports so load_and_render_prompt is imported
once when the module loads and to follow project conventions. Update the
file-level imports to include "from holmes.plugins.prompts import
load_and_render_prompt" and remove the inline import inside
_reload_instructions, ensuring any references to load_and_render_prompt inside
_reload_instructions remain unchanged.
In `@holmes/plugins/toolsets/datadog/toolset_datadog_logs.py`:
- Around line 163-170: The import of load_and_render_prompt is currently inside
the _reload_instructions method; move the import statement to the module
top-level imports so it follows the project's import guidelines and reference
load_and_render_prompt from the module scope. Update the file imports section to
include "from holmes.plugins.prompts import load_and_render_prompt" and remove
the in-method import in _reload_instructions; if a circular-import error appears
when moving it, add a short comment explaining the circular dependency and keep
a single localized import with that explanation instead of duplicating imports.
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 859-868: The method _reload_instructions currently does a
function-local import of load_and_render_prompt; hoist that import to module
scope by adding "from holmes.plugins.prompts import load_and_render_prompt" to
the file's top-level imports and remove the in-function import inside
_reload_instructions so the method simply calls load_and_render_prompt directly;
ensure no other references rely on the local import and run tests/lint after the
change.
In `@holmes/plugins/toolsets/datadog/toolset_datadog_traces.py`:
- Around line 155-162: Move the local import of load_and_render_prompt out of
the _reload_instructions method and into the module-level imports: remove "from
holmes.plugins.prompts import load_and_render_prompt" from inside the
_reload_instructions function and add it to the top of the file alongside other
imports so the symbol load_and_render_prompt (from holmes.plugins.prompts) is
imported at file scope and can be referenced directly within
_reload_instructions.
In `@tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py`:
- Around line 353-385: The tests
test_account_param_routes_to_target_url_and_keys and
test_omitted_account_uses_default are patching
holmes.plugins.toolsets.datadog.datadog_api.requests.get which violates the
testing guideline; replace the requests.get patch with the responses library by
registering the expected Datadog endpoint(s) and response bodies (matching the
URL patterns used by the ListActiveMetrics tool and its tool._invoke call),
assert the call was made by inspecting responses.calls (and verify the request
URL and DD-API-KEY / DD-APPLICATION-KEY headers), and remove the `@patch`
decorator so the HTTP interaction is intercepted at the transport/adapter level
instead of mocking requests.get directly.
In `@tests/plugins/toolsets/datadog/test_datadog_accounts.py`:
- Around line 35-41: Add a regression test that asserts blank or whitespace
credential values are rejected: create a new test (e.g.,
test_blank_credentials_raises) that calls _base_config with api_key="" and
api_key=" " and similarly with app_key empty/whitespace, and verify each
invocation raises ValueError (use pytest.raises with an appropriate match like
"missing credentials" or "blank"). Place this alongside
test_shorthand_missing_field_raises and test_no_credentials_raises so the suite
covers missing, blank, and whitespace-only credential cases.
In `@tests/plugins/toolsets/datadog/test_toolset_datadog_general.py`:
- Around line 181-185: The test's setup_method currently contains function-local
imports for DatadogGeneralConfig and DatadogGeneralToolset; move these two
imports out of setup_method to the top-level of the test module so they are
imported at module scope (referencing DatadogGeneralConfig and
DatadogGeneralToolset) and remove the in-method import statements in
setup_method to comply with the project's import style.
- Around line 207-224: Replace the
`@patch`("holmes.plugins.toolsets.datadog.datadog_api.requests.get") usage in the
tests test_account_param_routes_to_target and test_omitted_account_uses_default
by using the responses library: call responses.activate (or the responses
fixture), register a GET response for the expected base URL with
responses.add(...) returning status=200 and the JSON body {"data": []}, then
invoke get_tool._invoke (DatadogAPIGet via self.toolset.tools[0]) with the same
context and params; finally assert against responses.calls to verify the
requested URL starts with "https://api.stg.datadoghq.eu" and that the outgoing
request headers include "DD-API-KEY" == "stg-key" (instead of inspecting
mock_get.call_args). Ensure both tests follow this pattern and remove the `@patch`
decorator and mock_get usage.
In `@tests/plugins/toolsets/datadog/traces/test_datadog_traces.py`:
- Around line 166-169: The imports for DatadogAccount and DatadogTracesConfig
are currently inside setup_method; move "from
holmes.plugins.toolsets.datadog.datadog_api import DatadogAccount" and "from
holmes.plugins.toolsets.datadog.datadog_models import DatadogTracesConfig" to
the top of the module (module scope) and remove them from inside the
setup_method so the test file follows the guideline of placing imports at file
top and setup_method just references those symbols.
🪄 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: ebba76fb-a1db-4336-ac87-6819a620df56
📒 Files selected for processing (16)
docs/data-sources/builtin-toolsets/datadog.mdholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/datadog_general_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_url_utils.pyholmes/plugins/toolsets/datadog/instructions_datadog_traces.jinja2holmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pytests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/test_datadog_accounts.pytests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/plugins/toolsets/datadog/traces/test_datadog_traces.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@holmes/plugins/toolsets/datadog/datadog_api.py`:
- Around line 273-281: The code creates a DatadogAccount in self.accounts and
unconditionally passes api_url=self.api_url, which breaks Pydantic validation
when self.api_url is None; instead, only include the api_url argument when
self.api_url is not None (e.g. build account_kwargs =
{"name":"default","api_key":self.api_key,"app_key":self.app_key,"default":True}
and add "api_url": self.api_url to account_kwargs only if self.api_url is not
None) and then instantiate DatadogAccount(**account_kwargs) so legacy configs
that omit api_url keep the model's default.
🪄 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: 968173e4-b5ee-443b-918e-7930c51b58b1
📒 Files selected for processing (1)
holmes/plugins/toolsets/datadog/datadog_api.py
011c92c to
b9c92f6
Compare
mdecalf
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/plugins/toolsets/datadog/test_toolset_datadog_general.py (1)
182-185: ⚡ Quick winMove these imports to module scope.
Lines 182-183 contain imports inside
setup_method(), which violates the coding guideline that Python imports must be placed at the top of the file. MoveDatadogGeneralConfigand theDatadogGeneralToolsetimport to the module-level import block (note thatDatadogGeneralToolsetis already imported at module level, so remove the duplicate).🤖 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/datadog/test_toolset_datadog_general.py` around lines 182 - 185, The imports for DatadogGeneralConfig and DatadogGeneralToolset are inside setup_method(), violating the module-scope import guideline; move the DatadogGeneralConfig import to the top import block and remove the duplicate DatadogGeneralToolset import (since DatadogGeneralToolset is already imported at module level) so both symbols are imported at module scope and setup_method() no longer contains any imports.tests/plugins/toolsets/datadog/traces/test_datadog_traces.py (1)
167-169: ⚡ Quick winMove method-local imports to the file import section.
These imports should be moved to module scope to match the Python import guideline used in this repo. According to the coding guidelines: "Always place Python imports at the top of the file, not inside functions or methods."
from holmes.plugins.toolsets.datadog.datadog_api import DatadogAccount from holmes.plugins.toolsets.datadog.datadog_models import DatadogTracesConfig🤖 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/datadog/traces/test_datadog_traces.py` around lines 167 - 169, Move the method-local imports for DatadogAccount and DatadogTracesConfig out of the test method and add them to the module-level import section at the top of the test file; locate the local imports referencing holmes.plugins.toolsets.datadog.datadog_api.DatadogAccount and holmes.plugins.toolsets.datadog.datadog_models.DatadogTracesConfig, remove them from inside the test function, and place equivalent import statements with those symbols in the file's import block so the tests use the top-level imports.
🤖 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/datadog.md`:
- Around line 200-215: Convert the fenced YAML code block to an indented code
block to satisfy MD046: replace the triple-backtick fenced block containing the
"toolsets: datadog/metrics: enabled: true config: accounts: ..." example with a
properly indented code block (prefix each line with four spaces) so the YAML
snippet remains intact; locate the example by searching for the "toolsets",
"datadog/metrics", "config", and "accounts" keys and update that block only.
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Line 867: The import of load_and_render_prompt is currently inside the
_reload_instructions() method; move that import to the module-level import block
at the top of the file so it is no longer nested in the function. Update the top
imports to include "from holmes.plugins.prompts import load_and_render_prompt"
and remove the in-function import inside _reload_instructions() to match other
toolsets (e.g., bash_toolset.py, prometheus.py) and comply with the project's
import guidelines.
In `@tests/plugins/toolsets/datadog/test_toolset_datadog_general.py`:
- Around line 207-224: Replace the `@patch-based` HTTP mocking in the test
functions (e.g., test_account_param_routes_to_target and
test_omitted_account_uses_default) with the responses library: wrap the
invocation of DatadogAPIGet._invoke inside a with responses.RequestsMock() as
rsps: block, register the expected GET using rsps.add(responses.GET,
expected_url, json={"data": []}, status=200) where expected_url matches the
https://api.stg.datadoghq.eu... or default domain used by the tool, then run
get_tool._invoke(...) and assert behavior by inspecting the response/result and
any outbound request headers or URLs via responses (e.g., checking rsps.calls or
the registered URL) instead of using mock_get.call_args; update both
test_account_param_routes_to_target and test_omitted_account_uses_default
accordingly.
---
Nitpick comments:
In `@tests/plugins/toolsets/datadog/test_toolset_datadog_general.py`:
- Around line 182-185: The imports for DatadogGeneralConfig and
DatadogGeneralToolset are inside setup_method(), violating the module-scope
import guideline; move the DatadogGeneralConfig import to the top import block
and remove the duplicate DatadogGeneralToolset import (since
DatadogGeneralToolset is already imported at module level) so both symbols are
imported at module scope and setup_method() no longer contains any imports.
In `@tests/plugins/toolsets/datadog/traces/test_datadog_traces.py`:
- Around line 167-169: Move the method-local imports for DatadogAccount and
DatadogTracesConfig out of the test method and add them to the module-level
import section at the top of the test file; locate the local imports referencing
holmes.plugins.toolsets.datadog.datadog_api.DatadogAccount and
holmes.plugins.toolsets.datadog.datadog_models.DatadogTracesConfig, remove them
from inside the test function, and place equivalent import statements with those
symbols in the file's import block so the tests use the top-level imports.
🪄 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: bee091b5-4c9b-4428-a1ee-f3820c79e2cb
📒 Files selected for processing (16)
docs/data-sources/builtin-toolsets/datadog.mdholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/datadog_general_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_url_utils.pyholmes/plugins/toolsets/datadog/instructions_datadog_traces.jinja2holmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pytests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/test_datadog_accounts.pytests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/plugins/toolsets/datadog/traces/test_datadog_traces.py
✅ Files skipped from review due to trivial changes (1)
- holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2
🚧 Files skipped from review as they are similar to previous changes (11)
- holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2
- holmes/plugins/toolsets/datadog/datadog_general_instructions.jinja2
- holmes/plugins/toolsets/datadog/instructions_datadog_traces.jinja2
- tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py
- tests/plugins/toolsets/datadog/test_datadog_accounts.py
- holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
- holmes/plugins/toolsets/datadog/toolset_datadog_traces.py
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
- holmes/plugins/toolsets/datadog/datadog_api.py
- holmes/plugins/toolsets/datadog/datadog_url_utils.py
- tests/plugins/toolsets/datadog/logs/test_check_prerequisites.py
b9c92f6 to
5a1933b
Compare
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 (1)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
693-751:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the failed request invocation for
list_datadog_metric_tags.
invocationcan never be populated here: the request sends{}inline,query_paramsstaysNone, and the final truthiness check also suppresses empty params. That drops the exact account-scoped endpoint from failures, which makes self-correction much harder.🛠️ Minimal fix
- api_url = None - query_params = None + api_url = None + query_params = {} @@ data = execute_datadog_http_request( url=api_url, headers=headers, timeout=self.toolset.dd_config.timeout_seconds, method="GET", - payload_or_params={}, + payload_or_params=query_params, ) @@ error=error_msg, params=params, invocation=json.dumps({"url": api_url, "params": query_params}) - if api_url and query_params + if api_url is not None else None, )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/datadog/toolset_datadog_metrics.py` around lines 693 - 751, The error path currently never records the actual request because query_params remains None and the call uses an inline {}; change the flow so query_params is set (e.g., query_params = {}) before calling execute_datadog_http_request and pass query_params as the payload_or_params argument (execute_datadog_http_request(..., payload_or_params=query_params)), then in the exception return include the preserved invocation by JSON-encoding {"url": api_url, "params": query_params, "headers": headers} (use the existing api_url, query_params and headers variables) so list_datadog_metric_tags / execute_datadog_http_request failures include the exact request details.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
840-853: ⚡ Quick winPublish
self.dd_configonly after every account passes healthcheck.
self.dd_configis set before the account loop completes. If a later account fails, the toolset keeps an unvalidated config bound on the instance while the status/instructions still reflect the previous state. Keeping it local until the loop succeeds makes the state transition atomic.♻️ Suggested change
try: dd_config = DatadogMetricsConfig(**config) - self.dd_config = dd_config for account in dd_config.accounts: success, error_msg = self._perform_healthcheck(account, dd_config) if not success: return False, error_msg + self.dd_config = dd_config # Re-render the LLM instructions now that the validated config # (with its account list) is bound on the toolset, so the LLM # sees the available account names. self._reload_instructions()🤖 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/datadog/toolset_datadog_metrics.py` around lines 840 - 853, The code sets self.dd_config before validating all accounts which can leave an unvalidated config on the instance if a later account fails; instead keep the parsed DatadogMetricsConfig in a local variable (DatadogMetricsConfig(**config)), run the account loop using _perform_healthcheck for each account, and only assign to self.dd_config after every account check succeeds, then call _reload_instructions; ensure that on any failure you return (False, error_msg) without mutating self.dd_config.
🤖 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 `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 693-751: The error path currently never records the actual request
because query_params remains None and the call uses an inline {}; change the
flow so query_params is set (e.g., query_params = {}) before calling
execute_datadog_http_request and pass query_params as the payload_or_params
argument (execute_datadog_http_request(..., payload_or_params=query_params)),
then in the exception return include the preserved invocation by JSON-encoding
{"url": api_url, "params": query_params, "headers": headers} (use the existing
api_url, query_params and headers variables) so list_datadog_metric_tags /
execute_datadog_http_request failures include the exact request details.
---
Nitpick comments:
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 840-853: The code sets self.dd_config before validating all
accounts which can leave an unvalidated config on the instance if a later
account fails; instead keep the parsed DatadogMetricsConfig in a local variable
(DatadogMetricsConfig(**config)), run the account loop using
_perform_healthcheck for each account, and only assign to self.dd_config after
every account check succeeds, then call _reload_instructions; ensure that on any
failure you return (False, error_msg) without mutating self.dd_config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5fcdedc-4b08-4d4d-b65d-2a4df1f43060
📒 Files selected for processing (17)
docs/data-sources/builtin-toolsets/datadog.mdholmes/core/tools.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/datadog_general_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2holmes/plugins/toolsets/datadog/datadog_url_utils.pyholmes/plugins/toolsets/datadog/instructions_datadog_traces.jinja2holmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pytests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/test_datadog_accounts.pytests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/plugins/toolsets/datadog/traces/test_datadog_traces.py
✅ Files skipped from review due to trivial changes (1)
- docs/data-sources/builtin-toolsets/datadog.md
🚧 Files skipped from review as they are similar to previous changes (13)
- holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2
- holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2
- holmes/plugins/toolsets/datadog/instructions_datadog_traces.jinja2
- tests/plugins/toolsets/datadog/test_datadog_accounts.py
- tests/plugins/toolsets/datadog/traces/test_datadog_traces.py
- tests/plugins/toolsets/datadog/logs/test_check_prerequisites.py
- tests/plugins/toolsets/datadog/test_toolset_datadog_general.py
- holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
- holmes/plugins/toolsets/datadog/toolset_datadog_traces.py
- tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
- holmes/plugins/toolsets/datadog/datadog_api.py
- holmes/plugins/toolsets/datadog/datadog_url_utils.py
acdfae7 to
8c68429
Compare
Many organizations run separate Datadog orgs for staging and production. Until now a single Holmes instance could only query one Datadog org per toolset, which meant maintaining two Holmes deployments or writing brittle custom toolset YAML. This change adds first-class multi-account support to all four Datadog toolsets (logs, metrics, traces, general) while keeping full backward compatibility with the existing single-account configuration. ## Config schema (datadog_api.py) - New DatadogAccount Pydantic model holds per-account credentials (name, api_key, app_key, api_url, default). api_url defaults to https://api.datadoghq.com (US1), preserving the upstream default. api_key and app_key have min_length=1 to fail fast on blank strings. - DatadogBaseConfig gains accounts: List[DatadogAccount]. A model_validator normalises the legacy single-account shorthand (api_key/app_key/api_url at top level) into a one-element list named 'default'. api_url is omitted from the DatadogAccount constructor when None so the field default applies correctly. - _ui_required_fields = ['api_key', 'app_key'] keeps the UI schema requiring credentials; _hidden_fields = ['accounts'] hides the complex nested field from form UIs. - get_account(name) resolves the per-call account name or returns the default; raises KeyError listing valid names for LLM self-correction. - get_headers(account) takes a DatadogAccount instead of the full config. ## Per-toolset changes (logs, metrics, traces, general) - Every tool gains an optional account string parameter. - _invoke resolves the account via get_account() and uses it for get_headers() and all URL constructors. - _perform_healthcheck takes a DatadogAccount; prerequisites_callable iterates all accounts and fails fast on the first unhealthy one. - _reload_instructions() passes dd_config to the Jinja context so the system prompt lists available accounts when more than one is present. ## Jinja instruction templates (all four) Guard block listing accounts only when more than one is configured. Single-account users see no change in their system prompt. ## datadog_url_utils.py - URL generators accept DatadogAccount instead of the full typed config. - generate_datadog_logs_url: use '*' not in indexes instead of indexes \!= ['*'] to treat any list containing '*' as unfiltered. ## Tests (67 pass, 16 skipped/slow) - New test_datadog_accounts.py: config parsing, validation edge cases, get_account() resolution. - Account-routing tests for all four toolsets. - Updated existing assertions for new account-scoped error messages. ## Docs Added 'Multiple Datadog accounts' section to the Datadog toolset docs. Signed-off-by: mdecalf <maxime.decalf@ledger.fr>
8c68429 to
c34e7fa
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # directly in their YAML/Helm values. | ||
| _ui_required_fields: ClassVar[List[str]] = ["api_key", "app_key"] | ||
| _hidden_fields: ClassVar[List[str]] = ["accounts"] |
There was a problem hiding this comment.
🔴 The new _hidden_fields = ["accounts"] on DatadogBaseConfig (datadog_api.py:184) is silently overridden for two of the four sibling toolsets: DatadogTracesConfig and DatadogLogsConfig already define their own _hidden_fields = ["indexes"] in datadog_models.py:50,65, and a ClassVar redefined in a subclass shadows the parent's rather than merging. Because build_schema_entry and build_config_example in holmes/utils/pydantic_utils.py read the attribute directly (no MRO walk), the nested accounts field leaks into both the form-UI JSON schema and the generated YAML example for datadog/logs and datadog/traces — directly contradicting the comment immediately above line 184. Easiest fix: extend each subclass to _hidden_fields = ["accounts", "indexes"]; alternatively, change the two consumers in pydantic_utils.py to walk the MRO and union the values.
Extended reasoning...
What the bug is. This PR adds _hidden_fields: ClassVar[List[str]] = ["accounts"] on DatadogBaseConfig (holmes/plugins/toolsets/datadog/datadog_api.py:184) with the explicit goal — stated in the comment immediately above the line — of hiding the nested accounts list from the form UI and the generated YAML example, because nested complex objects don't render well in form inputs. This works for the two sibling subclasses that don't otherwise touch the attribute (DatadogMetricsConfig at datadog_models.py:36, DatadogGeneralConfig at datadog_models.py:121). It silently breaks for the other two:\n\n- DatadogTracesConfig (holmes/plugins/toolsets/datadog/datadog_models.py:50): _hidden_fields: ClassVar[List[str]] = ["indexes"]\n- DatadogLogsConfig (holmes/plugins/toolsets/datadog/datadog_models.py:65): _hidden_fields: ClassVar[List[str]] = ["indexes"]\n\nBoth of those subclasses redefine the ClassVar — which in Python shadows rather than merges with the parent — so for those two classes cls._hidden_fields == ["indexes"] and accounts is not hidden at all.\n\nThe code path. Both consumers in holmes/utils/pydantic_utils.py read the attribute directly off the most-derived class, with no MRO walking:\n\n- build_schema_entry (pydantic_utils.py:103): hidden = list(cls._hidden_fields or []) — feeds Toolset.get_config_schema(), which is what the frontend's config form consumes.\n- build_config_example (pydantic_utils.py:235): hidden_fields = set(getattr(model_cls, "_hidden_fields", []) or []) — feeds the generated YAML example.\n\nSo for the datadog/logs and datadog/traces toolsets, the form schema gets the complex nested accounts field as a top-level form field, and the YAML example emits accounts: [] — the exact opposite of the PR's stated UX goal. The four sibling toolsets are now gratuitously inconsistent.\n\nWhy existing tests don't catch it. The new tests/plugins/toolsets/datadog/test_datadog_accounts.py suite exercises only the validator and the get_account() resolver. Nothing covers build_schema_entry / build_config_example on DatadogLogsConfig / DatadogTracesConfig, so the regression sails through CI.\n\nImpact. UI/UX regression introduced by this PR (not a runtime crash): two of the four toolset config forms expose a nested object input the author explicitly tried to hide, and two of the four example YAMLs include a stray accounts: [] line that confuses single-account users. The intent is documented inline; the code doesn't match it.\n\nStep-by-step proof. Take the on-disk classes:\n\npython\nclass DatadogBaseConfig(ToolsetConfig):\n _hidden_fields: ClassVar[List[str]] = ["accounts"] # datadog_api.py:184\n ...\n\nclass DatadogLogsConfig(DatadogBaseConfig):\n _hidden_fields: ClassVar[List[str]] = ["indexes"] # datadog_models.py:65\n indexes: list[str] = Field(default_factory=lambda: ["*"])\n ...\n\nclass DatadogTracesConfig(DatadogBaseConfig):\n _hidden_fields: ClassVar[List[str]] = ["indexes"] # datadog_models.py:50\n indexes: list[str] = Field(default_factory=lambda: ["*"])\n\n\n1. DatadogLogsConfig._hidden_fields → ["indexes"] (NOT ["accounts", "indexes"]) — standard Python class attribute shadowing.\n2. build_schema_entry(DatadogLogsConfig) does hidden = list(cls._hidden_fields or []) → ["indexes"], then pops only "indexes" from raw_schema["properties"]. The schema returned to the frontend therefore contains accounts as a top-level property (with its nested DatadogAccount object shape).\n3. build_config_example(DatadogLogsConfig) does hidden_fields = set(getattr(model_cls, "_hidden_fields", []) or []) → {"indexes"}. The loop over model_cls.model_fields.items() then emits a key for accounts (whose default_factory=list produces []), so the example YAML contains accounts: [].\n4. Same reasoning, by identical class structure, applies to DatadogTracesConfig.\n5. Compare to DatadogMetricsConfig / DatadogGeneralConfig — neither redefines _hidden_fields, so attribute lookup walks up to DatadogBaseConfig and returns ["accounts"], correctly hiding the field. Hence the gratuitous inconsistency across the four sibling toolsets, with the two more-customised ones being the broken pair.\n\nFix. Either of:\n\n1. Local one-line per subclass — extend the lists so they union with the parent's intent: _hidden_fields: ClassVar[List[str]] = ["accounts", "indexes"] on DatadogLogsConfig and DatadogTracesConfig. Trivial and keeps the consumers simple.\n2. Structural — change the two consumers in holmes/utils/pydantic_utils.py (build_schema_entry and build_config_example) to merge _hidden_fields across the MRO (e.g. set().union(*[getattr(c, "_hidden_fields", []) or [] for c in cls.__mro__])), so subclass overrides extend rather than replace. This makes the contract less surprising for any future subclass.
There was a problem hiding this comment.
Fixed in 55be474. Went with the subclass-extension approach: both DatadogTracesConfig and DatadogLogsConfig now declare _hidden_fields = ["accounts", "indexes"] explicitly, so the parent's "accounts" is never dropped by ClassVar shadowing. Skipped the MRO-walking alternative in pydantic_utils.py to keep the change minimal and localised.
| dd_config = DatadogLogsConfig(**config) | ||
| self.dd_config = dd_config | ||
|
|
||
| success, error_msg = self._perform_healthcheck() | ||
| return success, error_msg | ||
| for account in dd_config.accounts: | ||
| success, error_msg = self._perform_healthcheck(account) | ||
| if not success: | ||
| return False, error_msg | ||
| self._reload_instructions() |
There was a problem hiding this comment.
🟡 Nit: inconsistent with the rest of this PR — self.dd_config = dd_config is assigned before the per-account healthcheck loop (line 150) here, whereas the refactor in this PR moved it after the loop in the metrics, traces, and general toolsets. The structural reason is that _perform_healthcheck reads self.dd_config.indexes and self.dd_config.timeout_seconds from the instance rather than from a parameter. Real user-visible impact is small (status=FAILED gates tool invocation), but the fix is trivial: change _perform_healthcheck(self, account) to _perform_healthcheck(self, account, dd_config), read dd_config.indexes / dd_config.timeout_seconds, and move self.dd_config = dd_config to after the loop — matching the pattern the PR already applies to the other three sibling toolsets.
Extended reasoning...
What's inconsistent
This PR deliberately refactored prerequisites_callable in three of the four Datadog toolsets so that self.dd_config is mutated only after the per-account healthcheck loop succeeds:
toolset_datadog_metrics.py:838-848— loop first,self.dd_config = dd_configafter.toolset_datadog_traces.py:78-83— loop first,self.dd_config = dd_configafter.toolset_datadog_general.py:254-260— loop first,self.dd_config = dd_configafter.
The logs toolset kept the pre-PR ordering — assignment happens at toolset_datadog_logs.py:150, before the new multi-account loop at lines 152-155:
dd_config = DatadogLogsConfig(**config)
self.dd_config = dd_config # <-- assigned BEFORE the loop
for account in dd_config.accounts:
success, error_msg = self._perform_healthcheck(account)
if not success:
return False, error_msgWhy logs was written this way
The other three toolsets pass dd_config as a parameter into _perform_healthcheck(self, account, dd_config). The logs version takes only (self, account) and reads self.dd_config.indexes (line 98) and self.dd_config.timeout_seconds (line 108) off the instance attribute — that's what forces self.dd_config to be populated before the loop runs.
Concrete reasoning, step by step
- A previously-healthy
DatadogLogsToolsetis sitting with a validself.dd_config(e.g. account A — healthy). - A reconfigure code path re-invokes
prerequisites_callable(new_config)on the same instance. (ToolsetManager.refresh_server_toolsets_and_get_changes/refresh_toolsets_and_get_changescan re-run prerequisites on existing toolset instances in some flows.) - The new config has two accounts; account[1]'s credentials are wrong.
- Line 150 runs first:
self.dd_config = new_dd_config. The previously-valid config is now gone. - The loop hits account[1], healthcheck fails, function returns
(False, error_msg). - The toolset transitions to
status=FAILED, butself.dd_confignow references the half-validated new config (account[0] passed, account[1] failed).
In the metrics/traces/general toolsets, step 4 doesn't happen — self.dd_config retains its prior valid value because the assignment is gated on the loop succeeding.
Why I'm calling this a nit, not a normal bug
The failure path requires both (a) a reconfigure flow that re-invokes prerequisites_callable on a live instance, and (b) a caller that reads self.toolset.dd_config without first checking status. The standard tool-invocation path is gated by tool_executor on status == FAILED and rejects the call with "Toolset is unavailable", and _reload_instructions is only called on the success path (line 156). So the user-visible blast radius is essentially nil today.
That said, the inconsistency with the PR's own pattern for the three sibling toolsets is real, the new multi-account loop is what introduces the partial-validation surface area (pre-PR there was a single healthcheck call, so the half-validated state wasn't reachable), and the fix is mechanical.
Suggested fix
Match what the PR already does for the other three toolsets:
def _perform_healthcheck(
self, account: DatadogAccount, dd_config: DatadogLogsConfig
) -> Tuple[bool, str]:
headers = get_headers(account)
payload = {
"filter": {
"from": "now-1m",
"to": "now",
"query": "*",
"indexes": dd_config.indexes, # was self.dd_config.indexes
},
"page": {"limit": 1},
}
search_url = f"{account.api_url}/api/v2/logs/events/search"
execute_datadog_http_request(
url=search_url,
headers=headers,
payload_or_params=payload,
timeout=dd_config.timeout_seconds, # was self.dd_config.timeout_seconds
method="POST",
)
...
def prerequisites_callable(self, config: dict[str, Any]) -> Tuple[bool, str]:
...
dd_config = DatadogLogsConfig(**config)
for account in dd_config.accounts:
success, error_msg = self._perform_healthcheck(account, dd_config)
if not success:
return False, error_msg
self.dd_config = dd_config # moved here, matches the other three
self._reload_instructions()
return True, ""The if not self.dd_config guard at the top of _perform_healthcheck can also be dropped since the function no longer depends on instance state.
There was a problem hiding this comment.
Fixed in 55be474. _perform_healthcheck now takes dd_config: DatadogLogsConfig as an explicit parameter (reads dd_config.indexes / dd_config.timeout_seconds directly instead of self.dd_config), the if not self.dd_config guard removed, and self.dd_config = dd_config moved to after the per-account loop — identical pattern to metrics and traces.
| default_time_span_seconds=ACTIVE_METRICS_DEFAULT_TIME_SPAN_SECONDS, | ||
| ) | ||
|
|
||
| url = f"{self.toolset.dd_config.api_url}/api/v1/metrics" | ||
| headers = get_headers(self.toolset.dd_config) | ||
| url = f"{account.api_url}/api/v1/metrics" |
There was a problem hiding this comment.
🟣 Pre-existing: URL builders in the logs, metrics, and traces toolsets use raw f-string concatenation against account.api_url (e.g. f"{account.api_url}/api/v1/metrics" at toolset_datadog_metrics.py:143), but Pydantic v2 AnyUrl normalizes any user-supplied URL by appending a trailing slash — so a user setting api_url: https://api.datadoghq.eu yields https://api.datadoghq.eu//api/v1/metrics. The datadog/general toolset already does the correct str(account.api_url).rstrip("/") (toolset_datadog_general.py:470, 679); the same one-liner is missing from the other three toolsets at logs:103,258; metrics:143,360,581,699,800; traces:116,294,668. Existing behavior — pre-PR dd_config.api_url was the same AnyUrl type and produced the same double slash — but since this PR refactors every URL builder, it's a natural opportunity to fix it consistently (ideally by stripping the slash once in a DatadogAccount validator). The new account-routing tests only assert startswith(...) so they don't catch the bug.
Extended reasoning...
What's happening
Pydantic v2's AnyUrl validator normalizes URLs supplied by users by appending a trailing slash to the path. The literal default value declared on the field stays unnormalized (because Pydantic v2 doesn't run validators on field defaults unless validate_default=True), but any user-provided value is normalized. Reproducer:
from pydantic import BaseModel, AnyUrl, Field
class A(BaseModel): url: AnyUrl = Field(default="https://api.datadoghq.com")
str(A().url) # 'https://api.datadoghq.com' (no slash — default literal stays as-is)
str(A(url='https://api.datadoghq.eu').url) # 'https://api.datadoghq.eu/' (trailing slash appended)How it manifests in this PR
The new DatadogAccount model has api_url: AnyUrl = Field(default="https://api.datadoghq.com"). In the new multi-account accounts: list form, every account MUST supply api_url (there's no default-from-parent behavior in _normalize_accounts), so the trailing slash always gets appended. The legacy shorthand also hits this when a user explicitly sets api_url.
Then 10 sites refactored by this PR build URLs by raw f-string concat:
toolset_datadog_logs.py:103, 258toolset_datadog_metrics.py:143, 360, 581, 699, 800toolset_datadog_traces.py:116, 294, 668
All of the form f"{account.api_url}/api/v2/…" — producing https://api.datadoghq.eu//api/v2/… for any user-supplied URL.
Step-by-step proof
- User configures
accounts: [{name: prod, api_key: …, app_key: …, api_url: https://api.datadoghq.eu}]. - Pydantic constructs
DatadogAccount(api_url=AnyUrl('https://api.datadoghq.eu'))→ normalized tohttps://api.datadoghq.eu/. - Tool call hits e.g.
ListActiveMetrics._invokeattoolset_datadog_metrics.py:140-143:url = f"{account.api_url}/api/v1/metrics"
str(AnyUrl('https://api.datadoghq.eu/'))is'https://api.datadoghq.eu/', so the f-string produces'https://api.datadoghq.eu//api/v1/metrics'— note the double slash beforeapi.
Why existing code doesn't prevent it
The PR author was clearly aware of the issue: toolset_datadog_general.py:470, 679 (and datadog_url_utils.py's convert_api_url_to_app_url which explicitly strips the trailing slash) defensively use str(account.api_url).rstrip("/"). The pattern just wasn't carried over when refactoring the other three toolsets. The new account-routing tests only assert url.startswith("https://api.stg.datadoghq.eu"), which a double-slash URL passes trivially, so the bug isn't caught by the new test suite.
Impact
Datadog's edge/LB tolerance for // in paths varies by region. In many regions Datadog silently normalizes the path and the call succeeds; in others (or behind certain WAF/edge configs) the request 400s. So this is a latent reliability bug whose symptoms depend on which Datadog site a user is on. It also produces ugly, inconsistent URLs in logs and error messages.
Why pre-existing
Before this PR, DatadogBaseConfig.api_url was a required AnyUrl (also normalized by Pydantic), and pre-PR call sites had the same f-string pattern (e.g. pre-PR f"{self.toolset.dd_config.api_url}/api/v1/metrics"). So users who explicitly set api_url were already affected. The PR doesn't introduce the bug or worsen its reach — it just preserves the pre-existing inconsistency between general (rstrip applied) and logs/metrics/traces (no rstrip). The submitter's claim that the new accounts: list "materially increases reach" because api_url is mandatory there is misleading — pre-PR api_url was also required.
Suggested fix
The cleanest fix is one-line, applied once on the model so every consumer is safe — add a validator to DatadogAccount:
@field_validator("api_url", mode="after")
@classmethod
def _strip_trailing_slash(cls, v: AnyUrl) -> AnyUrl:
s = str(v).rstrip("/")
return AnyUrl(s)Alternatively, apply str(account.api_url).rstrip("/") at the 10 missing call sites to match general. This is a good opportunity-of-fix since the PR is already touching every affected line.
There was a problem hiding this comment.
Fixed in 55be474. Applied str(account.api_url).rstrip("/") at all 10 URL construction sites across logs (2), metrics (5) and traces (3), consistent with the pattern already used in toolset_datadog_general.py. Went with the call-site approach rather than a DatadogAccount validator to avoid Pydantic v2 AnyUrl type reconstruction complexity.
- Extend _hidden_fields in DatadogTracesConfig and DatadogLogsConfig to
include "accounts" alongside "indexes", preventing the parent ClassVar
from being shadowed and leaking `accounts` into the form-UI JSON schema.
- Align DatadogLogsToolset._perform_healthcheck signature with the metrics
and traces pattern: accept dd_config as an explicit parameter and move
self.dd_config assignment to after the per-account healthcheck loop.
- Add str(...).rstrip("/") to all 10 URL construction sites in logs,
metrics and traces toolsets to prevent Pydantic v2 AnyUrl trailing-slash
from producing double-slash paths (e.g. https://api.datadoghq.eu//api/v1).
Consistent with the existing pattern already used in the general toolset.
2 participants
Closes #2065
Problem
Many organizations run separate Datadog organizations for staging and production. Until now a single Holmes instance could only query one Datadog org per toolset, which meant either running two Holmes deployments or writing brittle custom-toolset YAML. This PR adds first-class multi-account support to all four Datadog toolsets while keeping the existing single-account configuration entirely unchanged.
Solution
A new
accounts:list on each toolset config lets users declare N Datadog accounts. The LLM picks the target account on every tool call via a new optionalaccountparameter. When only one account is configured (including all existing deployments), the parameter is a no-op — behavior is identical to today.Before (unchanged, still works)
After (new multi-account form)
Changes
datadog_api.pyDatadogAccountmodel holds per-account credentials (name,api_key,app_key,api_url,default).api_urldefaults tohttps://api.datadoghq.compreserving the existing upstream default.DatadogBaseConfiggainsaccounts: List[DatadogAccount]. A@model_validatornormalises the single-account shorthand into a one-element[DatadogAccount(name="default", ...)], so zero migration is required._ui_required_fields = ["api_key", "app_key"]keeps the UI form marking credentials as required;_hidden_fields = ["accounts"]hides the complex nested field from the form UI (advanced users configure it via YAML).get_account(name)resolves the per-call account name, or returns the default; raisesKeyErrorlisting available names so the LLM can self-correct on retry.get_headers(account)now takes aDatadogAccountinstead of the full config.Four toolsets (logs, metrics, traces, general)
accountstring parameter._invokeresolves the account viaget_account()and passes it toget_headers()and all URL constructors._perform_healthchecktakes aDatadogAccount;prerequisites_callableiterates all accounts and fails fast on the first unhealthy one, embedding the account name in the error message._reload_instructions()passes the validateddd_configto the Jinja context so the system prompt lists available accounts and the default.Jinja instruction templates (all four)
Added a guard block listing configured accounts only when more than one is present. Single-account users see no change in their system prompt.
datadog_url_utils.pyURL generators accept
DatadogAccountinstead of the full typed config, decoupling URL building from toolset config.Tests (67 pass, 16 skipped/slow)
tests/plugins/toolsets/datadog/test_datadog_accounts.py(new, 11 tests): config parsing for both schemas, validation edge cases, andget_account()resolution.Docs
Added a "Multiple Datadog accounts" section to
docs/data-sources/builtin-toolsets/datadog.md.Backward compatibility
api_key/app_key/api_urlconfigapi_url(defaults to US1)dd_api_key,site_api_url, …)accounts:keyaccounts:listValueError(intentional)Note on
llm_instructionstiming_reload_instructions()is called from both__init__(renders withdd_config=None, template guard emits nothing) and fromprerequisites_callable(renders with the validated config including the account list). Becausecheck_prerequisitesruns before the first LLM conversation in all deployment paths, the account list is always present in the system prompt when it matters.Summary by CodeRabbit
New Features
Documentation
Tests