fix datadog docs and remove experimental tag - #939
Conversation
WalkthroughRemoved Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (3)
145-149: Capability mismatch with docstring — consider adding substring support flag.
Docstring says “historical data and substring matching,” but only HISTORICAL_DATA is returned. If applicable, include SUBSTRING_MATCH in the set; otherwise, align the docstring.- return { - LoggingCapability.HISTORICAL_DATA - } # No regex support, no exclude filter, but supports historical data + return { + LoggingCapability.HISTORICAL_DATA, + LoggingCapability.SUBSTRING_MATCH, # if available in enum + } # No regex support, no exclude filter; supports historical + substring
86-88: Avoid shadowing built-infilter.
Use a different name to prevent confusion and improve readability.- if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + if params.filter: + filter_term = params.filter.replace('"', '\\"') + query += f' "{filter_term}"'
164-166: Consistent brand casing.
Prefer “Datadog” over “DataDog”.def logger_name(self) -> str: - return "DataDog" + return "Datadog"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py(0 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(0 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_traces.py(0 hunks)
💤 Files with no reviewable changes (3)
- holmes/plugins/toolsets/datadog/toolset_datadog_traces.py
- holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must live under holmes/plugins/toolsets as either {name}.yaml or a {name}/ directory
Files:
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
154-154: Docs URL update — LGTM.
New link looks correct and clearer for users.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
holmes/plugins/toolsets/newrelic.py (5)
82-89: Add a client-side timeout and handle GraphQL 200-with-errors responsesRequests without timeouts can hang; GraphQL may return 200 with an "errors" payload. Handle both.
Apply:
- response = requests.post(url, headers=headers, json=query) # type: ignore[arg-type] + response = requests.post(url, headers=headers, json=query, timeout=15) # type: ignore[arg-type] - if response.status_code == 200: - return success(response.json()) + if response.status_code == 200: + body = response.json() + if isinstance(body, dict) and body.get("errors"): + return error(f"New Relic GraphQL errors: {body['errors']}") + return success(body)
167-174: Same here: add timeout and check for GraphQL errorsMirror the fix in traces.
- response = requests.post(url, headers=headers, json=query) # type: ignore[arg-type] + response = requests.post(url, headers=headers, json=query, timeout=15) # type: ignore[arg-type] - if response.status_code == 200: - return success(response.json()) + if response.status_code == 200: + body = response.json() + if isinstance(body, dict) and body.get("errors"): + return error(f"New Relic GraphQL errors: {body['errors']}") + return success(body)
137-144: Validate and coerce duration; avoid string-math and enforce > 0Prevents bad NRQL and type issues.
- duration = get_param_or_raise(params, "duration") + try: + duration = float(get_param_or_raise(params, "duration")) + except Exception: + return error("Invalid 'duration': must be a number (seconds).") + if duration <= 0: + return error("'duration' must be > 0 seconds.") @@ - if trace_id: - query_string = f"SELECT * FROM Span WHERE trace.id = '{trace_id}' and duration.ms > {duration * 1000} and span.kind != 'internal'" - else: - query_string = f"SELECT * FROM Span WHERE duration.ms > {duration * 1000} and span.kind != 'internal'" + ms = int(duration * 1000) + if trace_id: + query_string = f"SELECT * FROM Span WHERE trace.id = '{trace_id}' and duration.ms > {ms} and span.kind != 'internal'" + else: + query_string = f"SELECT * FROM Span WHERE duration.ms > {ms} and span.kind != 'internal'"
61-72: Harden NRQL string construction (basic injection/breakage guard)User-provided app/since/trace_id are interpolated verbatim; guard with minimal validation.
Add before building each query:
+ # Basic validation to avoid broken NRQL/injection + import re # at file top + if not isinstance(app, str) or not re.fullmatch(r"[\w.\-]+", app): + return error("Invalid 'app' (allowed: letters, digits, _, -, .).") + if not isinstance(since, str) or not re.fullmatch(r"\d+\s+(minutes?|hours?|days?)\s+ago", since): + return error("Invalid 'since' (e.g., '1 hour ago').")For traces (when trace_id is provided):
+ if trace_id and not re.fullmatch(r"[A-Fa-f0-9\-]+", str(trace_id)): + return error("Invalid 'trace_id' format.")Note: If you prefer stricter patterns, we can align with your NR standards.
Also applies to: 146-157
196-209: Replace invalid docs_url in azure_sql_toolset.py
holmes/plugins/toolsets/azure_sql/azure_sql_toolset.py:60 – update the kagi.com link to the official holmesgpt.dev documentation page.
🧹 Nitpick comments (4)
holmes/plugins/toolsets/newrelic.py (4)
207-207: Confirm intent: remove experimental flag here tooPR title mentions removing the experimental tag. This toolset still sets experimental=True. If intended to de-experimentalize, drop the arg.
- experimental=True,If you want consistency across toolsets, I can sweep and patch all remaining occurrences.
230-231: Provide example config keys for better UXHelps users wire credentials correctly.
- return {} + return { + "nr_account_id": "<your_newrelic_account_id>", + "nr_api_key": "<your_newrelic_api_key>" + }
1-1: Optional: drop import-wide type ignore by adding stubsPrefer installing types-requests and removing the import-level ignore to keep type coverage high.
I can open a small PR adding types-requests to dev deps if you want.
82-82: Consider using a Session for connection reuse (nit)If these tools are invoked frequently, a shared requests.Session with keep-alive can help.
I can sketch a minimal session holder on the Toolset if desired.
Also applies to: 167-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
holmes/plugins/toolsets/datadog/toolset_datadog_general.py(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_traces.py(1 hunks)holmes/plugins/toolsets/git.py(1 hunks)holmes/plugins/toolsets/newrelic.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/plugins/toolsets/git.py
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/toolsets/datadog/toolset_datadog_traces.py
- holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/plugins/toolsets/newrelic.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must live under holmes/plugins/toolsets as either {name}.yaml or a {name}/ directory
Files:
holmes/plugins/toolsets/newrelic.py
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/newrelic.py
82-82: Probable use of requests call without timeout
(S113)
167-167: Probable use of requests call without timeout
(S113)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
holmes/plugins/toolsets/newrelic.py (1)
200-200: docs_url update looks goodCorrect link to holmesgpt.dev.
2 participants
No description provided.