DataDog metrics toolset improvement - #758
Conversation
Add tool for fetching metrics tags Small fixes on the dd metrics toolset Support rendering metrics blocks from dd
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DatadogMetricsToolset
participant ListMetricTags
participant DatadogAPI
User->>DatadogMetricsToolset: Call ListMetricTags with metric_name
DatadogMetricsToolset->>ListMetricTags: _invoke(params)
ListMetricTags->>DatadogAPI: GET /api/v2/metrics/{metric_name}/active-configurations
DatadogAPI-->>ListMetricTags: Return tags and aggregations
ListMetricTags-->>DatadogMetricsToolset: StructuredToolResult (tags, aggregations)
DatadogMetricsToolset-->>User: Return tags and aggregations
sequenceDiagram
participant User
participant DatadogMetricsToolset
participant QueryMetrics
participant DatadogAPI
User->>DatadogMetricsToolset: Call QueryMetrics with description and output_type
DatadogMetricsToolset->>QueryMetrics: _invoke(params)
QueryMetrics->>DatadogAPI: Query timeseries data
DatadogAPI-->>QueryMetrics: Return timeseries data
QueryMetrics-->>DatadogMetricsToolset: StructuredToolResult (Prometheus-compatible format, random_key, tool_name, description, output_type)
DatadogMetricsToolset-->>User: Return formatted result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
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: 1
🧹 Nitpick comments (1)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
36-38: Consider collision risk with 4-character random keysWith only 4 alphanumeric characters, there's a ~1% collision probability after ~8,000 keys. If this is used in high-volume scenarios, consider increasing the key length.
def generate_random_key(): - return "".join(random.choices(string.ascii_letters + string.digits, k=4)) + return "".join(random.choices(string.ascii_letters + string.digits, k=6))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2(2 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(8 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus_instructions.jinja2(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: PRs require maintainer approval
holmes/plugins/toolsets/prometheus/prometheus_instructions.jinja2 (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: nherment
PR: #436
File: tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_all_tools/dns_troubleshooting_instructions.md:59-60
Timestamp: 2025-06-05T06:16:37.361Z
Learning: In the holmesgpt project, 4-space indentation for nested lists in markdown files is the preferred style, not the 2-space indentation suggested by default markdownlint rules.
⏰ 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 (4)
holmes/plugins/toolsets/prometheus/prometheus_instructions.jinja2 (1)
28-32: LGTM!The removal of backticks makes the example consistent with the actual format users should use when embedding Prometheus graphs in their responses.
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2 (1)
18-50: LGTM!The new instructions are well-structured and provide clear guidance for:
- Using the new
list_datadog_metric_tagstool effectively- Handling query results with proper embedding format
- Maintaining consistency with Prometheus graph embedding patterns
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
259-308: LGTM! Well-implemented data transformationThe transformation from Datadog to Prometheus format is correctly implemented with:
- Proper timestamp conversion from milliseconds to seconds
- Correct extraction of metric labels from scope
- RFC3339 date formatting
- Appropriate response structure matching Prometheus format
469-553: LGTM! Well-implemented ListMetricTags toolThe new tool is properly implemented with:
- Appropriate use of the v2 API endpoint for active configurations
- Comprehensive error handling with user-friendly messages
- Consistent structure with other tools in the toolset
Add tool for fetching metrics tags Small fixes on the dd metrics toolset Support rendering metrics blocks from dd
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/llm/utils/test_case_utils.py (3)
144-169: Logic correctly handles prompt variants.The implementation properly creates separate test case instances for each prompt variant with unique IDs and preserves the original prompt data. The flow control with
continueis correct.Consider extracting the test case creation logic into a helper method to reduce duplication between the list and single prompt handling.
254-287: Good refactor with minor typing inconsistency.The extraction of MD file parsing into a separate helper function improves modularity. However, the return type annotation uses the newer union syntax (
None | List[...]) while the rest of the file usesUnionfrom typing.For consistency with the rest of the file, consider using:
-def _parse_conversation_history_md_files( - conversation_history_dir, -) -> None | List[Dict[str, str]]: +def _parse_conversation_history_md_files( + conversation_history_dir, +) -> Optional[List[Dict[str, str]]]:
314-360: Remove commented-out code.This large block of commented-out code (47 lines) appears to be the old implementation. Commented-out code should be removed to maintain code cleanliness - version control preserves the history if needed.
-# def load_conversation_history(test_case_folder: Path) -> Optional[list[dict[str, str]]]: -# """ -# Loads conversation history from .md files in a specified folder structure. -# -# The folder structure is expected to be: -# test_case_folder/ -# conversation_history/ -# <index>_<role>.md -# ... -# """ -# conversation_history_dir = test_case_folder / "conversation_history" -# -# if not conversation_history_dir.is_dir(): -# return None -# -# md_files = sorted(list(conversation_history_dir.glob("*.md"))) -# -# # If no .md files are found in the directory, return None. -# if not md_files: -# return None -# -# conversation_history: list[dict[str, str]] = [] -# for md_file_path in md_files: -# # Get the filename without the .md extension (the "stem") -# # e.g., "01_system.md" -> "01_system" -# stem = md_file_path.stem -# -# # The filename pattern is "<index>_<role>.md". -# # The role is the part of the stem after the first underscore. -# # Example: "01_system" -> role is "system" -# # str.split("_", 1) splits the string at the first underscore. -# # It will return a list of two strings if an underscore is present. -# # e.g., "01_system".split("_", 1) -> ["01", "system"] -# try: -# _index_part, role = stem.split("_", 1) -# except ValueError: -# raise ValueError( -# f"Filename '{md_file_path.name}' in '{conversation_history_dir}' " -# f"does not conform to the expected '<index>_<role>.md' pattern." -# ) -# -# content = md_file_path.read_text(encoding="utf-8") -# -# conversation_history.append({"role": role, "content": content}) -# -# return conversation_history -holmes/plugins/toolsets/kubernetes.yaml (1)
202-205: Condense & clarifyllm_instructionstextThe three sentences convey the same restriction and can be reduced to a single, clearer statement. Repetition inflates the prompt size sent to the LLM and risks truncation on large conversations.
- llm_instructions: | - The kubectl_top_pods or kubectl_top_nodes do not return time series data or metrics that can be used for graphs - Do NOT use kubectl_top_pods or kubectl_top_nodes for graph generation - it only shows current snapshot data - kubectl_top_pods or kubectl_top_nodes are for current status checks, not historical graphs + llm_instructions: | + kubectl_top_pods and kubectl_top_nodes provide only current-snapshot metrics; never ask them for historical or graph-ready time-series data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2(1 hunks)holmes/plugins/toolsets/kubernetes.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/conversation_history.json(1 hunks)tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/toolsets.yaml(1 hunks)tests/llm/utils/test_case_utils.py(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/toolsets.yaml
- tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/conversation_history.json
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2
🧰 Additional context used
📓 Path-based instructions (2)
holmes/plugins/toolsets/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
Toolsets must be placed in holmes/plugins/toolsets/{name}.yaml or {name}/
Files:
holmes/plugins/toolsets/kubernetes.yaml
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
tests/llm/utils/test_case_utils.py
🧠 Learnings (5)
📚 Learning: applies to holmes/plugins/toolsets/**/*.yaml : toolsets must be placed in holmes/plugins/toolsets/{n...
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets must be placed in holmes/plugins/toolsets/{name}.yaml or {name}/
Applied to files:
holmes/plugins/toolsets/kubernetes.yaml
📚 Learning: in the kubernetes logs toolset for holmes, both current and previous logs are intentionally fetched ...
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/toolsets/kubernetes.yaml
📚 Learning: in llm-as-judge test cases for holmesgpt, expected outputs should be descriptive rather than prescri...
Learnt from: Sheeproid
PR: robusta-dev/holmesgpt#586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Applied to files:
tests/llm/utils/test_case_utils.py
📚 Learning: applies to holmes/plugins/prompts/**/*.jinja2 : prompts must be placed in holmes/plugins/prompts/{na...
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts must be placed in holmes/plugins/prompts/{name}.jinja2
Applied to files:
tests/llm/utils/test_case_utils.py
📚 Learning: the robusta-dev/holmesgpt codebase has comprehensive existing validation for azure environment varia...
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/llm/utils/test_case_utils.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). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
tests/llm/utils/test_case_utils.py (2)
66-78: LGTM! Well-designed support for prompt variants.The type change from
strtoUnion[str, List[str]]foruser_promptenables flexible test case definitions. The internal fields for variant tracking are appropriately prefixed and typed.
289-311: Excellent enhancement for conversation history loading.The function now flexibly supports both .md file directories and JSON files, with clear documentation and proper error handling. This improves test case flexibility.
holmes/plugins/toolsets/kubernetes.yaml (1)
208-209: Add missing prerequisite forkubectl top pods
kubectl top podswill also fail when the metrics-server or resource-metrics API is missing. To surface this early, include an explicit prerequisite check similar to the one for nodes.prerequisites: - command: "kubectl top nodes" + - command: "kubectl top pods -A"
… into dd-metrics-toolset-enhancements
arikalon1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
holmes/utils/keygen_utils.py (1)
1-6: LGTM! Consider usingsecretsmodule for cryptographic security.The implementation is clean and follows best practices. However, if these random keys are used in security-sensitive contexts, consider using the
secretsmodule instead ofrandomfor cryptographically secure randomness.If cryptographic security is needed, apply this diff:
-import random +import secrets import string def generate_random_key(): - return "".join(random.choices(string.ascii_letters + string.digits, k=4)) + return "".join(secrets.choices(string.ascii_letters + string.digits, k=4))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(8 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(2 hunks)holmes/utils/keygen_utils.py(1 hunks)tests/llm/utils/test_case_utils.py(4 hunks)tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/plugins/toolsets/datadog/metrics/test_datadog_metrics.py
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/plugins/toolsets/prometheus/prometheus.py
- tests/llm/utils/test_case_utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/utils/keygen_utils.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
🧠 Learnings (1)
📚 Learning: in the newrelic toolset's gettraces.get_parameterized_one_liner() method, the return strings only us...
Learnt from: nherment
PR: robusta-dev/holmesgpt#776
File: holmes/plugins/toolsets/newrelic.py:175-180
Timestamp: 2025-08-04T06:12:09.457Z
Learning: In the NewRelic toolset's GetTraces.get_parameterized_one_liner() method, the return strings only use trace_id and duration parameters, not any query strings that would need truncation.
Applied to files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.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 (11)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2 (4)
18-21: Excellent addition of tag discovery guidance.The new step for using
list_datadog_metric_tagsprovides clear guidance on discovering available tags and aggregations, which will help users construct accurate queries.
27-41: Critical workflow guidance addresses a common pitfall.The pod name resolution workflow is excellent - it addresses a common confusion where users might try to use deployment/service names directly as pod names in Datadog queries. The clear explanation of why this fails and the two-step process to resolve it will prevent many query failures.
47-62: Well-structured investigation patterns.The common investigation patterns provide practical guidance for different scenarios (pod/container, node-level, service-level metrics) with concrete examples. This will significantly improve the user experience.
64-76: Clear guidance on query result handling.The instructions for embedding execution results with tool_name and random_key, along with the formatting requirements for multiple graphs, provide clear expectations for the LLM response format.
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (7)
30-32: LGTM! Proper import organization.The imports are correctly placed at the top of the file following the coding guidelines, and the new datetime import and keygen_utils import are appropriately positioned.
66-66: Good clarification of tag naming conventions.The updated description clearly specifies that
pod_nameis used for pod tags andkube_namespacefor namespace tags, which will help users construct correct queries.
194-203: Well-designed parameter additions.The new
description(required) andoutput_type(optional) parameters are well-documented. Theoutput_typeparameter provides clear guidance on different formatting options (Plain, Bytes, Percentage, CPUUsage).
256-302: Excellent Prometheus-compatible transformation logic.The transformation from Datadog series format to Prometheus-compatible format is well-implemented:
- Properly extracts metric names and labels from Datadog scope
- Converts timestamps from milliseconds to seconds
- Handles pointlist data correctly
- Creates proper Prometheus matrix result structure
- Includes all required fields (random_key, tool_name, description, etc.)
346-347: Good enhancement to one-liner description.Including the description parameter in the one-liner provides better context for the query execution.
464-547: Well-implemented ListMetricTags tool.The new tool provides essential functionality for discovering available tags and aggregations:
- Clear parameter validation
- Proper error handling for common HTTP status codes (404, 429, 403)
- Uses the correct Datadog API v2 endpoint
- Returns structured results for downstream processing
- Good error messages that guide users
564-564: Proper tool registration.The new
ListMetricTagstool is correctly added to the toolset's tools list.
2 participants
Add tool for fetching metrics tags
Small fixes on the dd metrics toolset
Support rendering metrics blocks from dd