Improve historical data querying for logs and metrics - #905
Conversation
…_pod/send_datadog_logs.sh Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughMultiple logging toolsets delay creating their PodLoggingTool until after the base class finishes initializing so self.name exists. A new LoggingCapability.HISTORICAL_DATA was added and PodLoggingTool.description now references the toolset name and optionally notes historical-data support. Datadog docs, instructions, and test fixtures were updated to exercise historical-metrics/logs scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Construct as Toolset constructor
participant Base as BaseToolset
participant PodTool as PodLoggingTool
Construct->>Base: super().__init__(tools=[])
activate Base
Base-->>Construct: base init complete (self.name set)
deactivate Base
Note right of Construct: post-init create logging tool
Construct->>PodTool: self.tools = [PodLoggingTool(self)]
activate PodTool
PodTool-->>Construct: initialized (description may include HISTORICAL_DATA)
deactivate PodTool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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/logging_utils/logging_api.py (1)
50-56: Add missing type hints to the Pydantic validatorType hints are required by the repo guidelines. Please annotate the validator’s argument and return type to keep mypy happy with Pydantic v2’s expectations.
Apply this diff:
- def convert_start_time_to_string(cls, v): + def convert_start_time_to_string(cls, v: object) -> object:
🧹 Nitpick comments (6)
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml (1)
5-7: Normalize boolean casing for consistency.YAML booleans are case-insensitive, but we already use lowercase true for Datadog below. Recommend using false here for consistency.
- kubernetes/logs: - enabled: False + kubernetes/logs: + enabled: falseholmes/plugins/toolsets/logging_utils/logging_api.py (2)
89-93: Avoid duplicating “historical data” messaging across toolset and tool descriptionsThe tool description appends the historical-data note based on capability, while some toolsets (e.g., Datadog) also hard-code it in their toolset description. Consider relying on capability-driven text only to keep messages consistent in one place.
124-131: Documentation: start_time accepts integers too (converted to strings)The parameter description says “negative string number,” but the validator also accepts ints. Clarify to reduce confusion and reflect test behavior.
Apply this diff:
- "start_time": ToolParameter( - description=f"Start time for logs. Can be an RFC3339 formatted datetime (e.g. '2023-03-01T10:30:00Z') for absolute time or a negative string number (e.g. -3600) for relative seconds before end_time. Default: -{DEFAULT_TIME_SPAN_SECONDS} (last {DEFAULT_TIME_SPAN_SECONDS // SECONDS_PER_DAY} days)", + "start_time": ToolParameter( + description=f"Start time for logs. Can be an RFC3339 formatted datetime (e.g. '2023-03-01T10:30:00Z') for absolute time or a negative integer/string number (e.g. -3600) for relative seconds before end_time. Default: -{DEFAULT_TIME_SPAN_SECONDS} (last {DEFAULT_TIME_SPAN_SECONDS // SECONDS_PER_DAY} days)",tests/plugins/toolsets/test_logging_api.py (1)
21-21: Good: tests now provide a toolset name for description augmentationSetting mock_toolset.name in each test aligns with the new description logic and avoids attribute errors. To DRY the setup, consider a small helper factory inside this module to build a mock toolset with defaults (name, supported_capabilities, and fetch_pod_logs stub).
Example helper (optional, no markers involved):
def make_mock_toolset(name="test-logging-backend", capabilities=None): m = MagicMock(spec=BasePodLoggingToolset) m.name = name m.supported_capabilities = capabilities or set() m.fetch_pod_logs.return_value = StructuredToolResult( data="Sample logs", status=ToolResultStatus.SUCCESS ) return mYou could also add one new test to assert the description wiring:
def test_description_includes_name_and_historical_capability(): mock_toolset = make_mock_toolset( capabilities={LoggingCapability.HISTORICAL_DATA} ) tool = PodLoggingTool(mock_toolset) assert "from test-logging-backend" in tool.description assert "including historical data" in tool.descriptionAlso applies to: 60-60, 92-92, 121-121, 145-145
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
85-88: Nit: avoid shadowing built-in ‘filter’ and escape backslashes tooMinor polish: rename the local variable to avoid shadowing Python’s built-in and also escape backslashes before quotes to keep the Datadog query well-formed when payload is JSON-encoded.
Apply this diff:
- if params.filter: - filter = params.filter.replace('"', '\\"') - query += f' "{filter}"' + if params.filter: + filter_term = params.filter.replace("\\", "\\\\").replace('"', '\\"') + query += f' "{filter_term}"'holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
31-36: Declare ‘config’ with a type to satisfy mypy (attribute set outside init)This class sets self.config in prerequisites_callable, which can trip mypy for “attribute defined outside init”. Add a typed attribute at class level (or set it in init) to appease static checks.
Add near the top of the class:
class CoralogixLogsToolset(BasePodLoggingToolset): + config: Optional[CoralogixConfig] = None
📜 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 (9)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py(3 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py(1 hunks)holmes/plugins/toolsets/kubernetes_logs.py(1 hunks)holmes/plugins/toolsets/logging_utils/logging_api.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(1 hunks)tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml(1 hunks)tests/plugins/toolsets/test_logging_api.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/logging_utils/logging_api.pyholmes/plugins/toolsets/kubernetes_logs.pyholmes/plugins/toolsets/opensearch/opensearch_logs.pyholmes/plugins/toolsets/coralogix/toolset_coralogix_logs.pyholmes/plugins/toolsets/grafana/toolset_grafana_loki.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pytests/plugins/toolsets/test_logging_api.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.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/logging_utils/logging_api.pyholmes/plugins/toolsets/kubernetes_logs.pyholmes/plugins/toolsets/opensearch/opensearch_logs.pyholmes/plugins/toolsets/coralogix/toolset_coralogix_logs.pyholmes/plugins/toolsets/grafana/toolset_grafana_loki.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.py
tests/llm/**/toolsets.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/toolsets.yaml: Eval toolset overrides must be defined in a separate toolsets.yaml file in the test directory (do not put toolset config in test_case.yaml)
In toolsets.yaml, all toolset-specific configuration must be nested under a config field
Only the following top-level fields are allowed in toolsets YAML: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Files:
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
tests/llm/**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.{yaml,yml}: Each LLM eval test must use a dedicated Kubernetes namespace named app-
For Kubernetes-related eval assets, always use Secrets for scripts; do not embed scripts in inline manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Files:
tests/plugins/toolsets/test_logging_api.py
🧠 Learnings (4)
📚 Learning: 2025-08-05T00:42:23.792Z
Learnt from: vishiy
PR: robusta-dev/holmesgpt#782
File: config.example.yaml:31-49
Timestamp: 2025-08-05T00:42:23.792Z
Learning: In robusta-dev/holmesgpt config.example.yaml, the azuremonitorlogs toolset configuration shows "enabled: true" as an example of how to enable the toolset, not as a default setting. The toolset is disabled by default and requires explicit enablement in user configurations.
Applied to files:
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
📚 Learning: 2025-08-24T07:21:02.579Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.579Z
Learning: Applies to tests/llm/**/toolsets.yaml : Only the following top-level fields are allowed in toolsets YAML: 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/91f_datadog_logs_historical_pod/toolsets.yaml
📚 Learning: 2025-08-24T07:21:02.579Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.579Z
Learning: Applies to tests/llm/**/toolsets.yaml : Eval toolset overrides must be defined in a separate toolsets.yaml file in the test directory (do not put toolset config in test_case.yaml)
Applied to files:
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
📚 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/toolsets/kubernetes_logs.py
🧬 Code graph analysis (6)
holmes/plugins/toolsets/logging_utils/logging_api.py (6)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
supported_capabilities(33-35)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
supported_capabilities(143-147)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (1)
supported_capabilities(39-41)holmes/plugins/toolsets/kubernetes_logs.py (1)
supported_capabilities(54-59)holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
supported_capabilities(37-39)tests/plugins/toolsets/test_toolset_utils.py (1)
supported_capabilities(175-176)
holmes/plugins/toolsets/kubernetes_logs.py (2)
holmes/core/tools.py (1)
ToolsetTag(109-112)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
PodLoggingTool(76-202)
holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
holmes/plugins/toolsets/logging_utils/logging_api.py (1)
PodLoggingTool(76-202)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (2)
holmes/core/tools.py (1)
ToolsetTag(109-112)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
PodLoggingTool(76-202)
holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (1)
holmes/plugins/toolsets/logging_utils/logging_api.py (1)
PodLoggingTool(76-202)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
holmes/plugins/toolsets/logging_utils/logging_api.py (2)
LoggingCapability(25-32)PodLoggingTool(76-202)
⏰ 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). (7)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (11)
tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml (2)
5-7: Disable k8s logs explicitly here — good separation of concerns for this eval.
- Using a separate toolsets.yaml to override toolset enablement is aligned with the guidelines.
- Per the learnings, toolset-specific config is correctly nested under config for Datadog, and the only fields used under each toolset key are allowed.
4-21: No namespace issues found in 91f_datadog_logs_historical_pod fixtureI’ve checked the contents of this test fixture:
- There are no standalone Kubernetes manifests in this directory that declare a
namespace:field.- The test_case.yaml’s
user_promptcorrectly specifies app-91f as the namespace.- The
before_testscript invokessend_datadog_logs.sh app-91f, so all log‐pushing operations use the intended namespace.No changes are needed.
holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
48-55: Two-phase tool initialization avoids premature access to toolset.name — LGTM.Initializing with tools=[] and assigning self.tools = [PodLoggingTool(self)] after super().init ensures the PodLoggingTool can read the finalized name/capabilities to build accurate descriptions.
holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (1)
50-54: Deferred PodLoggingTool creation is correct and consistent with the shared logging API.This keeps tool descriptions accurate and prevents ordering bugs stemming from name/capabilities not being set yet.
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
57-58: Descriptions now clearly communicate Datadog as the source and mention historical data — good UX improvement.
- Improves clarity around where data comes from and sets correct expectations about pods that no longer exist.
Also applies to: 185-186, 566-566
holmes/plugins/toolsets/kubernetes_logs.py (1)
70-75: Two-phase initialization is applied correctly.Creating PodLoggingTool only after the base Toolset is initialized prevents description/capability mismatches and keeps behavior consistent with other toolsets.
holmes/plugins/toolsets/logging_utils/logging_api.py (2)
30-32: HISTORICAL_DATA capability addition looks goodEnum extension is backward-compatible and clearly documented. No impact on toolsets that return an empty capability set.
84-87: Nice: tool description now includes the toolset nameGood UX improvement with a sensible fallback when the name is empty.
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (2)
144-147: Capability declaration is correctDeclaring HISTORICAL_DATA here makes the PodLoggingTool description auto-augment correctly. No regex/exclude filters exposed, which matches the current implementation.
150-162: Two-phase initialization is appropriate hereInitializing with tools=[] and constructing PodLoggingTool after super().init ensures toolset.name is available for description building. This mirrors the pattern elsewhere.
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
44-49: Two-phase init: LGTMDeferring PodLoggingTool construction until after base init prevents name access issues and aligns with the new pattern.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2 (3)
36-40: Solid historical-pod guidance; add scoping, wildcard, and time-window guardrailsGreat call to skip k8s existence checks for exact pod names to unlock historical metrics. To reduce false positives and avoid empty graphs, I suggest tightening the instructions with namespace/cluster scoping, explicit handling of wildcards, and a fallback to widen the time window for historical pods.
Apply this diff to enrich the section:
**When user provides an exact pod name** (e.g., `my-workload-5f9d8b7c4d-x2km9`): - Query Datadog directly with that pod name using appropriate metrics and tags - Do NOT try to verify if the pod exists in Kubernetes first - This allows querying historical pods that have been deleted/replaced + - When namespace/cluster are known from the conversation or context, include them to avoid cross-environment leakage, e.g.: + `container.cpu.usage{pod_name:...,kube_namespace:...,kube_cluster_name:...}` + - If the user-provided "pod name" contains wildcards (e.g., `*`) or lacks the random suffix, treat it as a generic workload and use the workflow below (do not send wildcard tag values). + - For historical pods, expand the time window (e.g., last 24h or 7d) if the default 1h window returns no data before concluding that no metrics exist.
41-46: Be explicit about deployment/service tag alternatives and no-pod-found fallbackThe alternative path is good; spell out concrete tag keys and behavior when
kubectl_find_resourcereturns nothing (e.g., deleted namespaces). This also helps avoid the “*in pattern” pitfall mentioned in the PR notes.Apply this diff to clarify the workflow and provide examples:
**When user provides a generic workload name** (e.g., "my-workload", "nginx", "telemetry-processor"): - First use `kubectl_find_resource` to find actual pod names - Example: `kubectl_find_resource` with "my-workload" → finds pods like "my-workload-8f8cdfxyz-c7zdr" - Then use those specific pod names in Datadog queries - - Alternative: Use deployment-level tags when appropriate + - Alternative: Use workload-level tags when appropriate: + - Deployment-level: `kube_deployment:my-workload` + - Service-level: `kube_service:my-service` + - Always add `kube_namespace:<ns>` and, when available, `kube_cluster_name:<cluster>` to scope results. + - Examples: + - `container.cpu.usage{kube_deployment:my-workload,kube_namespace:default}` + - `container.cpu.usage{kube_service:frontend,kube_namespace:payments}` + - If `kubectl_find_resource` returns no pods (e.g., the namespace was deleted), still query Datadog using workload-level tags and expand the time window to cover when the workload last existed. + - Avoid wildcard tag values (e.g., `pod_name:my-workload-*`). Resolve exact pod names or use workload-level tags. Use `list_datadog_metric_tags` to discover valid tag values.
49-50: Reinforce pod-name ephemerality; prefer workload tags for stable viewsNice clarification. Add one more line to guide stable dashboards/answers toward workload-level tags by default, using
pod_nameonly when the ask targets a specific pod instance.Apply this diff:
**Why this matters:** - Pod names in Datadog are the actual Kubernetes pod names (with random suffixes) - Historical pods that no longer exist in the cluster can still have metrics in Datadog - Deployment/service names alone are NOT pod names (they need the suffix) + - For long-lived or comparative views, prefer workload-level tags (`kube_deployment`, `kube_service`) and add `kube_namespace`/`kube_cluster_name` to avoid flapping as pods roll.tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml (1)
1-3: Add fail-fast and verify helper script in before_testTo ensure the fixture setup fails clearly if the helper is missing and stops on any error, wrap the commands in a strict shell invocation and check for the script’s existence. You can keep the namespace as
defaultto match the conversation context.Apply this diff to
tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml:before_test: | + set -euo pipefail + test -f ../../shared/send_datadog_metrics.sh # Send CPU and memory metrics to Datadog for a specific robusta-runner mentioned in conversation history bash ../../shared/send_datadog_metrics.sh default robusta-runner-78599b764d-f847h robusta-runnertests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/conversation_history.json (2)
4-4: Optional: Clarify namespace in the user message or keep it namespace-agnostic.Given the guideline to use app-, you can either:
- keep the user line namespace-agnostic (fine); or
- explicitly reference app-92 for clarity.
If you prefer explicitness, apply:
- "content": "Can you show the memory usage of the robusta-runner-78599b764d-f847h pod?", + "content": "Can you show the memory usage of the robusta-runner-78599b764d-f847h pod in the app-92 namespace?",
1-12: Naming consistency: folder suggests CPU, prompt is about memory.The fixture directory is 92_cpu_graph_conversation, while the conversation asks about memory. This can confuse future maintainers and result triage.
Consider either:
- Renaming the folder to 92_metrics_graph_conversation; or
- Switching the user prompt to CPU to align with the folder name.
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yaml (2)
13-15: Reduce flakiness: increase wait timeout to 120sClusters can be slow in CI; 60s may flake. 120s is safer.
- kubectl wait --for=condition=ready pod -l app=orbiter-monitor -n app-91g --timeout=60s + kubectl wait --for=condition=ready pod -l app=orbiter-monitor -n app-91g --timeout=120s
1-1: Optional: neutralize scenario naming — rename tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_podDirectory name contains "mismatched_pod" which hints at the failure; please rename to a neutral name (example: 91g_datadog_metrics or 91g_orbiter).
- Affected path: tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/ (e.g., test_case.yaml).
- Note: an rg scan shows many other non‑neutral names under tests/llm/fixtures (crashpod, broken, leak, error). Consider a short repository sweep to align all tests/llm/** with the project guideline to use neutral scenario names.
This uses the project guideline (tests/llm/**) to avoid revealing the root cause in fixture/resource names.
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml (3)
44-58: Harden pod/container security (allowPrivilegeEscalation, non-root, RoFS)Satisfy CKV_K8S_20 and CKV_K8S_23 by setting securityContext at pod/container level. Even for tests, this reduces risk without impacting behavior.
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault volumes: @@ containers: - name: app image: busybox:latest + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 1000 command: ["/bin/sh", "-c"]
46-58: Pin BusyBox image to a stable tag to avoid CI driftlatest can change underneath you. Use a fixed tag (or digest) to stabilize tests.
- image: busybox:latest + image: busybox:1.36.1
14-22: Nit: simplify CPU load loopThe echo + awk pipeline is a bit odd (the echoed string is unused). A simpler CPU burner keeps intent clear.
script.sh: | #!/bin/sh echo "Starting orbiter-monitor pod..." # Simulate some CPU and memory usage while true; do - # Consume some CPU with a calculation - echo "scale=5000; 4*a(1)" | busybox awk 'BEGIN{for(i=0;i<100;i++){}}' 2>/dev/null - # Sleep to create varying pattern + # Consume some CPU + busybox awk 'BEGIN{for(i=0;i<5e7;i++) s+=i}' + # Sleep to create a varying pattern sleep 5 done
📜 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 (6)
holmes/plugins/toolsets/datadog/datadog_metrics_instructions.jinja2(1 hunks)tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.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)
🧰 Additional context used
📓 Path-based instructions (5)
tests/llm/**/toolsets.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/toolsets.yaml: Eval toolset overrides must be defined in a separate toolsets.yaml file in the test directory (do not put toolset config in test_case.yaml)
In toolsets.yaml, all toolset-specific configuration must be nested under a config field
Only the following top-level fields are allowed in toolsets YAML: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml
tests/llm/**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.{yaml,yml}: Each LLM eval test must use a dedicated Kubernetes namespace named app-
For Kubernetes-related eval assets, always use Secrets for scripts; do not embed scripts in inline manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yamltests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yamltests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yamltests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yamltests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yamltests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yamltests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/conversation_history.jsontests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml
tests/llm/**/test_case.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Eval test cases may declare runbooks in test_case.yaml using either runbooks: {} or runbooks: {catalog: [...]}; if omitted, defaults are used
Files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yamltests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml
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/datadog_metrics_instructions.jinja2
🧠 Learnings (3)
📚 Learning: 2025-08-24T07:21:02.579Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.579Z
Learning: Applies to tests/llm/**/toolsets.yaml : Only the following top-level fields are allowed in toolsets YAML: 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/91g_datadog_metrics_mismatched_pod/toolsets.yaml
📚 Learning: 2025-08-24T07:21:02.579Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.579Z
Learning: Applies to tests/llm/**/toolsets.yaml : Eval toolset overrides must be defined in a separate toolsets.yaml file in the test directory (do not put toolset config in test_case.yaml)
Applied to files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml
📚 Learning: 2025-08-24T07:21:02.579Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.579Z
Learning: Applies to tests/llm/**/toolsets.yaml : In toolsets.yaml, all toolset-specific configuration must be nested under a config field
Applied to files:
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml
[MEDIUM] 24-58: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 24-58: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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 (5)
tests/llm/fixtures/test_ask_holmes/92_cpu_graph_conversation/test_case.yaml (1)
16-21: Test oracle looks good and matches PR goal (ensure final output embeds a Datadog graph).The expected_output constraints explicitly enforce emitting a datadogql embed via query_datadog_metrics in the final answer (not only intermediate output), which addresses point (4) in the PR description. Looks solid.
tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml (2)
1-9: Structure and config placement look good
- Toolset overrides are in a dedicated toolsets.yaml.
- Tool-specific settings are correctly nested under config.
- Secrets are sourced from env vars (good for CI).
7-9: Updated Datadog keys verification scriptThe original snippet failed under
set -udue to an invalid${#VAR:-0}expansion. To both check for presence and report lengths safely, please replace it with:#!/bin/bash set -euo pipefail # Safely default to empty strings if unset api_key="${DATADOG_API_KEY:-}" app_key="${DATADOG_APP_KEY:-}" echo "DATADOG_API_KEY length: ${#api_key}" echo "DATADOG_APP_KEY length: ${#app_key}" echo "Configured site_api_url: https://api.datadoghq.eu" # Fail if either key is missing if [[ -z "$api_key" || -z "$app_key" ]]; then echo "Error: Missing Datadog keys (ensure EU-site API and APP keys are set)" exit 1 fi• This will correctly handle undefined variables under
set -u.
• Once updated, please re-run the check in your CI to confirm bothDATADOG_API_KEYandDATADOG_APP_KEYare populated for the EU site.tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yaml (1)
24-31: No action required:tool_nameis stableVerified that the Datadog metrics tool remains named
"query_datadog_metrics"in both its implementation (holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py:184) and across all related fixtures and tests. The existing assertion in the YAML fixture is therefore correct and does not need updating.tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml (1)
40-43: Confirm script mounting and permissionsSecret volume with defaultMode 0755 ensures /etc/scripts/script.sh is executable. Mount path aligns with args. LGTM.
Also applies to: 49-51
arikalon1
left a comment
arikalon1
left a comment
There was a problem hiding this comment.
looks good, nice work
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (3)
145-148: Clarify capability docstring (this property enumerates optional features only).Substring matching is the default fallback when regex isn’t supported; it’s not an explicit capability. Suggest tightening the docstring to avoid confusion.
- """Datadog logs API supports historical data and substring matching""" + """Optional capabilities: Datadog logs support historical data (no regex/exclude)."""
153-153: Nit: brand consistency ("Datadog").Description uses “Datadog” while
logger_name()returns “DataDog”. Consider aligning to “Datadog” everywhere for consistency.Outside this hunk:
def logger_name(self) -> str: return "Datadog"
279-286: Add return type hint for mypy compliance.Project guideline: type hints are required. This private helper should declare
-> None.- def _reload_instructions(self): + def _reload_instructions(self) -> None: """Load Datadog logs specific troubleshooting instructions.""" template_file_path = os.path.abspath( os.path.join(os.path.dirname(__file__), "datadog_logs_instructions.jinja2") ) self._load_llm_instructions(jinja_template=f"file://{template_file_path}")
📜 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 (2)
holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2(1 hunks)holmes/plugins/toolsets/datadog/toolset_datadog_logs.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/plugins/toolsets/datadog/datadog_logs_instructions.jinja2
🧰 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
🧬 Code graph analysis (1)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (4)
holmes/plugins/toolsets/logging_utils/logging_api.py (2)
LoggingCapability(25-32)PodLoggingTool(76-202)holmes/core/tools.py (3)
CallablePrerequisite(329-330)ToolsetTag(109-112)_load_llm_instructions(479-484)holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
_reload_instructions(634-641)holmes/plugins/toolsets/datadog/toolset_datadog_traces.py (1)
_reload_instructions(65-72)
⏰ 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 (2)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (2)
1-1: Import placement is correct.
osis imported at the top-level as required.
157-157: Two-phase tool initialization looks good.Deferring
PodLoggingToolcreation until afterBasePodLoggingToolsetinit avoids name/description ordering footguns and matches other toolsets.
2 participants
This should be good to review and merge. Holmes is now more reliable at querying historical data in datadog for pods no longer in the cluster.
Full evals run for '-m logging or datadog'
Results table from CLI:
I reviewed the residual failures and they are because:
In 91f, holmes queried
pod_name: api-gateway-*instead ofpod_name: api-gateway-*(see run here) - this is a real failure and a target for future improvement, either by letting holmes run datadog queries raw or by updating tool description to mention * explicitlyIn 91a, if the entire namespace no longer exists in the cluster (not just the pod) holmes sometimes doesn't even try to fetch logs - this is a target for future improvement too
Holmes brought metrics correctly from datadog but didn't render a graph due to ambiguous user questions (e.g. here)
I verified in these cases that holmes really fetched the information with the right queries.