Datadog improvements - #973
Conversation
WalkthroughAdds Datadog OpenAPI/time-preprocessing utilities, generates Datadog UI deep links for logs and metrics, normalizes time fields before API calls, surfaces per-tier "View in Datadog" links and richer diagnostics on success/errors, updates prompts/tests/passthroughs, and adjusts minor logging and capability declarations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as DatadogLogsToolset
participant util as datadog_api utils
participant API as Datadog API
U->>L: fetch_pod_logs(params)
rect rgba(200,220,255,0.18)
L->>util: preprocess_time_fields(payload, "/api/v2/logs/events/search")
util-->>L: processed_payload
end
loop per page / per storage tier
L->>API: POST /api/v2/logs/events/search (processed_payload + pagination)
alt 200 OK
API-->>L: logs page (+cursor)
L->>L: accumulate results
opt final page or success
L->>util: generate_datadog_logs_url(dd_config, params, tier)
util-->>L: per-tier URL
end
else 429
API-->>L: rate-limited
L->>util: generate_datadog_logs_url(...)
util-->>L: URL
L-->>U: error with rate-limit info + URL
else 400
API-->>L: bad request
L->>util: enhance_error_message(error, endpoint, method, base)
util-->>L: augmented message
L->>util: generate_datadog_logs_url(...)
util-->>L: URL
L-->>U: error + URL
else other
API-->>L: error
L-->>U: diagnostics + URL
end
end
alt no data
L-->>U: NO_DATA + diagnostics + per-tier URLs
else success
L-->>U: logs output + "View in Datadog" link(s)
end
sequenceDiagram
autonumber
participant M as DatadogMetricsToolset
participant util as datadog_api utils
participant API as Datadog API
participant U as User
M->>API: metrics query (with timestamps)
alt 200 with series
API-->>M: series
M->>M: transform series, add RFC3339 start/end
M->>util: convert_api_url_to_app_url + build metrics UI URL
util-->>M: metrics UI URL
M-->>U: results + URL
else 400
API-->>M: bad request
M->>util: enhance_error_message(error, endpoint, method, base)
util-->>M: detailed message
M-->>U: error + best-effort URL
else no data
API-->>M: empty
M-->>U: no-data message + optional URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Actionable comments posted: 5
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)
385-387: Use UTC when emitting RFC3339 timestamps.datetime.fromtimestamp() is localtime; you append “Z” (UTC). Use utcfromtimestamp().
Apply:
- start_rfc = datetime.fromtimestamp(from_time).strftime("%Y-%m-%dT%H:%M:%SZ") - end_rfc = datetime.fromtimestamp(to_time).strftime("%Y-%m-%dT%H:%M:%SZ") + start_rfc = datetime.utcfromtimestamp(from_time).strftime("%Y-%m-%dT%H:%M:%SZ") + end_rfc = datetime.utcfromtimestamp(to_time).strftime("%Y-%m-%dT%H:%M:%SZ")
🧹 Nitpick comments (4)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (2)
239-266: NO_DATA: include the link in the StructuredToolResult.url, not only in error text.Keeps consumers/UI integration consistent.
Apply in the return below:
return StructuredToolResult( status=StructuredToolResultStatus.NO_DATA, - error=error_msg, + error=error_msg, + url=datadog_url, params=params.model_dump(), )
280-305: ERROR path: set url field when available; minor cleanups.Provide the Datadog deep link in StructuredToolResult.url for better UX.
Apply in the return below:
return StructuredToolResult( status=StructuredToolResultStatus.ERROR, error=error_msg, params=params.model_dump(), invocation=json.dumps(e.payload), + url=datadog_url if e.status_code != 429 else None, )Also consider using filter_str (as in the URL helper) when building the query in messages to avoid quoting pitfalls.
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
330-352: QueryMetrics NO_DATA: wrong param names for time range.Use from_time/to_time, not start_time/end_time.
Apply:
- start_time = params.get("start_time") - end_time = params.get("end_time") + start_time = params.get("from_time") + end_time = params.get("to_time")
661-697: ListMetricTags error context references non-existent params.This endpoint has no query or time range; drop that noise and the Datadog URL attempt.
Apply:
- if params: - error_msg += f"\nQuery: {params.get('query', 'not specified')}" - - start_time = params.get("start_time") - end_time = params.get("end_time") - - if start_time: - start_desc = start_time - else: - start_desc = ( - f"default (last {DEFAULT_TIME_SPAN_SECONDS // 86400} days)" - ) - - end_desc = end_time or "now" - error_msg += f"\nTime range: {start_desc} to {end_desc}" - - # Add Datadog web UI URL even for errors (if we have time info) - if "query" in params and params.get("query"): - try: - (from_time, to_time) = process_timestamps_to_int( - start=params.get("from_time"), - end=params.get("to_time"), - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) - datadog_url = generate_datadog_metrics_url( - self.toolset.dd_config, - params.get("query", ""), - from_time, - to_time, - ) - error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + # No query/time context available for this endpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(4 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting
Type hints are required (checked by mypy)
Always place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (3)
holmes/plugins/toolsets/logging_utils/logging_api.py (1)
FetchPodLogsParams(42-57)holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_rfc3339(75-87)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_int(90-136)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
217-218: try-except-pass detected, consider logging the exception
(S110)
217-217: Do not catch blind exception: Exception
(BLE001)
456-457: try-except-pass detected, consider logging the exception
(S110)
456-456: Do not catch blind exception: Exception
(BLE001)
696-697: try-except-pass detected, consider logging the exception
(S110)
696-696: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (2)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
6-6: Imports placement LGTM.holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
5-5: Imports placement LGTM.
There was a problem hiding this comment.
Actionable comments posted: 4
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/datadog/datadog_api.py (2)
157-168: Sanitize headers robustly; don’t drop all headers on non‑string valuesThe current try/except returns {} on any non‑string header value. Redact safely and stringify.
-def sanitize_headers(headers: Union[dict, CaseInsensitiveDict]) -> dict: - try: - return { - k: v - if ("key" not in k.lower() and "key" not in v.lower()) - else "[REDACTED]" - for k, v in headers.items() - } - except (AttributeError, TypeError): - # Return empty dict for mock objects or other non-dict types - return {} +def sanitize_headers(headers: Union[dict, CaseInsensitiveDict]) -> dict: + try: + redacted: dict[str, str] = {} + for k, v in headers.items(): + ks = str(k).lower() + vs = str(v).lower() if isinstance(v, str) else "" + if "key" in ks or "key" in vs: + redacted[str(k)] = "[REDACTED]" + else: + redacted[str(k)] = str(v) + return redacted + except Exception: + return {}
97-121: Rate‑limit wait misinterprets X-RateLimit-Reset (epoch vs seconds)Handle both “seconds to wait” and “epoch seconds” to avoid long sleeps.
- reset_time_header = exc.response_headers.get( + reset_time_header = exc.response_headers.get( RATE_LIMIT_REMAINING_SECONDS_HEADER ) if reset_time_header: try: - reset_time = int(reset_time_header) - wait_time = max(0, reset_time) + 0.1 + reset_time = int(reset_time_header) + # If looks like epoch (e.g., > 1e6), convert to delta. + if reset_time > 1_000_000: + import time + delta = max(0, reset_time - int(time.time())) + wait_time = delta + 0.1 + else: + wait_time = max(0, reset_time) + 0.1 return wait_timeholmes/plugins/toolsets/datadog/toolset_datadog_traces.py (1)
93-111: Healthcheck sends unsupported relative times (“now-1m”, “now”) to v2 searchThis will likely 400. Use epoch ms like the other calls.
- payload = { + now_s = int(time.time()) + from_time_ms = (now_s - 60) * 1000 + to_time_ms = now_s * 1000 + payload = { "data": { "type": "search_request", "attributes": { "filter": { - "from": "now-1m", - "to": "now", + "from": str(from_time_ms), + "to": str(to_time_ms), "query": "*", "indexes": dd_config.indexes, }, "page": {"limit": 1}, }, } }
♻️ Duplicate comments (5)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (2)
148-189: Fix domain replacement and use epoch milliseconds for deep links.The Datadog Logs Explorer expects
from_tsandto_tsas Unix milliseconds for deep links. The current implementation has two issues:
- The domain replacement pattern won't match
https://api.datadoghq.comcorrectly- RFC3339 timestamps are used instead of epoch milliseconds
Apply this fix:
def generate_datadog_logs_url( dd_config: DatadogLogsConfig, params: FetchPodLogsParams, storage_tier: DataDogStorageTier, ) -> str: """Generate a Datadog web UI URL for the logs query.""" # Extract the base domain from the API URL - # Convert https://api.datadoghq.com to https://app.datadoghq.com - # or https://api.datadoghq.eu to https://app.datadoghq.eu - base_url = str(dd_config.site_api_url).replace("/api.", "/app.") + # Convert e.g. https://api.datadoghq.com → https://app.datadoghq.com + base_url = str(dd_config.site_api_url).replace("://api.", "://app.") if base_url.endswith("/"): base_url = base_url[:-1] # Build the query string query = f"{dd_config.labels.namespace}:{params.namespace}" query += f" {dd_config.labels.pod}:{params.pod_name}" if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + filter_str = params.filter.replace('"', '\\"') + query += f' "{filter_str}"' - # Process timestamps for URL parameters - (from_time, to_time) = process_timestamps_to_rfc3339( - start_timestamp=params.start_time, - end_timestamp=params.end_time, - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) + # Process timestamps for URL parameters (epoch ms for UI) + from_time, to_time = process_timestamps_to_int( + start=params.start_time, + end=params.end_time, + default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, + ) + from_ms = from_time * 1000 + to_ms = to_time * 1000 # Build URL parameters url_params = { "query": query, - "from_ts": from_time, - "to_ts": to_time, + "from_ts": str(from_ms), + "to_ts": str(to_ms), "storage": storage_tier.value, }Also add the missing import:
from holmes.plugins.toolsets.utils import process_timestamps_to_int
235-244: Use StructuredToolResult.url field for Datadog link.The Datadog link should be exposed via the
urlfield instead of being appended to the data.if raw_logs: logs_str = format_logs(raw_logs) # Generate Datadog web UI URL datadog_url = generate_datadog_logs_url( self.dd_config, params, storage_tier ) - logs_with_link = f"{logs_str}\n\nView in Datadog: {datadog_url}" return StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, - data=logs_with_link, + data=logs_str, + url=datadog_url, params=params.model_dump(), )holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (3)
46-73: Fix domain replacement for metrics URL generation.The domain replacement pattern won't correctly convert
https://api.datadoghq.comtohttps://app.datadoghq.com.def generate_datadog_metrics_url( dd_config: DatadogMetricsConfig, query: str, from_time: int, to_time: int, ) -> str: """Generate a Datadog web UI URL for the metrics query.""" # Extract the base domain from the API URL # Convert https://api.datadoghq.com to https://app.datadoghq.com - base_url = str(dd_config.site_api_url).replace("/api.", "/app.") + base_url = str(dd_config.site_api_url).replace("://api.", "://app.") if base_url.endswith("/"): base_url = base_url[:-1]Based on the verification, the Metrics Explorer expects
startandendparameters (notfrom_ts/to_ts). Consider updating the URL parameters:# Build URL parameters url_params = { "live": "false", - "from_ts": str(from_ms), - "to_ts": str(to_ms), + "start": str(from_ms), + "end": str(to_ms), "query": query, }
182-219: Fix incorrect parameter names in ListActiveMetrics error handling.The error handling uses incorrect parameter names (
start_time/end_timeandquery) that don't exist in this tool's parameters. ListActiveMetrics only hasfrom_time,host, andtag_filter.else: # Include full API error details for better debugging error_msg = ( f"Datadog API error (status {e.status_code}): {e.response_text}" ) if params: - error_msg += f"\nQuery: {params.get('query', 'not specified')}" - - start_time = params.get("start_time") - end_time = params.get("end_time") - - if start_time: - start_desc = start_time - else: - start_desc = ( - f"default (last {DEFAULT_TIME_SPAN_SECONDS // 86400} days)" - ) - - end_desc = end_time or "now" - error_msg += f"\nTime range: {start_desc} to {end_desc}" - - # Add Datadog web UI URL even for errors (if we have time info) - if "query" in params and params.get("query"): - try: - (from_time, to_time) = process_timestamps_to_int( - start=params.get("from_time"), - end=params.get("to_time"), - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) - datadog_url = generate_datadog_metrics_url( - self.toolset.dd_config, - params.get("query", ""), - from_time, - to_time, - ) - error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + host = params.get("host") or "<any>" + tag_filter = params.get("tag_filter") or "<none>" + error_msg += f"\nFilters: host={host}, tag_filter={tag_filter}" + from_desc = params.get("from_time") or f"default (last {ACTIVE_METRICS_DEFAULT_LOOK_BACK_HOURS} hours)" + error_msg += f"\nTime range: {from_desc} to now"Also remove the bare except/pass (Lines 217-218) as it violates Ruff rules S110 and BLE001.
421-458: Fix parameter names in QueryMetrics error handling.The error handling uses
start_time/end_timebut the actual parameters arefrom_time/to_time.else: # Include full API error details for better debugging error_msg = ( f"Datadog API error (status {e.status_code}): {e.response_text}" ) if params: error_msg += f"\nQuery: {params.get('query', 'not specified')}" - start_time = params.get("start_time") - end_time = params.get("end_time") + start_time = params.get("from_time") + end_time = params.get("to_time")Also replace the bare except/pass (Lines 456-457) with proper error handling:
- except Exception: - pass # Skip URL generation if timestamps can't be processed + except (TypeError, ValueError) as ex: + logging.debug("Skipping URL generation: %s", ex)
🧹 Nitpick comments (15)
holmes/plugins/toolsets/datadog/datadog_api.py (6)
227-235: Rename unused parameter to satisfy Ruff and guidelinessite_api_url isn’t used. Keep for compatibility but underscore it.
-def fetch_openapi_spec( - site_api_url: Optional[str] = None, version: str = "both" +def fetch_openapi_spec( + site_api_url: Optional[str] = None, version: str = "both" ) -> Optional[Dict[str, Any]]: + _ = site_api_url # kept for compatibility
312-315: Use logging.exception for traceback, don’t catch blind Exception- except Exception as e: - logging.error(f"Failed to fetch spec for {ver}: {e}") + except Exception: + logging.exception(f"Failed to fetch spec for {ver}")
327-330: Same: prefer logging.exception, and avoid overly broad catches- except Exception as e: - logging.error(f"Error fetching OpenAPI spec: {e}") + except Exception: + logging.exception("Error fetching OpenAPI spec")
479-539: Avoid inner import; fix unused arg; no-op endpoint paramComply with guidelines and Ruff ARG001.
-def preprocess_time_fields(payload: Dict[str, Any], endpoint: str) -> Dict[str, Any]: +def preprocess_time_fields(payload: Dict[str, Any], _endpoint: str) -> Dict[str, Any]: @@ - # Deep copy to avoid modifying original - import copy + # Deep copy to avoid modifying original
541-554: site_api_url unused; rename for Ruff, or add link enrichmentRight now unused. Either underscore it or use it to add a “View in Datadog” URL.
-def enhance_error_message( - error: DataDogRequestError, endpoint: str, method: str, site_api_url: str +def enhance_error_message( + error: DataDogRequestError, endpoint: str, method: str, site_api_url: str ) -> str: + _ = site_api_url
177-186: Limit payload logging to avoid PII/large dumpsConsider truncating payload/params and using structured logs.
I can provide a small utility to pretty‑print with max length and redact keys like “query”, “filter.query” if desired.
holmes/plugins/toolsets/datadog/toolset_datadog_traces.py (1)
296-304: Tuple handling is dead code with execute_datadog_http_requestexecute_datadog_http_request never returns a tuple. Safe to simplify.
I can send a follow‑up diff to remove the tuple branches to reduce complexity.
Also applies to: 436-444, 644-654
tests/plugins/toolsets/datadog/logs/test_fetch_pod_logs.py (4)
82-95: Filtering “View in Datadog” line: OK; consider a helper to avoid string couplingMinor: a small util like strip_datadog_link(lines) keeps tests DRY and resilient to copy changes.
141-148: Same filtering pattern appears multiple timesExtract to a helper in the test module.
198-205: Same
258-264: Sameholmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
90-91: Avoid shadowing Python's built-infilterfunction.Line 90 shadows the built-in
filterfunction. While this is in a local scope and won't cause issues, it's better to use a different variable name for clarity.if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + filter_str = params.filter.replace('"', '\\"') + query += f' "{filter_str}"'holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
661-698: Remove irrelevant parameters from ListMetricTags error handling.ListMetricTags only has
metric_nameparameter, notquery,start_time, orend_time.else: # Include full API error details for better debugging error_msg = ( f"Datadog API error (status {e.status_code}): {e.response_text}" ) if params: - error_msg += f"\nQuery: {params.get('query', 'not specified')}" - - start_time = params.get("start_time") - end_time = params.get("end_time") - - if start_time: - start_desc = start_time - else: - start_desc = ( - f"default (last {DEFAULT_TIME_SPAN_SECONDS // 86400} days)" - ) - - end_desc = end_time or "now" - error_msg += f"\nTime range: {start_desc} to {end_desc}" - - # Add Datadog web UI URL even for errors (if we have time info) - if "query" in params and params.get("query"): - try: - (from_time, to_time) = process_timestamps_to_int( - start=params.get("from_time"), - end=params.get("to_time"), - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) - datadog_url = generate_datadog_metrics_url( - self.toolset.dd_config, - params.get("query", ""), - from_time, - to_time, - ) - error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + metric_name = params.get("metric_name", "unknown") + error_msg += f"\nMetric: {metric_name}"Also fix the bare except/pass issue (Lines 696-697).
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (2)
703-703: Remove unused parameteruser_approved.The
user_approvedparameter is not used in this method.def _invoke( - self, params: dict, user_approved: bool = False + self, params: dict ) -> StructuredToolResult:Note: If this is part of a base class interface requirement, then this can be ignored.
340-355: Consider extracting shared URL generation logic.The URL generation logic for converting API URLs to app URLs is duplicated across
toolset_datadog_logs.pyandtoolset_datadog_metrics.py. Consider creating a shared utility function.Would you like me to create a shared utility function
convert_api_to_app_url(site_api_url: str) -> strinholmes/plugins/toolsets/datadog/datadog_api.pythat can be reused across all Datadog toolsets?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
holmes/plugins/toolsets/datadog/datadog_api.py(3 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_general.py(15 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(7 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(11 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_rds.py(2 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_traces.py(3 hunks)tests/plugins/toolsets/datadog/logs/test_check_prerequisites.py(2 hunks)tests/plugins/toolsets/datadog/logs/test_fetch_pod_logs.py(4 hunks)tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/plugins/toolsets/datadog/toolset_datadog_rds.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting
Type hints are required (checked by mypy)
Always place Python imports at the top of the file, not inside functions or methods
Files:
tests/plugins/toolsets/datadog/logs/test_check_prerequisites.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/logs/test_fetch_pod_logs.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use pytest markers/tags that are not declared; only use markers listed in pyproject.toml
Files:
tests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/logs/test_fetch_pod_logs.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should mirror the source structure under tests/
Files:
tests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics.pytests/plugins/toolsets/datadog/logs/test_fetch_pod_logs.py
🧬 Code graph analysis (3)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (3)
holmes/plugins/toolsets/datadog/datadog_api.py (4)
preprocess_time_fields(479-538)enhance_error_message(541-634)fetch_openapi_spec(227-329)execute_datadog_http_request(170-224)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)holmes/core/tools.py (3)
ToolParameter(154-159)StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (4)
holmes/plugins/toolsets/datadog/datadog_api.py (2)
enhance_error_message(541-634)preprocess_time_fields(479-538)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
FetchPodLogsParams(42-57)holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_rfc3339(75-87)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_int(90-136)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/datadog/datadog_api.py
228-228: Unused function argument: site_api_url
(ARG001)
312-312: Do not catch blind exception: Exception
(BLE001)
313-313: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
327-327: Do not catch blind exception: Exception
(BLE001)
328-328: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
479-479: Unused function argument: endpoint
(ARG001)
542-542: Unused function argument: site_api_url
(ARG001)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py
703-703: Unused method argument: user_approved
(ARG002)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
217-218: try-except-pass detected, consider logging the exception
(S110)
217-217: Do not catch blind exception: Exception
(BLE001)
456-457: try-except-pass detected, consider logging the exception
(S110)
456-456: Do not catch blind exception: Exception
(BLE001)
696-697: try-except-pass detected, consider logging the exception
(S110)
696-696: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (8)
holmes/plugins/toolsets/datadog/toolset_datadog_traces.py (3)
160-160: Prefix in description: LGTMConsistent with other Datadog toolsets.
364-364: Prefix in description: LGTM
503-503: Prefix in description: LGTMtests/plugins/toolsets/datadog/logs/test_check_prerequisites.py (2)
20-23: Updated error copy: LGTMClearer guidance and doc link.
32-35: Updated error copy: LGTMtests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py (1)
303-306: Updated error copy: LGTMMatches logs toolset wording.
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (2)
242-252: Good addition: OpenAPI spec fetching on startup.The addition of OpenAPI spec fetching at startup is excellent for providing enhanced error messages and documentation. The error handling with a warning log when the spec can't be fetched is appropriate.
449-451: Good improvement: Automatic time field preprocessing.The addition of
preprocess_time_fieldsfor both GET and POST requests is a great improvement. This will automatically convert relative time formats (like '-24h', 'now') to the appropriate format for each API version, reducing user errors.Also applies to: 615-617
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2 (1)
1-6: Move template under holmes/plugins/prompts to satisfy project layout.Guideline: prompts must live under holmes/plugins/prompts/**/*.jinja2. Please relocate this file and update the loader path.
Proposed steps:
- git mv holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2 holmes/plugins/prompts/datadog_logs_instructions.jinja2
- In DatadogLogsToolset._reload_instructions, adjust template_file_path accordingly.
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
31-31: Import process_timestamps_to_int at top; avoid in-function import.Complies with project rule: no imports inside functions.
Apply:
-from holmes.plugins.toolsets.utils import process_timestamps_to_rfc3339 +from holmes.plugins.toolsets.utils import process_timestamps_to_rfc3339, process_timestamps_to_int
♻️ Duplicate comments (6)
holmes/plugins/toolsets/datadog/datadog_api.py (1)
21-26: Move in-function imports to module top (violates import placement rule).yaml and copy are imported inside functions; move them to the module header per guidelines.
Apply:
@@ -import re -from datetime import datetime, timedelta, timezone -from typing import Any, Optional, Dict, Union, Tuple +import re +from datetime import datetime, timedelta, timezone +from typing import Any, Optional, Dict, Union, Tuple +import copy +import yaml @@ - try: - import yaml + try: @@ - # Deep copy to avoid modifying original - import copy - - processed = copy.deepcopy(payload) + # Deep copy to avoid modifying original + processed = copy.deepcopy(payload)Also applies to: 231-233, 472-475
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (4)
46-73: Fix app-domain replacement and Metrics Explorer params.replace("/api.", "/app.") won’t match https://api.datadoghq.com. Use ://api. → ://app. and prefer start/end (ms) for Metrics Explorer deep links.
Apply:
def generate_datadog_metrics_url( @@ - # Convert https://api.datadoghq.com to https://app.datadoghq.com - base_url = str(dd_config.site_api_url).replace("/api.", "/app.") + # Convert e.g. https://api.datadoghq.com -> https://app.datadoghq.com + base_url = str(dd_config.site_api_url).replace("://api.", "://app.") if base_url.endswith("/"): base_url = base_url[:-1] @@ - url_params = { - "live": "false", - "from_ts": str(from_ms), - "to_ts": str(to_ms), - "query": query, - } + url_params = { + "live": "false", + "start": str(from_ms), + "end": str(to_ms), + "query": query, + }
182-219: Error context uses wrong params and hides exceptions.ListActiveMetrics has no query/start_time/end_time params. Show filters and look-back only; avoid bare except.
Apply:
- # Include full API error details for better debugging - error_msg = ( - f"Datadog API error (status {e.status_code}): {e.response_text}" - ) - if params: - error_msg += f"\nQuery: {params.get('query', 'not specified')}" - - start_time = params.get("start_time") - end_time = params.get("end_time") - - if start_time: - start_desc = start_time - else: - start_desc = ( - f"default (last {DEFAULT_TIME_SPAN_SECONDS // 86400} days)" - ) - - end_desc = end_time or "now" - error_msg += f"\nTime range: {start_desc} to {end_desc}" - - # Add Datadog web UI URL even for errors (if we have time info) - if "query" in params and params.get("query"): - try: - (from_time, to_time) = process_timestamps_to_int( - start=params.get("from_time"), - end=params.get("to_time"), - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) - datadog_url = generate_datadog_metrics_url( - self.toolset.dd_config, - params.get("query", ""), - from_time, - to_time, - ) - error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + error_msg = ( + f"Datadog API error (status {e.status_code}): {e.response_text}" + ) + if params: + host = params.get("host") or "<any>" + tag_filter = params.get("tag_filter") or "<none>" + look_back = params.get("from_time") or f"default (last {ACTIVE_METRICS_DEFAULT_LOOK_BACK_HOURS} hours)" + error_msg += f"\nFilters: host={host}, tag_filter={tag_filter}" + error_msg += f"\nTime range: {look_back} → now"
421-458: Align error context param names and avoid bare except.Use from_time/to_time and catch (TypeError, ValueError) with a debug note.
Apply:
- start_time = params.get("start_time") - end_time = params.get("end_time") + start_time = params.get("from_time") + end_time = params.get("to_time") @@ - if "query" in params and params.get("query"): - try: + if "query" in params and params.get("query"): + try: (from_time, to_time) = process_timestamps_to_int( start=params.get("from_time"), end=params.get("to_time"), default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, ) datadog_url = generate_datadog_metrics_url( self.toolset.dd_config, params.get("query", ""), from_time, to_time, ) error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + except (TypeError, ValueError) as ex: + logging.debug("Skipping URL generation: %s", ex)
329-352: NO_DATA message uses start_time/end_time (not supported here).Switch to from_time/to_time to reflect actual inputs.
Apply:
- start_time = params.get("start_time") - end_time = params.get("end_time") + start_time = params.get("from_time") + end_time = params.get("to_time")holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
160-208: Fix app-domain replacement, avoid shadowing builtins, and keep epoch-ms in deep links.
- Use ://api. → ://app. (current replace misses common hosts).
- Rename local variable filter → filter_str.
- Keep from_ts/to_ts in ms.
Apply:
def generate_datadog_logs_url( @@ - """Generate a Datadog web UI URL for the logs query.""" - from holmes.plugins.toolsets.utils import process_timestamps_to_int + """Generate a Datadog web UI URL for the logs query.""" @@ - # Convert https://api.datadoghq.com to https://app.datadoghq.com - # or https://api.datadoghq.eu to https://app.datadoghq.eu - base_url = str(dd_config.site_api_url).replace("/api.", "/app.") + # Convert e.g. https://api.datadoghq.com -> https://app.datadoghq.com + base_url = str(dd_config.site_api_url).replace("://api.", "://app.") @@ - if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + if params.filter: + filter_str = params.filter.replace('"', '\\"') + query += f' "{filter_str}"' @@ - (from_time_seconds, to_time_seconds) = process_timestamps_to_int( + (from_time_seconds, to_time_seconds) = process_timestamps_to_int( start=params.start_time, end=params.end_time, default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, )
🧹 Nitpick comments (14)
conftest.py (1)
171-178: Add passthroughs for regional Datadog app hosts (and optionally GitHub raw).Tests may generate links like app.us3/app.us5/app.ap1.datadoghq.com; allow these to avoid accidental blocking if anything tries to resolve metadata. Optionally allow raw.githubusercontent.com for OpenAPI fetches.
Apply:
rsps.add_passthru("https://api.us3.datadoghq.com") rsps.add_passthru("https://api.us5.datadoghq.com") rsps.add_passthru("https://api.ap1.datadoghq.com") rsps.add_passthru("https://app.datadoghq.com") rsps.add_passthru("https://app.datadoghq.eu") + rsps.add_passthru("https://app.us3.datadoghq.com") + rsps.add_passthru("https://app.us5.datadoghq.com") + rsps.add_passthru("https://app.ap1.datadoghq.com") + # Needed if tests end up fetching Datadog OpenAPI specs + rsps.add_passthru("https://raw.githubusercontent.com")holmes/plugins/toolsets/datadog/datadog_api.py (3)
336-346: Endpoint matching is brittle; use regexes derived from path templates.The current brace-removal substring check can mis-match endpoints. Compile regex from templates like “/foo/{id}”.
Apply:
@@ - if endpoint not in paths: - # Try to find a matching pattern (e.g., /api/v2/logs/events/search) - for path_pattern in paths.keys(): - if ( - path_pattern == endpoint - or path_pattern.replace("{", "").replace("}", "") in endpoint - ): - endpoint = path_pattern - break - else: - return None + if endpoint not in paths: + best_match = None + for path_pattern in paths.keys(): + # Convert OpenAPI template to regex, e.g., /foo/{id} -> ^/foo/[^/]+$ + pat = "^" + re.sub(r"\{[^}]+\}", r"[^/]+", re.escape(path_pattern)).replace(r"\*", ".*") + "$" + if re.match(pat, endpoint): + best_match = path_pattern + break + if not best_match: + return None + endpoint = best_match
291-297: Prefer logging.exception over broad except; narrow exception types.Catching bare Exception and logging.error hides stack traces and can mask issues.
Apply:
- except Exception as e: - logging.error(f"Failed to fetch spec for {ver}: {e}") + except requests.RequestException: + logging.exception(f"Failed to fetch spec for {ver}") @@ - except Exception as e: - logging.error(f"Error fetching OpenAPI spec: {e}") + except Exception: + logging.exception("Error fetching OpenAPI spec")Also applies to: 309-312
461-469: Clean up unused parameters to satisfy Ruff ARG001 (or use them).endpoint (preprocess_time_fields) and site_api_url (enhance_error_message) are unused. Either remove them or use endpoint to decide RFC3339 vs Unix conversions.
Would you like me to wire endpoint awareness so v1 fields are coerced to Unix seconds and v2 to RFC3339 during preprocessing?
Also applies to: 523-526
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
661-697: ListMetricTags error context includes irrelevant query/time info.This tool only takes metric_name. Trim noise.
Apply:
- # Include full API error details for better debugging - error_msg = ( - f"Datadog API error (status {e.status_code}): {e.response_text}" - ) - if params: - error_msg += f"\nQuery: {params.get('query', 'not specified')}" - - start_time = params.get("start_time") - end_time = params.get("end_time") - - if start_time: - start_desc = start_time - else: - start_desc = ( - f"default (last {DEFAULT_TIME_SPAN_SECONDS // 86400} days)" - ) - - end_desc = end_time or "now" - error_msg += f"\nTime range: {start_desc} to {end_desc}" - - # Add Datadog web UI URL even for errors (if we have time info) - if "query" in params and params.get("query"): - try: - (from_time, to_time) = process_timestamps_to_int( - start=params.get("from_time"), - end=params.get("to_time"), - default_time_span_seconds=DEFAULT_TIME_SPAN_SECONDS, - ) - datadog_url = generate_datadog_metrics_url( - self.toolset.dd_config, - params.get("query", ""), - from_time, - to_time, - ) - error_msg += f"\nView in Datadog: {datadog_url}" - except Exception: - pass # Skip URL generation if timestamps can't be processed + error_msg = ( + f"Datadog API error (status {e.status_code}): {e.response_text}" + ) + if params: + metric_name = params.get("metric_name", "<unknown>") + error_msg += f"\nMetric: {metric_name}"holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (2)
252-263: Expose Datadog link via StructuredToolResult.url (keep appended link for now).Set url to the generated link while keeping the appended line to avoid test churn.
Apply:
- logs_with_link = f"{logs_str}\n\nView in Datadog: {datadog_url}" return StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, - data=logs_with_link, + data=f"{logs_str}\n\nView in Datadog: {datadog_url}", + url=datadog_url, params=params.model_dump(), )
303-311: Minor: Render index list more readably.Join the list to avoid printing Python list syntax.
Apply:
- f"Indexes searched: {diagnostics['indexes_searched']}\n" + f"Indexes searched: {', '.join(diagnostics['indexes_searched'])}\n"holmes/plugins/toolsets/datadog/toolset_datadog_general.py (7)
242-253: Don’t block startup on OpenAPI fetch; make it opt‑in or lazyFetching large specs on init can add latency or fail in air‑gapped runs. Gate it behind a config flag and fall back to lazy fetch upon first 400/error.
Apply:
class DatadogGeneralConfig(DatadogBaseConfig): """Configuration for general-purpose Datadog toolset.""" max_response_size: int = MAX_RESPONSE_SIZE allow_custom_endpoints: bool = ( False # If True, allows endpoints not in whitelist (still filtered for safety) ) + load_openapi_on_startup: bool = True @@ - # Fetch OpenAPI spec on startup for better error messages and documentation - logging.debug("Fetching Datadog OpenAPI specification...") - self.openapi_spec = fetch_openapi_spec(version="both") - if self.openapi_spec: - logging.info( - f"Successfully loaded OpenAPI spec with {len(self.openapi_spec.get('paths', {}))} endpoints" - ) - else: - logging.warning( - "Could not fetch OpenAPI spec; enhanced error messages will be limited" - ) + # Optionally fetch OpenAPI spec on startup for better error messages and documentation + if dd_config.load_openapi_on_startup: + logging.debug("Fetching Datadog OpenAPI specification...") + self.openapi_spec = fetch_openapi_spec(version="both") + if self.openapi_spec: + logging.info( + f"Successfully loaded OpenAPI spec with {len(self.openapi_spec.get('paths', {}))} endpoints" + ) + else: + logging.warning( + "Could not fetch OpenAPI spec; enhanced error messages will be limited" + )
449-457: Record the processed params and URL in resultsYou call the API with processed_params but surface the original query in the error payload and omit the URL on success. Include the actual invocation for debuggability and compliance with detailed‑error guidelines.
Apply:
# Preprocess time fields if any processed_params = preprocess_time_fields(query_params, endpoint) @@ return StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, data=response_str, - params=params, + params=params, + url=url, ) @@ return StructuredToolResult( status=StructuredToolResultStatus.ERROR, error=error_msg, params=params, - invocation=json.dumps({"url": url, "params": query_params}) + invocation=json.dumps({"url": url, "params": processed_params}) if url else None, )Also applies to: 498-505, 473-477
615-617: Same: record processed body and URL for POSTMirror the GET fix for POST searches.
Apply:
# Preprocess time fields if any processed_body = preprocess_time_fields(body, endpoint) @@ return StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, data=response_str, - params=params, + params=params, + url=url, ) @@ return StructuredToolResult( status=StructuredToolResultStatus.ERROR, error=error_msg, params=params, - invocation=json.dumps({"url": url, "body": body}) if url else None, + invocation=json.dumps({"url": url, "body": processed_body}) if url else None, )Also applies to: 668-669, 639-643
745-751: Fix method inference: ‘GET/POST’ branch is unreachableBecause "search" is a substring of patterns with "(/search)?$", the first if captures it and you never hit the GET/POST branch.
Apply:
- if "search" in pattern or "query" in pattern or "aggregate" in pattern: - methods = "POST" - elif "/search)?$" in pattern: - methods = "GET/POST" + if "/search)?$" in pattern: + methods = "GET/POST" + elif ("search" in pattern) or ("query" in pattern) or ("aggregate" in pattern): + methods = "POST" else: methods = "GET"
793-841: Show only allowed methods’ OpenAPI detailsListing PUT/DELETE schemas can mislead users into thinking writes are supported here. Limit to GET/POST based on the inferred methods.
Apply:
- # Add actual OpenAPI schema for the endpoint - for method in ["get", "post", "put", "delete"]: + # Add actual OpenAPI schema only for allowed methods + allowed = [] + if info["methods"] == "GET": + allowed = ["get"] + elif info["methods"] == "POST": + allowed = ["post"] + else: + allowed = ["get", "post"] + for method in allowed: if method in path_spec: method_spec = path_spec[method] line += f"\n\n OpenAPI Schema ({method.upper()}):"
760-766: Consider NO_DATA for zero matchesReturning NO_DATA improves UX consistency and coloring for “nothing matched” cases.
Apply:
- return StructuredToolResult( - status=StructuredToolResultStatus.SUCCESS, + return StructuredToolResult( + status=StructuredToolResultStatus.NO_DATA, data=f"No endpoints found matching regex: {search_regex}", params=params, )
703-703: Silence ARG002: ‘user_approved’ is unusedKeep the signature (callers may pass by name) but reference the arg to satisfy Ruff.
Apply:
def _invoke( self, params: dict, user_approved: bool = False ) -> StructuredToolResult: """Execute the GET request.""" + _ = user_approved # silence unused-arg (ruff ARG002)def _invoke( self, params: dict, user_approved: bool = False ) -> StructuredToolResult: """Execute the POST search request.""" + _ = user_approved # silence unused-arg (ruff ARG002)def _invoke( self, params: dict, user_approved: bool = False ) -> StructuredToolResult: """List available API resources.""" + _ = user_approved # silence unused-arg (ruff ARG002)Also applies to: 402-405, 570-573
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
conftest.py(1 hunks)holmes/config.py(1 hunks)holmes/plugins/prompts/_fetch_logs.jinja2(2 hunks)holmes/plugins/toolsets/datadog/datadog_api.py(3 hunks)holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_general.py(15 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(8 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/config.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
conftest.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
holmes/plugins/prompts/**/*.jinja2
📄 CodeRabbit inference engine (CLAUDE.md)
Prompts must be stored as .jinja2 templates under holmes/plugins/prompts/{name}.jinja2
Files:
holmes/plugins/prompts/_fetch_logs.jinja2
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2holmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
🧠 Learnings (2)
📚 Learning: 2025-05-15T05:13:43.169Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Applied to files:
holmes/plugins/prompts/_fetch_logs.jinja2
📚 Learning: 2025-05-15T05:14:06.519Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#408
File: holmes/plugins/toolsets/kubernetes_logs.py:100-102
Timestamp: 2025-05-15T05:14:06.519Z
Learning: The `fetch_logs` method in KubernetesLogsToolset is designed to apply the limit parameter after filtering and combining both current and previous logs, rather than using the API's tail_lines parameter, to ensure the limit applies to the final combined log set.
Applied to files:
holmes/plugins/prompts/_fetch_logs.jinja2
🧬 Code graph analysis (3)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (4)
holmes/plugins/toolsets/datadog/datadog_api.py (3)
enhance_error_message(523-616)preprocess_time_fields(461-520)execute_paginated_datadog_http_request(138-154)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
FetchPodLogsParams(42-57)holmes/plugins/toolsets/utils.py (2)
process_timestamps_to_int(90-136)process_timestamps_to_rfc3339(75-87)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (3)
holmes/plugins/toolsets/datadog/datadog_api.py (4)
preprocess_time_fields(461-520)enhance_error_message(523-616)fetch_openapi_spec(209-311)execute_datadog_http_request(170-206)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)holmes/core/tools.py (3)
ToolParameter(154-159)StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_int(90-136)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/datadog/datadog_api.py
210-210: Unused function argument: site_api_url
(ARG001)
294-294: Do not catch blind exception: Exception
(BLE001)
295-295: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
309-309: Do not catch blind exception: Exception
(BLE001)
310-310: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
461-461: Unused function argument: endpoint
(ARG001)
524-524: Unused function argument: site_api_url
(ARG001)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py
703-703: Unused method argument: user_approved
(ARG002)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
217-218: try-except-pass detected, consider logging the exception
(S110)
217-217: Do not catch blind exception: Exception
(BLE001)
456-457: try-except-pass detected, consider logging the exception
(S110)
456-456: Do not catch blind exception: Exception
(BLE001)
696-697: try-except-pass detected, consider logging the exception
(S110)
696-696: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (7)
holmes/plugins/prompts/_fetch_logs.jinja2 (2)
14-14: Good: mandate stating the date range in logs answers.This aligns answers with the actual window used and avoids ambiguity.
38-47: Datadog logs guidance reads well and is actionable.Clear directives on time handling, reporting counts, and filters. Matches recent tool behavior.
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
171-173: [Note] Pagination payload mutability is fine here.Reusing processed_payload across pages is OK as you’re only updating page.cursor/limit.
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (4)
35-140: Whitelist tuples + per-endpoint hints: solid improvementThe move to (pattern, hint) improves UX and keeps safety intact. Entries look consistent and conservative.
264-266: Good: normalized base URL avoids double slashesSmall but important correctness/safety tweak for URL building.
326-329: GET allow-list matcher updated for (pattern, hint) shapePattern-only matching while ignoring hints is correct.
340-355: Endpoint hint helper is usefulCompact helper will let callers surface guidance consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
462-463: Avoid bare except; log why URL generation was skipped.Ruff BLE001/S110. Limit to expected errors and keep diagnostics.
- except Exception: - pass # Skip URL generation if timestamps can't be processed + except (TypeError, ValueError) as ex: + logging.debug("Skipping Datadog URL generation: %s", ex)
434-446: Fix wrong param names in error message (uses start_time/end_time).Same bug as above in error path.
- start_time = params.get("start_time") - end_time = params.get("end_time") + start_time = params.get("from_time") + end_time = params.get("to_time") @@ - error_msg += f"\nTime range: {start_desc} to {end_desc}" + error_msg += f"\nTime range: {start_desc} to {end_desc}"holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
251-260: Don’t append the Datadog link into the logs payload; use StructuredToolResult.url.Keeps data clean and puts navigational links where the UI expects them.
- logs_with_link = f"{logs_str}\n\nView in Datadog: {datadog_url}" return StructuredToolResult( status=StructuredToolResultStatus.SUCCESS, - data=logs_with_link, + data=logs_str, url=datadog_url, params=params.model_dump(), )
🧹 Nitpick comments (12)
holmes/plugins/toolsets/datadog/datadog_api.py (5)
399-411: Endpoint matching is brittle; use regex from path templates.The current brace-stripping substring check can false‑match and pick the wrong spec path.
Replace with template-aware matching:
@@ - if endpoint not in paths: - # Try to find a matching pattern (e.g., /api/v2/logs/events/search) - for path_pattern in paths.keys(): - if ( - path_pattern == endpoint - or path_pattern.replace("{", "").replace("}", "") in endpoint - ): - endpoint = path_pattern - break - else: - return None + if endpoint not in paths: + import re as _re + def _template_to_regex(p: str) -> str: + # /api/v2/metrics/{name} -> ^/api/v2/metrics/[^/]+$ + esc = _re.escape + return "^" + _re.sub(r"\\{[^/]+\\}", r"[^/]+", esc(p)) + "$" + candidates = [ + (tpl, _re.compile(_template_to_regex(tpl))) + for tpl in paths.keys() + ] + for tpl, rx in candidates: + if rx.match(endpoint): + endpoint = tpl + break + else: + return None
221-232: Harden header redaction; don’t drop logs on non-str types.Current logic may throw and return {} if v isn’t str. Also only looks for “key”.
Apply:
def sanitize_headers(headers: Union[dict, CaseInsensitiveDict]) -> dict: - try: - return { - k: v - if ("key" not in k.lower() and "key" not in v.lower()) - else "[REDACTED]" - for k, v in headers.items() - } - except (AttributeError, TypeError): - # Return empty dict for mock objects or other non-dict types - return {} + try: + def _lower(x: Any) -> str: + try: + return str(x).lower() + except Exception: + return "" + SENSITIVE = ("key", "secret", "token", "authorization", "password") + redacted = {} + for k, v in headers.items(): + k_str, v_str = str(k), str(v) + kv_lower = _lower(k_str) + " " + _lower(v_str) + redacted[k_str] = "[REDACTED]" if any(s in kv_lower for s in SENSITIVE) else v_str + return redacted + except Exception: + return {}
291-296: Move imports to module top per project rules.yaml/copy are imported inside functions; move to the top import block.
@@ -import yaml +# moved to module imports @@ - # Deep copy to avoid modifying original - import copy + # Deep copy to avoid modifying originalAdd at the top with other imports:
+import copy +import yamlAlso applies to: 535-539
358-375: Narrow exceptions and use logging.exception for traceback.Catching bare Exception hides actionable errors; Ruff flags BLE001/TRY400.
- except Exception as e: - logging.error(f"Failed to fetch spec for {ver}: {e}") + except (requests.RequestException, yaml.YAMLError) as e: + logging.exception("Failed to fetch spec for %s", ver) if version != "both": return None @@ - except Exception as e: - logging.error(f"Error fetching OpenAPI spec: {e}") + except Exception: + logging.exception("Error fetching OpenAPI spec") return None
274-276: Silence ARG001 by marking intentionally unused parameters.These args are kept for compatibility but unused.
def fetch_openapi_spec( - site_api_url: Optional[str] = None, version: str = "both" + site_api_url: Optional[str] = None, version: str = "both" ) -> Optional[Dict[str, Any]]: @@ - global _openapi_spec_cache + global _openapi_spec_cache + _ = site_api_url # kept for backward-compat @@ def preprocess_time_fields(payload: Dict[str, Any], endpoint: str) -> Dict[str, Any]: @@ - # Deep copy to avoid modifying original + _ = endpoint # reserved for future endpoint-specific rules + # Deep copy to avoid modifying original @@ def enhance_error_message( error: DataDogRequestError, endpoint: str, method: str, site_api_url: str ) -> str: @@ - base_msg = f"HTTP error: {error.status_code} - {error.response_text}" + _ = site_api_url # reserved for future use + base_msg = f"HTTP error: {error.status_code} - {error.response_text}"Also applies to: 535-537, 588-596
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
46-54: Move import out of function; reuse shared helper via top-level import.Local import violates the repo’s import rule.
-from holmes.plugins.toolsets.datadog.datadog_api import convert_api_url_to_app_urlAdd to the existing datadog_api imports at the top:
from holmes.plugins.toolsets.datadog.datadog_api import ( DatadogBaseConfig, DataDogRequestError, execute_datadog_http_request, get_headers, MAX_RETRY_COUNT_ON_RATE_LIMIT, + convert_api_url_to_app_url, )
172-172: Use logging.exception without passing the exception object.Passing the exception object is redundant; TRY401.
- logging.exception(e, exc_info=True) + logging.exception("Datadog API error", exc_info=True) @@ - logging.exception(e, exc_info=True) + logging.exception("Datadog API error", exc_info=True) @@ - logging.exception( + logging.exception( f"Failed to query Datadog metrics for params: {params}", exc_info=True )Also applies to: 417-417, 475-477
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (5)
166-168: Move imports to module top; avoid in-function imports.process_timestamps_to_int and convert_api_url_to_app_url are imported inside the function.
@@ -from holmes.plugins.toolsets.utils import process_timestamps_to_rfc3339 +from holmes.plugins.toolsets.utils import process_timestamps_to_rfc3339, process_timestamps_to_int @@ from holmes.plugins.toolsets.datadog.datadog_api import ( DatadogBaseConfig, DataDogRequestError, execute_paginated_datadog_http_request, get_headers, MAX_RETRY_COUNT_ON_RATE_LIMIT, enhance_error_message, preprocess_time_fields, + convert_api_url_to_app_url, ) @@ def generate_datadog_logs_url(...): - from holmes.plugins.toolsets.utils import process_timestamps_to_int - from holmes.plugins.toolsets.datadog.datadog_api import convert_api_url_to_app_urlAlso applies to: 169-171, 31-32, 14-22
90-92: Avoid shadowing built-in name ‘filter’.Rename local variable to filter_str.
- if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + if params.filter: + filter_str = params.filter.replace('"', '\\"') + query += f' "{filter_str}"' @@ - if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + if params.filter: + filter_str = params.filter.replace('"', '\\"') + query += f' "{filter_str}"'Also applies to: 175-178
320-329: Avoid bare except; use logging.exception correctly.Limit exception types for URL generation and log the reason.
- try: + try: datadog_url = generate_datadog_logs_url( self.dd_config, params, self.dd_config.storage_tiers[0] ) - except Exception: - datadog_url = None + except (ValueError, TypeError) as ex: + logging.debug("Skipping Datadog URL generation: %s", ex) + datadog_url = NoneAdditionally, remove redundant exception object in logging:
- logging.exception(e, exc_info=True) + logging.exception("Datadog logs request failed", exc_info=True)
51-53: Avoid mutable default for Pydantic field.Use default_factory to prevent shared mutable list across instances.
- storage_tiers: list[DataDogStorageTier] = Field( - default=DEFAULT_STORAGE_TIERS, min_length=1 - ) + storage_tiers: list[DataDogStorageTier] = Field( + default_factory=lambda: DEFAULT_STORAGE_TIERS.copy(), min_length=1 + )
399-419: Healthcheck log level alignment.Nit: You switched to debug earlier; here success still logs at info. Consider debug for symmetry.
No code change required if intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/plugins/toolsets/datadog/datadog_api.py(3 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(7 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(16 hunks)
🧰 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
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (3)
holmes/plugins/toolsets/datadog/datadog_api.py (1)
convert_api_url_to_app_url(29-89)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)holmes/plugins/toolsets/utils.py (1)
process_timestamps_to_int(90-136)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (4)
holmes/plugins/toolsets/datadog/datadog_api.py (5)
enhance_error_message(587-680)preprocess_time_fields(525-584)execute_paginated_datadog_http_request(202-218)convert_api_url_to_app_url(29-89)DataDogRequestError(101-118)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
FetchPodLogsParams(42-57)holmes/plugins/toolsets/utils.py (2)
process_timestamps_to_int(90-136)process_timestamps_to_rfc3339(75-87)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
462-463: try-except-pass detected, consider logging the exception
(S110)
462-462: Do not catch blind exception: Exception
(BLE001)
holmes/plugins/toolsets/datadog/datadog_api.py
274-274: Unused function argument: site_api_url
(ARG001)
358-358: Do not catch blind exception: Exception
(BLE001)
359-359: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
373-373: Do not catch blind exception: Exception
(BLE001)
374-374: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
525-525: Unused function argument: endpoint
(ARG001)
588-588: Unused function argument: site_api_url
(ARG001)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
320-320: Redundant exception object included in logging.exception call
(TRY401)
327-327: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
121-128: Return NO_DATA with context instead of ERROR when no metrics found.Per coding guidelines, “no data” must not be an error and should state what was searched. Suggest:
- if not metrics: - return StructuredToolResult( - status=StructuredToolResultStatus.ERROR, - data="Your filter returned no metrics. Change your filter and try again", - params=params, - ) + if not metrics: + host = params.get("host") or "<any>" + tag_filter = params.get("tag_filter") or "<none>" + from_desc = params.get("from_time") or f"default (last {ACTIVE_METRICS_DEFAULT_LOOK_BACK_HOURS} hours)" + no_data_msg = ( + "The query returned no metrics.\n" + f"Filters: host={host}, tag_filter={tag_filter}\n" + f"Time range: {from_desc} to now" + ) + return StructuredToolResult( + status=StructuredToolResultStatus.NO_DATA, + error=no_data_msg, + params=params, + )
78-81: Tighten type hints for mypy compliance.Prefer
Dict[str, Any]and annotateparamsin one‑liner helpers.- def _invoke( - self, params: dict, user_approved: bool = False + def _invoke( + self, params: Dict[str, Any], user_approved: bool = False ) -> StructuredToolResult: @@ - def _invoke( - self, params: dict, user_approved: bool = False + def _invoke( + self, params: Dict[str, Any], user_approved: bool = False ) -> StructuredToolResult: @@ - def get_parameterized_one_liner(self, params) -> str: + def get_parameterized_one_liner(self, params: Dict[str, Any]) -> str: @@ - def _invoke( - self, params: dict, user_approved: bool = False + def _invoke( + self, params: Dict[str, Any], user_approved: bool = False ) -> StructuredToolResult: @@ - def _invoke( - self, params: dict, user_approved: bool = False + def _invoke( + self, params: Dict[str, Any], user_approved: bool = False ) -> StructuredToolResult: @@ - def get_parameterized_one_liner(self, params) -> str: + def get_parameterized_one_liner(self, params: Dict[str, Any]) -> str:Also applies to: 240-242, 416-418, 436-438, 552-554, 527-535
♻️ Duplicate comments (2)
holmes/plugins/toolsets/datadog/datadog_api.py (2)
399-412: Endpoint matching is brittle; use template-to-regex matching.Replacing braces and substring checks can mis-match. Build regex from path templates like /foo/{id}.
- if endpoint not in paths: - # Try to find a matching pattern (e.g., /api/v2/logs/events/search) - for path_pattern in paths.keys(): - if ( - path_pattern == endpoint - or path_pattern.replace("{", "").replace("}", "") in endpoint - ): - endpoint = path_pattern - break - else: - return None + if endpoint not in paths: + def _template_to_regex(tpl: str) -> re.Pattern[str]: + # Escape literals, replace {param} with [^/]+ + parts = [] + i = 0 + while i < len(tpl): + if tpl[i] == "{": + j = tpl.find("}", i + 1) + if j == -1: + parts.append(re.escape(tpl[i:])) + break + parts.append(r"[^/]+") + i = j + 1 + else: + parts.append(re.escape(tpl[i])) + i += 1 + return re.compile("^" + "".join(parts) + "$") + + for path_pattern in paths.keys(): + if _template_to_regex(path_pattern).match(endpoint): + endpoint = path_pattern + break + else: + return None
25-27: Fix relative-time parsing: support now-, ms, and consistent +/- handling.Current regex/logic rejects inputs like now-1h and lacks ms. It can misinterpret signs. Update pattern and converter as below.
-# Relative time pattern (m = minutes, mo = months) -RELATIVE_TIME_PATTERN = re.compile(r"^-?(\d+)([hdwsy]|min|m|mo)$|^now$", re.IGNORECASE) +# Relative time pattern (supports: now, now-<n><unit>, and bare ±<n><unit>) +RELATIVE_TIME_PATTERN = re.compile( + r"^(?:now(?:-(\d+)(ms|s|min|m|h|d|w|mo|y))?|([+-]?\d+)(ms|s|min|m|h|d|w|mo|y)|now)$", + re.IGNORECASE, +) @@ -def convert_relative_time(time_str: str) -> Tuple[str, str]: +def convert_relative_time(time_str: str) -> Tuple[str, str]: @@ - # Check for relative time - match = RELATIVE_TIME_PATTERN.match(time_str.strip()) + # Check for relative time + t = time_str.strip() + match = RELATIVE_TIME_PATTERN.match(t) if not match: # Return as-is if not recognized return time_str, "unknown" @@ - if time_str.lower() == "now": + if t.lower() == "now": return now.isoformat().replace("+00:00", "Z"), "relative" @@ - # Parse relative time - groups = match.groups() - if groups[0] is None: - return time_str, "unknown" - - amount = int(groups[0]) - unit = groups[1].lower() + # Parse relative time + now_amount, now_unit, bare_amount, bare_unit = match.groups() + if now_amount and now_unit: + amount = int(now_amount) + unit = now_unit.lower() + negate = True # now-<offset> subtracts + elif bare_amount and bare_unit: + amount = int(bare_amount) + unit = bare_unit.lower() + negate = str(bare_amount).startswith("-") + amount = abs(amount) + else: + return time_str, "unknown" @@ - if unit == "s": + if unit == "ms": + delta = timedelta(milliseconds=amount) + elif unit == "s": delta = timedelta(seconds=amount) - elif unit == "min": + elif unit in ("m", "min"): delta = timedelta(minutes=amount) - elif unit == "m": - delta = timedelta(minutes=amount) # m = minutes elif unit == "h": delta = timedelta(hours=amount) @@ - # Apply delta (subtract if negative relative time) - if time_str.startswith("-"): - result_time = now - delta - else: - result_time = now + delta + # Apply delta + result_time = now - delta if negate else now + deltaAlso applies to: 451-525
🧹 Nitpick comments (13)
tests/llm/utils/property_manager.py (2)
131-144: Make tool-call rendering backward-compatible and robust
- Don’t drop valid falsy results (e.g., 0, ""), use an
is not Nonecheck.- Fallback to
tc.outputwhenresultisn’t present to keep older tools working.- Guard missing
descriptionto avoid attribute errors.for i, tc in enumerate(result.tool_calls, 1): - tool_calls_text += f"* Tool #{i}: {tc.description}\n" - if hasattr(tc, "result") and tc.result: + desc = getattr(tc, "description", getattr(tc, "name", type(tc).__name__)) + tool_calls_text += f"* Tool #{i}: {desc}\n" + output_val = tc.result if hasattr(tc, "result") else getattr(tc, "output", None) + if output_val is not None: # Don't truncate - LLM judge needs complete output for accurate evaluation - output_text = str(tc.result) + output_text = str(output_val) # Indent the output for readability indented_output = "\n".join( f" {line}" for line in output_text.split("\n") ) tool_calls_text += f"Output:\n{indented_output}\n" tool_calls_text += "---\n"
1-2: Annotate pytest request for mypy compliancePer repo guidelines (“All Python code must include type hints”), explicitly type the pytest request argument. This avoids implicit Any and keeps CI happier.
-from typing import List, Any, Union, Optional, Dict +from typing import List, Any, Union, Optional, Dict +import pytest -def set_initial_properties(request, test_case: HolmesTestCase, model: str) -> None: +def set_initial_properties(request: pytest.FixtureRequest, test_case: HolmesTestCase, model: str) -> None: -def set_trace_properties(request, eval_span) -> None: +def set_trace_properties(request: pytest.FixtureRequest, eval_span: Any) -> None: -def update_property(request, key: str, value: Any) -> None: +def update_property(request: pytest.FixtureRequest, key: str, value: Any) -> None: -def update_test_results( - request, +def update_test_results( + request: pytest.FixtureRequest, -def handle_test_error( - request, +def handle_test_error( + request: pytest.FixtureRequest,Also applies to: 5-6, 53-54, 68-70, 78-88, 207-216
tests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.py (1)
235-235: LGTM on timestamp prefix; add coverage for fallbacks.Consider adding cases for attributes["@timestamp"] fallback and for missing timestamp (message-only output) to lock the new behavior.
tests/plugins/toolsets/datadog/test_toolset_datadog_general.py (1)
130-135: Mismatch: listing shows POST /api/v1/monitor but POST is blocked elsewhere.Your tests block POST /api/v1/monitor (creation) but here assert it in the listing. Either the listing should show GET for /api/v1/monitor and POST for /api/v1/monitor/search, or loosen the assertion. Suggest adjusting assertions:
- result = list_tool._invoke({}) + result = list_tool._invoke({}) assert result.status == StructuredToolResultStatus.SUCCESS assert "monitor" in result.data.lower() - assert "dashboard" in result.data.lower() - assert "POST /api/v1/monitor" in result.data + assert "dashboard" in result.data.lower() + # Read-only list should show GET for the resource: + assert "GET /api/v1/monitor" in result.data + # And POST specifically for the search endpoint: + assert "POST /api/v1/monitor/search" in result.dataholmes/plugins/toolsets/datadog/datadog_api.py (4)
273-376: Move import to top, narrow exceptions, and reduce external timeout.
- Import yaml at module top per project rules.
- Use requests.RequestException/yaml.YAMLError; log with logging.exception.
- 30s timeout is high for request threads; prefer <=5s.
- try: - import yaml + try: @@ - response = requests.get(spec_urls[ver], timeout=30) + response = requests.get(spec_urls[ver], timeout=5) @@ - except Exception as e: - logging.error(f"Failed to fetch spec for {ver}: {e}") + except requests.RequestException as e: + logging.exception(f"Failed to fetch spec for {ver}: {e}") if version != "both": return None @@ - except Exception as e: - logging.error(f"Error fetching OpenAPI spec: {e}") + except yaml.YAMLError as e: + logging.exception(f"Error parsing OpenAPI spec YAML: {e}") + return None + except Exception as e: + logging.exception(f"Error fetching OpenAPI spec: {e}") return NoneAdd these to the top-level imports (outside this hunk):
import yaml # at module top
221-232: Sanitize headers without dropping all data on non-str values.The except branch returns {}, losing useful context. Coerce to str and redact keys safely.
-def sanitize_headers(headers: Union[dict, CaseInsensitiveDict]) -> dict: - try: - return { - k: v - if ("key" not in k.lower() and "key" not in v.lower()) - else "[REDACTED]" - for k, v in headers.items() - } - except (AttributeError, TypeError): - # Return empty dict for mock objects or other non-dict types - return {} +def sanitize_headers(headers: Union[dict, CaseInsensitiveDict]) -> dict: + try: + out: dict = {} + for k, v in headers.items(): + ks = str(k).lower() + vs = str(v) if not isinstance(v, str) else v + out[k] = "[REDACTED]" if ("key" in ks or "key" in vs.lower()) else vs + return out + except Exception: + return {}
527-541: Move import to module top; silence unused arg.
- Import copy at module top.
- endpoint is unused; mark explicitly to satisfy ARG001.
- # Deep copy to avoid modifying original - import copy - - processed = copy.deepcopy(payload) + # Deep copy to avoid modifying original + processed = copy.deepcopy(payload) + # not currently used; keep in signature for future endpoint-specific logic + del endpointAdd this to the top-level imports:
import copy # at module top
589-682: Align 400-help text with new relative-time support; include request context.Message says relative times are NOT supported, but preprocess_time_fields converts them. Also use site_api_url or endpoint to add actionable context.
- enhanced_parts.append( - "\nTime format requirements:\n" - " - v1 API: Unix timestamps (e.g., 1704067200)\n" - " - v2 API: RFC3339 format (e.g., '2024-01-01T00:00:00Z')\n" - " - NOT supported: Relative times like '-24h', 'now', '-7d'" - ) + enhanced_parts.append( + "\nTime format requirements:\n" + " - v1 API: Unix seconds (e.g., 1704067200)\n" + " - v2 API: RFC3339 (e.g., 2024-01-01T00:00:00Z)\n" + " - Relative inputs like 'now-24h' are accepted and auto-converted." + ) @@ - return "\n".join(enhanced_parts) + # Add request context + enhanced_parts.append(f"\nRequest: {method} {endpoint}") + try: + enhanced_parts.append( + f"Payload: {json.dumps(error.payload, indent=2)[:1500]}" + + ("..." if len(json.dumps(error.payload)) > 1500 else "") + ) + except Exception: + pass + return "\n".join(enhanced_parts)tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml (1)
1-9: Tighten the expectation to avoid false positives.Consider asserting the app hostname appears (e.g., app.datadoghq or app.datadoghq.eu) rather than a generic “url to datadog”.
expected_output: - - "Result must include a tool call that includes a url to datadog" + - "Result must include a tool call that includes a Datadog app URL (e.g., https://app.datadoghq.*)"holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (4)
152-169: Error context is solid; include response_text for 403/429 too.To meet “full API error response” guidance, append
e.response_textin the 403 and 429 branches as well.- if e.status_code == 429: - error_msg = f"Datadog API rate limit exceeded. Failed after {MAX_RETRY_COUNT_ON_RATE_LIMIT} retry attempts." + if e.status_code == 429: + error_msg = ( + f"Datadog API rate limit exceeded. Failed after {MAX_RETRY_COUNT_ON_RATE_LIMIT} retry attempts. " + f"Details: {e.response_text}" + ) @@ - elif e.status_code == 403: - error_msg = ( - f"Permission denied. Ensure your Datadog Application Key has the 'metrics_read' " - f"and 'timeseries_query' permissions. Error: {str(e)}" - ) + elif e.status_code == 403: + error_msg = ( + "Permission denied. Ensure your Datadog Application Key has the 'metrics_read' " + "and 'timeseries_query' permissions. " + f"Details: {e.response_text}" + )
340-341: Use UTC for RFC3339 timestamps (avoid localtime with trailing Z).
datetime.fromtimestamp()uses local time. Since you appendZ, use UTC:- start_rfc = datetime.fromtimestamp(from_time).strftime("%Y-%m-%dT%H:%M:%SZ") - end_rfc = datetime.fromtimestamp(to_time).strftime("%Y-%m-%dT%H:%M:%SZ") + start_rfc = datetime.utcfromtimestamp(from_time).strftime("%Y-%m-%dT%H:%M:%SZ") + end_rfc = datetime.utcfromtimestamp(to_time).strftime("%Y-%m-%dT%H:%M:%SZ")
376-395: Great: rich API error details for QueryMetrics “else”. Add parity for 403/429.Mirror this level of detail (include
e.response_text) in the 403/429 branches above for consistency and compliance.
598-605: Include invocation even without params for ListMetricTags errors.Currently
invocationis omitted becausequery_paramsisNone. Suggest settingquery_params = {}(or buildinginvocationfromurlonly) so the returned error contains the called endpoint.Example:
# at initialization query_params: Dict[str, Any] = {} ... return StructuredToolResult( ..., invocation=json.dumps({"url": url, "params": query_params}) if url else None, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
holmes/plugins/toolsets/datadog/datadog_api.py(3 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(10 hunks)tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml(1 hunks)tests/llm/utils/property_manager.py(1 hunks)tests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.py(1 hunks)tests/plugins/toolsets/datadog/test_toolset_datadog_general.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/toolsets.yaml
🧰 Additional context used
📓 Path-based instructions (12)
tests/llm/**/toolsets.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
In evals, all toolset-specific configuration must be under a top-level 'config' field in toolsets.yaml; do not place toolset config directly at the top level
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml
{holmes/plugins/toolsets/**/*.yaml,tests/llm/**/toolsets.yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Valid top-level fields for toolset YAMLs are limited to: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml
tests/llm/fixtures/**/*.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/fixtures/**/*.yaml: Each LLM test must use a dedicated Kubernetes namespace 'app-' to prevent conflicts when running in parallel
All pod names in evals must be unique across tests; never reuse pod names
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yamltests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
tests/llm/fixtures/**/*.{yaml,md,log,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Eval artifacts must be realistic and neutral: no obvious/fake logs, no filenames or resource names that hint at the problem, and no messages revealing simulation
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yamltests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
tests/llm/**/*.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Kubernetes Secrets for scripts in evals; do not embed scripts inline in manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yamltests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yamltests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yamltests/llm/utils/property_manager.pytests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
tests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/llm/utils/property_manager.pytests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/plugins/toolsets/datadog/test_toolset_datadog_general.pytests/llm/utils/property_manager.pytests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.py
tests/llm/**/test_case.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Do not include toolset configuration in test_case.yaml; if runbooks are specified, entries must point to .md files in the same directory
Files:
tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
tests/llm/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always run LLM tests with RUN_LIVE=true to ensure results match real-world behavior
Files:
tests/llm/utils/property_manager.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/datadog_api.py
🧠 Learnings (1)
📚 Learning: 2025-09-15T07:09:16.052Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T07:09:16.052Z
Learning: Applies to {holmes/plugins/toolsets/**/*.yaml,tests/llm/**/toolsets.yaml} : Valid top-level fields for toolset YAMLs are limited to: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Applied to files:
tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml
🧬 Code graph analysis (2)
tests/plugins/toolsets/datadog/test_toolset_datadog_general.py (2)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py (3)
_invoke(402-513)_invoke(570-677)_invoke(702-860)holmes/core/tools.py (1)
StructuredToolResultStatus(51-75)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/datadog/datadog_api.py
274-274: Unused function argument: site_api_url
(ARG001)
358-358: Do not catch blind exception: Exception
(BLE001)
359-359: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
373-373: Do not catch blind exception: Exception
(BLE001)
374-374: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
527-527: Unused function argument: endpoint
(ARG001)
590-590: Unused function argument: site_api_url
(ARG001)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (8)
holmes/plugins/toolsets/datadog/datadog_api.py (1)
29-90: App-URL conversion looks correct.Covers api->app subdomain and strips
/apipath cleanly.holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (7)
57-57: Good: clearer ListActiveMetrics description.
207-208: Good: clearer QueryMetrics description.
284-303: Good NO_DATA message with query + time window.This aligns with the toolset error/NO_DATA requirements.
425-426: Good: clearer metadata tool description.
541-542: Good: clearer ListMetricTags description.
654-654: LGTM: downgrade healthcheck log to debug.
683-684: LGTM: clearer prerequisites error with docs URL.
2 participants
No description provided.