New Evals for newrelic - #983
Conversation
This demo environment in the trace-demo namespace runs three microservices—checkout (Node.js), inventory (Node.js), and risk (Python/Flask)—alongside a Postgres 16 database with an orders table. Checkout orchestrates requests by calling Inventory for stock checks, Risk for fraud scoring, and then writing to Postgres before replying to the client. Each service exposes a /healthz endpoint, logs via standard libraries, and is labeled for New Relic auto-instrumentation, so traces stitch across all components.
query httpResponseCode instead of http.statusCode and gets empty data https://www.braintrust.dev/app/robustadev/p/HolmesGPT/experiments/tomerkeshet-freeform-newrelic-k%3D117_new-20250914_100547?c=&r=9ad754c9-8fa6-4024-ab9f-6e79630e9ee5&s=18a0be5f-8e62-4681-8487-8fbc08787495
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (6)
holmes/core/supabase_dal.py (3)
334-349: Avoid duplicate entries when adding alert_raw_data; also add type hints and simplify logic.alert_raw_data items are currently included in
dataand also appended post‑decompression, yielding duplicates (and sinceunzip_evidence_filemutates in place, you end up with the same dict twice). Exclude compressed items up front and use a single membership test for the decompressible types. Added return/param hints per guidelines.-def extract_relevant_issues(self, evidence): +def extract_relevant_issues(self, evidence: Any) -> List[Dict]: + decompress_types = {"text_file", "alert_raw_data"} data = [ enrich for enrich in evidence.data - if enrich.get("enrichment_type") not in ENRICHMENT_BLACKLIST_SET + if enrich.get("enrichment_type") not in ENRICHMENT_BLACKLIST_SET + and enrich.get("enrichment_type") not in decompress_types ] unzipped_files = [ self.unzip_evidence_file(enrich) for enrich in evidence.data - if enrich.get("enrichment_type") == "text_file" - or enrich.get("enrichment_type") == "alert_raw_data" + if enrich.get("enrichment_type") in decompress_types ] data.extend(unzipped_files) return data
8-8: Import Any to support the added type hint.Required for the
evidence: Anyannotation.-from typing import Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple
330-333: Sanitize exception logging to avoid leaking payloads (PII/compliance risk).On unzip failure we log the entire evidence record, which may include sensitive data. Log only metadata.
- except Exception: - logging.exception(f"Unknown issue unzipping gz finding: {data}") - return data + except Exception: + logging.exception( + "Failed to unzip gz finding (type=%s, evidence_id=%s, data_len=%s).", + data.get("enrichment_type", "unknown"), + data.get("id", "unknown"), + len(str(data.get("data", ""))), + ) + return dataholmes/plugins/toolsets/newrelic/newrelic.py (3)
100-101: Return a StructuredToolResult on missing configuration instead of raising.Tools under holmes/{core,plugins} must surface detailed, structured errors rather than raising. Replace the ValueError with an ERROR result that includes context.
Apply:
- if not self._toolset.nr_api_key or not self._toolset.nr_account_id: - raise ValueError("NewRelic API key or account ID is not configured") + if not self._toolset.nr_api_key or not self._toolset.nr_account_id: + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR, + error="New Relic API key or account ID is not configured", + params={"reason": "missing_config"}, + invocation="newrelic_execute_nrql_query", + )
136-147: Metrics branch likely inverted the fallback condition.Currently, when results exist, you overwrite the formatted payload with the raw result; that’s the opposite of the intended flow. Also consider signaling NO_DATA explicitly when empty.
- return_result = self.format_metrics(result, params=enriched_params) - if len(return_result.get("data", {}).get("results", [])): - return_result = result # type: ignore[assignment] + return_result = self.format_metrics(result, params=enriched_params) + if not return_result.get("data", {}).get("results"): + # Optional: keep raw NRQL rows to aid debugging empty responses + return StructuredToolResult( + status=StructuredToolResultStatus.NO_DATA, + data={ + "message": "No metrics returned from NRQL", + "query": query, + "raw_result": result, + }, + params=params, + invocation=query, + )(The final SUCCESS return below can remain unchanged.)
155-198: Avoid logging entire log records on exception (PII risk).Dumping records may leak sensitive data. Log count and a sample of keys instead.
- except Exception: - logging.exception(f"Failed to reformat newrelic logs {records}") - return records + except Exception: + logging.exception( + "Failed to reformat newrelic logs; record_count=%d", + len(records) if isinstance(records, list) else -1, + ) + return records
🧹 Nitpick comments (8)
tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml (1)
7-7: Consider making EU datacenter setting more explicit.The comment suggests using
"{{env.NEW_RELIC_IS_EU}}"but defaults to hardcodedfalse. Consider defaulting to the environment variable with a fallback to maintain consistency.- is_eu_datacenter: false # set "{{env.NEW_RELIC_IS_EU}}" with true to use eu + is_eu_datacenter: "{{env.NEW_RELIC_IS_EU|false}}" # set to true to use EU datacenterThis makes the pattern consistent with the other configuration values and allows runtime override while maintaining the same default behavior.
holmes/plugins/toolsets/newrelic/newrelic.py (4)
199-201: Add type hint to params.mypy requirement: all Python code must include type hints.
- def get_parameterized_one_liner(self, params) -> str: + def get_parameterized_one_liner(self, params: Dict[str, Any]) -> str:
253-255: Add return type to stub.- def call_nerdql(self): + def call_nerdql(self) -> None: pass
66-66: Typo: “breif” -> “brief”.User-facing copy should be clean.
- description="A breif 6 word human understandable description of the query you are running.", + description="A brief 6 word human understandable description of the query you are running.",
41-41: Typo in docs string: “Usa ge” -> “Usage”.-### ⚠️ Critical Rule: NRQL `FACET` Usa ge +### ⚠️ Critical Rule: NRQL `FACET` Usagetests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/setup.sh (3)
3-3: Namespace creation should be idempotent.
kubectl create namespace app-117fails on reruns. Use get-or-create to support repeated CI runs.-kubectl create namespace app-117 +kubectl get ns app-117 >/dev/null 2>&1 || kubectl create namespace app-117
5-16: Make secret creation idempotent to avoid rerun failures.
kubectl create secreterrors if the secret exists. Use dry-run + apply.-kubectl -n app-117 create secret generic checkout-src \ - --from-file=server.js=checkout/server.js \ - --from-file=package.json=checkout/package.json +kubectl -n app-117 create secret generic checkout-src \ + --from-file=server.js=checkout/server.js \ + --from-file=package.json=checkout/package.json \ + --dry-run=client -o yaml | kubectl apply -f - -kubectl -n app-117 create secret generic inventory-src \ - --from-file=server.js=inventory/server.js \ - --from-file=package.json=inventory/package.json +kubectl -n app-117 create secret generic inventory-src \ + --from-file=server.js=inventory/server.js \ + --from-file=package.json=inventory/package.json \ + --dry-run=client -o yaml | kubectl apply -f - -kubectl -n app-117 create secret generic risk-src \ - --from-file=app.py=risk/app.py \ - --from-file=requirements.txt=risk/requirements.txt +kubectl -n app-117 create secret generic risk-src \ + --from-file=app.py=risk/app.py \ + --from-file=requirements.txt=risk/requirements.txt \ + --dry-run=client -o yaml | kubectl apply -f -
25-27: Wait on the StatefulSet directly for stronger guarantees (optional).Waiting on pods by label works but can be brittle with restarts. If the name is stable, prefer
statefulset/postgres.-echo "Waiting for postgres statefulset to be ready..." -kubectl wait --for=condition=ready --timeout=300s pod -l app=postgres -n app-117 +echo "Waiting for postgres statefulset to be ready..." +kubectl wait --for=condition=ready --timeout=300s statefulset/postgres -n app-117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
holmes/core/supabase_dal.py(1 hunks)holmes/plugins/toolsets/newrelic/newrelic.jinja2(1 hunks)holmes/plugins/toolsets/newrelic/newrelic.py(3 hunks)tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/instrumentation.yaml(2 hunks)tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/setup.sh(1 hunks)tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/120_new_relic_traces2/test_case.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/test_case.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/fixtures/test_ask_holmes/120_new_relic_traces2/test_case.yaml
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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/core/supabase_dal.pyholmes/plugins/toolsets/newrelic/newrelic.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/core/supabase_dal.pyholmes/plugins/toolsets/newrelic/newrelic.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/setup.shtests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/instrumentation.yamltests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/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/117_new_relic_tracing/instrumentation.yamltests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.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/117_new_relic_tracing/instrumentation.yamltests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.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/117_new_relic_tracing/instrumentation.yamltests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml
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/117b_new_relic_block_embed/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/117b_new_relic_block_embed/toolsets.yaml
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/newrelic/newrelic.pyholmes/plugins/toolsets/newrelic/newrelic.jinja2
🧠 Learnings (3)
📚 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/117b_new_relic_block_embed/toolsets.yaml
📚 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 tests/llm/**/test_case.yaml : Do not include toolset configuration in test_case.yaml; if runbooks are specified, entries must point to .md files in the same directory
Applied to files:
tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml
📚 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 tests/llm/**/toolsets.yaml : 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
Applied to files:
tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml
🧬 Code graph analysis (1)
holmes/plugins/toolsets/newrelic/newrelic.py (2)
holmes/utils/keygen_utils.py (1)
generate_random_key(5-6)holmes/core/tools.py (2)
StructuredToolResult(78-102)StructuredToolResultStatus(51-75)
🪛 Shellcheck (0.11.0)
tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/setup.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ 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). (4)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (8)
tests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml (2)
1-7: LGTM! Configuration structure follows the required pattern.The toolset configuration correctly places all New Relic-specific settings under the top-level
configfield as required by the coding guidelines. The structure withenabled: trueand environment variable placeholders for sensitive data follows best practices.
5-6: Document NEW_RELIC_ACCOUNT_ID and NEW_RELIC_API_KEY in test setuptests/llm/fixtures/test_ask_holmes/117b_new_relic_block_embed/toolsets.yaml (lines 5–6) references NEW_RELIC_ACCOUNT_ID and NEW_RELIC_API_KEY but a repository-wide search returned no documentation or README entries. Add both variables to the relevant test setup/prerequisites (fixture README or project README) and state expected values/format.
holmes/plugins/toolsets/newrelic/newrelic.py (2)
18-18: Import of generate_random_key is appropriate.Matches the new embedding workflow that requires a per-response key.
225-226: Graduating toolset from experimental: verify readiness.Non-experimental implies stability. Confirm docs updated and e2e tests cover logs, metrics (TIMESERIES), traces, and empty-result paths.
Would you like a quick script to scan tests under tests/llm/fixtures/test_ask_holmes for NR cases and list coverage gaps?
tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/instrumentation.yaml (3)
1-2: Docs links helpful.Keeping pointers to upstream operator docs in the fixture is useful for maintenance.
12-17: Confirm reserved namespace label exists or use a custom labelFile: tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/instrumentation.yaml Lines: 12-17 (also applies to 27-31)
Automated check failed: kubectl not available in sandbox. Manually verify that namespace "app-117" exposes label "kubernetes.io/metadata.name" — if empty, add and match a custom label instead.
Verify:
kubectl get ns app-117 -o jsonpath='{.metadata.labels.kubernetes.io/metadata.name}{"\n"}'Add label (if needed):
kubectl label ns app-117 app.newrelic.instrumentation=enabled --overwrite
11-11: Pin New Relic init images to an immutable version or digest — do not use :latestReplace :latest with a specific version tag (major.minor.patch, or nodejs-suffixed for Node builds) or, preferably, a sha256 digest to make CI reproducible. New Relic supports versioned tags and Node-specific tags (e.g., 11.19.0-nodejs20x).
Example:
- image: newrelic/newrelic-node-init:latest + image: newrelic/newrelic-node-init:<major.minor.patch-nodejsXX> # or sha256:<digest> - image: newrelic/newrelic-python-init:latest + image: newrelic/newrelic-python-init:<major.minor.patch> # or sha256:<digest>tests/llm/fixtures/test_ask_holmes/117_new_relic_tracing/setup.sh (1)
17-20: No action required — both db.yaml and manifest.yaml already specify metadata.namespace: app-117.
Applying them without -n will deploy into app-117.
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)
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml (1)
15-73: Use the required dedicated namespace per test: app-121 (and fix all occurrences).Current namespace "nr-115" violates the guideline and may collide with other evals. Align the echo text with the test id too.
-before_test: | - echo "🚀 Setting up test 115 - Creating namespace nr-115" - kubectl create namespace nr-115 || true - echo "✅ Namespace nr-115 created successfully!" +before_test: | + echo "🚀 Setting up test 121 - Creating namespace app-121" + kubectl create namespace app-121 || true + echo "✅ Namespace app-121 created successfully!" @@ - kubectl apply -f ../../shared/tempo.yaml -n nr-115 + kubectl apply -f ../../shared/tempo.yaml -n app-121 @@ - kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n nr-115 + kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n app-121 @@ - kubectl apply -f checkout-service.yaml -n nr-115 + kubectl apply -f checkout-service.yaml -n app-121 @@ - kubectl wait --for=condition=ready pod -l app=checkout -n nr-115 --timeout=60s + kubectl wait --for=condition=ready pod -l app=checkout -n app-121 --timeout=60s @@ - kubectl get pods -n nr-115 -l app=checkout + kubectl get pods -n app-121 -l app=checkout @@ - kubectl apply -f traffic-generator.yaml -n nr-115 + kubectl apply -f traffic-generator.yaml -n app-121 @@ - kubectl wait --for=condition=ready pod -l app=traffic-generator -n nr-115 --timeout=60s + kubectl wait --for=condition=ready pod -l app=traffic-generator -n app-121 --timeout=60s @@ - kubectl get pods -n nr-115 + kubectl get pods -n app-121 @@ - if kubectl logs -n nr-115 -l app=traffic-generator --tail=-1 | grep -q "WITH promo_code"; then + if kubectl logs -n app-121 -l app=traffic-generator --tail=-1 | grep -q "WITH promo_code"; then @@ - if kubectl logs -n nr-115 -l app=traffic-generator --tail=-1 | grep -q "WITHOUT promo_code"; then + if kubectl logs -n app-121 -l app=traffic-generator --tail=-1 | grep -q "WITHOUT promo_code"; then @@ - if kubectl logs -n nr-115 -l app=checkout --tail=100 | grep -q "Processing checkout request"; then + if kubectl logs -n app-121 -l app=checkout --tail=100 | grep -q "Processing checkout request"; then @@ - kubectl delete -f traffic-generator.yaml -n nr-115 + kubectl delete -f traffic-generator.yaml -n app-121 @@ -after_test: | - kubectl delete namespace nr-115 || true +after_test: | + kubectl delete namespace app-121 || true
🧹 Nitpick comments (7)
holmes/plugins/toolsets/newrelic/newrelic.jinja2 (2)
39-41: Tighten guidance: add explicit time window; grammar; safe LIMIT exampleMinor copy and query hygiene to prevent heavy queries and fix wording.
- - ALWAYS include an embed, and don't include the raw results as plain text, for example << {"type": "traces-summary", "tool_name": "newrelic_execute_nrql_query", "random_key": "928js0l"} >>. "random_key" should always be replaced with the "random_key" from the tool's output. - - ALWAYS start by querying all fields using `SELECT * FROM DistributedTraceSummary`. We need as much fields as possible to visualize the traces to the user. However, the trade-off is that we might exceed the context size. In that case, if you need to narrow down your search, follow this strategy: First, select only the essential fields: trace.id, spanCount, root.entity.accountId, root.entity.guid, root.entity.name, root.span.name, timestamp, duration.ms. These are the absolute minimum fields required for the visualization. If that's still failing, add the `LIMIT` keyword to the query. `LIMIT` should always be the second option, we prefer to show the user as much traces as we can. + - ALWAYS include an embed, and don't include the raw results as plain text; for example: << {"type": "traces-summary", "tool_name": "newrelic_execute_nrql_query", "random_key": "928js0l"} >>. Replace "random_key" with the value returned by the tool. + - ALWAYS start by querying all fields using `SELECT * FROM DistributedTraceSummary SINCE 1 hour ago`. We need as many fields as possible to visualize the traces for the user. If this exceeds context, first select only the essential fields: `trace.id, spanCount, root.entity.accountId, root.entity.guid, root.entity.name, root.span.name, timestamp, duration.ms`. If that's still too large, add a conservative `LIMIT` (for example, `LIMIT 50`) as a last resort—we prefer to show the user as many traces as we can.Note: The essential fields you listed are valid DistributedTraceSummary attributes. (docs.newrelic.com)
42-42: Fix typo: “even type” → “event type”Also small punctuation polish.
- - If you query any other even type (like Span or Transaction), don't use any embeds - return the results as is. + - If you query any other event type (like Span or Transaction), don't use any embeds—return the results as is.tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml (1)
42-44: Align message with actual sleep.Echo says 45s but sleeps 20s.
- echo "⏰ Letting traffic generator run for 45 seconds to generate requests" - sleep 20 + echo "⏰ Letting traffic generator run for 20 seconds to generate requests" + sleep 20tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml (2)
247-313: Harden the pod/container security context (Checkov CKV_K8S_20, CKV_K8S_23).Run as non‑root, drop caps, disable privilege escalation, enable seccomp, and use read‑only root FS.
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: traffic-generator image: python:3.11-slim + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 1000 + runAsGroup: 1000 + capabilities: + drop: ["ALL"] command: ["/bin/bash", "-c"]
131-137: Optional: avoid tracing the New Relic log POSTs.Auto‑instrumenting requests will also create spans for NR log ingestion calls, adding noise. Consider suppressing instrumentation in NewRelicLogHandler.emit when posting logs.
If you want, I can draft a minimal suppression using the OpenTelemetry “suppress_instrumentation” context utility compatible with your pinned versions.
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml (2)
260-327: Harden the checkout pod/container security context (Checkov CKV_K8S_20, CKV_K8S_23).Mirror the same security hardening as the generator.
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: checkout image: python:3.11-slim + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 1000 + runAsGroup: 1000 + capabilities: + drop: ["ALL"] command: ["/bin/bash", "-c"]
136-141: Optional: avoid tracing NR log POSTs from the app too.Same consideration as the generator to reduce span noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
holmes/plugins/toolsets/newrelic/newrelic.jinja2(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/toolsets.yaml
🧰 Additional context used
📓 Path-based instructions (6)
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/121_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.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/121_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.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/121_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml
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/newrelic/newrelic.jinja2
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/121_new_relic_checkout_errors_tracing/test_case.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml
[medium] 233-313: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 233-313: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml
[medium] 247-333: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 247-333: 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/plugins/toolsets/newrelic/newrelic.jinja2 (2)
37-37: Fix invalid NRQL attribute on DistributedTraceSummary
root.span.eventTypedoesn’t exist on DistributedTraceSummary; this will yield empty results. Use an existence check on fields that do exist (or drop the filter entirely). The data dictionary listsroot.entity.*,root.span.*,trace.id,spanCount,timestamp,duration.ms, but notroot.span.eventType.Apply:
- - ALWAYS use the filter `WHERE root.span.eventType = 'Span'` + - Do not filter on `root.span.eventType` (it doesn't exist on DistributedTraceSummary). If you must sanity‑check root span presence, use `WHERE root.span.id IS NOT NULL` or omit this filter altogether.Sources: New Relic attribute dictionary entries for DistributedTraceSummary showing available attributes (no
root.span.eventType). (docs.newrelic.com)
39-39: Verify embed “type” and tool name match the codeConfirm that the consumer recognizes
type: "traces-summary"andtool_name: "newrelic_execute_nrql_query"; there’s a risk of silent no‑ops if these identifiers don’t match your dispatcher.tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml (1)
188-189: Cross‑file check: make sure test grep matches this log.App logs “Processing checkout for user …”. The test has been updated to grep this exact string.
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml (1)
20-20: Shared Tempo path verified — ../../shared/tempo.yaml resolves to tests/llm/fixtures/shared/tempo.yaml (file exists).
33f5f4a to
8f54d41
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
tests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yaml (5)
176-180: Ensure volumes item indentation is valid YAML.Minor indentation nits here can also trigger “expected ':'” errors with strict linters.
Apply:
- volumes: - - name: app + volumes: + - name: app secret: secretName: checkout-app
1-1: Remove scenario‑revealing comment.Artifacts must be neutral; “Dummy SQL” hints at simulation.
- # Simplified Checkout Service with Dummy SQL
70-93: Make latency deterministic; avoid “simulate” wording in code comments.Random sleeps can cause flaky evals and “simulate” comments are non‑neutral. Use env‑configurable fixed delays and neutral comments.
- # Simulate database query for shipping calculation + # Database query for shipping calculation (emulated latency) - if promo_code: - # Simulate slow query with promo_code + if promo_code: + # Slow path when promo code is used query = "SELECT rate_per_kg, discount_percent FROM shipping_rates WHERE zone_id = ? AND promo_code = ? AND active = true" db_span.set_attribute("db.statement", query) - # print(f"[DB] Executing shipping rate query", flush=True) - sleep_time = random.uniform(1.5, 3.5) - time.sleep(sleep_time) # Simulate slow query + slow_ms = int(os.environ.get("SLOW_QUERY_MS", "2500")) + time.sleep(slow_ms / 1000.0) shipping_rate = 4.5 discount = 15.0 else: - # Simulate fast query without promo_code + # Fast path without promo code query = "SELECT rate_per_kg, discount_percent FROM shipping_rates WHERE zone_id = ? AND active = true" db_span.set_attribute("db.statement", query) - # print(f"[DB] Executing shipping rate query", flush=True) - sleep_time = random.uniform(0.1, 0.2) - time.sleep(sleep_time) # Simulate fast query + fast_ms = int(os.environ.get("FAST_QUERY_MS", "150")) + time.sleep(fast_ms / 1000.0) shipping_rate = 5.0 discount = 0.0Additionally, add these near the imports (already using os):
from opentelemetry.instrumentation.flask import FlaskInstrumentor + +# Tunable latency via env (defaults used if not provided)
136-139: Pin dependencies for reproducibility.Installing floating latest versions in-cluster risks future breakage.
Consider switching to a requirements file in the Secret and install with:
- pip install flask opentelemetry-api opentelemetry-sdk \ - opentelemetry-instrumentation-flask \ - opentelemetry-exporter-otlp-proto-grpc && \ + pip install --no-cache-dir -r /app/requirements.txt && \ python /app/app.pyand add stringData.requirements.txt with pinned versions (or a constraints file).
160-168: Add a readinessProbe (startupProbe alone isn’t sufficient).Ensures traffic only routes when the app is ready.
startupProbe: httpGet: path: /health port: 8080 initialDelaySeconds: 10 periodSeconds: 5 timeoutSeconds: 3 successThreshold: 1 failureThreshold: 24 + readinessProbe: + httpGet: + path: /health + port: 8080 + periodSeconds: 5 + timeoutSeconds: 2 + failureThreshold: 3tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yaml (2)
72-85: Harden log checks to avoid false positives.Grepping the last 100 lines may miss earlier entries on slower clusters. Consider a wider window or label selector per container.
Apply:
- if kubectl logs -n "${TEST_NS}" -l app=traffic-generator --tail=100 | grep -q "WITH promo_code"; then + if kubectl logs -n "${TEST_NS}" -l app=traffic-generator --tail=1000 | grep -q "WITH promo_code"; thenRepeat similarly for WITHOUT promo_code.
118-121: Neutralize meta/mocked language in test logs.Avoid phrases that reveal simulation (“so the ai won't cheat”).
- # Delete Traffic generator so the ai won't cheat + # Delete traffic generator to stop background load before assertionstests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml (3)
121-154: Add basic container securityContext (non-root, no privilege escalation).Addresses CKV_K8S_20 and CKV_K8S_23.
- name: traffic-generator image: python:3.11-slim + securityContext: + runAsNonRoot: true + runAsUser: 1000 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"]
125-130: Pin dependency versions for determinism.- pip install requests && \ + pip install requests==2.32.3 && \
12-12: Remove unused import.- from datetime import datetimetests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yaml (3)
130-176: Add container securityContext (non-root, no privilege escalation).Same CKV findings as traffic generator.
- name: checkout image: python:3.11-slim + securityContext: + runAsNonRoot: true + runAsUser: 1000 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"]
136-139: Pin Python package versions to reduce flakiness.- pip install flask opentelemetry-api opentelemetry-sdk \ - opentelemetry-instrumentation-flask \ - opentelemetry-exporter-otlp-proto-grpc && \ + pip install \ + flask==3.0.3 \ + opentelemetry-api==1.26.0 \ + opentelemetry-sdk==1.26.0 \ + opentelemetry-instrumentation-flask==0.47b0 \ + opentelemetry-exporter-otlp-proto-grpc==1.26.0 && \Adjust to the repo’s standard pin set if different.
55-55: Avoid logging PII-like identifiers in plain logs (even in tests).Consider masking or truncating
user_id.- print(f"[CHECKOUT] Processing checkout request for user {data.get('user_id', 'guest')}", flush=True) + print("[CHECKOUT] Processing checkout request", flush=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/122_new_relic_checkout_latency_tracing_rebuild /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/122_new_relic_checkout_latency_tracing_rebuild /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/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yamltests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.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/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yamltests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.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/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yamltests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yamltests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml
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/122_new_relic_checkout_latency_tracing_rebuild /test_case.yaml
🧠 Learnings (4)
📚 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 tests/llm/**/test_case.yaml : Do not include toolset configuration in test_case.yaml; if runbooks are specified, entries must point to .md files in the same directory
Applied to files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml
📚 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/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml
📚 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 tests/llm/**/toolsets.yaml : 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
Applied to files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml
📚 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/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml
🪛 YAMLlint (1.37.1)
tests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yaml
[error] 116-116: syntax error: could not find expected ':'
(syntax)
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /checkout-service.yaml
[medium] 116-180: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 116-180: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml
[medium] 105-157: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 105-157: 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: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
🔇 Additional comments (4)
tests/llm/fixtures/test_ask_holmes/114_checkout_latency_tracing_rebuild/checkout-service.yaml (2)
34-39: Verify OTLP endpoint aligns with the New Relic eval.Endpoint points to Tempo (tempo.app-114…), while this PR is for New Relic evals. If Tempo is an intermediary, fine—otherwise update to the intended collector/ingest.
Would you confirm the expected path (Tempo → NR or direct NR OTLP)?
8-44: Good: script stored in a Secret and OTel Flask instrumentation.Using a Secret for code and instrumenting Flask via OpenTelemetry align with the guidelines.
Please confirm no other manifests in this test embed scripts inline or via ConfigMaps.
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /traffic-generator.yaml (1)
25-33: If tracing is re-enabled later, update OTLP endpoint to app-122 or make it configurable.-# endpoint="tempo.app-114.svc.cluster.local:4317", +# endpoint="tempo.app-122.svc.cluster.local:4317",Or read from
OTEL_EXPORTER_OTLP_ENDPOINTenv var.tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild /toolsets.yaml (1)
1-10: Approve —healthcheckis a supported grafana/tempo config key.GrafanaConfig defines
healthcheck(default "ready") and GrafanaTempoConfig inherits it;grafana_health_checkusesconfig.healthcheckand the Tempo toolset additionally verifies the Tempo echo endpoint.
Locations: holmes/plugins/toolsets/grafana/common.py, holmes/plugins/toolsets/grafana/grafana_api.py, holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (16)
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yaml (2)
43-85: Synchronous log shipping can block on network; consider async buffering.Current handler POSTs per log with 5s timeout. For higher rates, use a queue + background thread or batch sender. Fine for small evals, but worth a TODO.
151-157: Avoid reassigning tracer redundantly.setup_telemetry already returns a tracer; the second get_tracer call is unnecessary.
- tracer, logger = setup_telemetry("billing-atc", "billing-service") + tracer, logger = setup_telemetry("billing-atc", "billing-service") @@ - tracer = trace.get_tracer(__name__) + # tracer already set by setup_telemetrytests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/test_case.yaml (1)
25-26: Optional: add a post-deploy sanity curl to reduce flakes.After waits, curl the Service once to ensure HTTP 200 before starting traffic.
kubectl get pods -n app-123 -l app=billing-123 +kubectl run curl --rm -i -t --image=curlimages/curl:8.7.1 -n app-123 --restart=Never -- \ + curl -sS http://billing-123.app-123.svc.cluster.local:8080/health || trueAlso applies to: 35-35
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yaml (2)
93-96: Avoid hard-coded namespace default in telemetry.Mirror the change made in billing to not bake in nr-115.
- ns = os.getenv("KUBERNETES_NAMESPACE") or os.getenv("K8S_NAMESPACE") or "nr-115" + ns = os.getenv("KUBERNETES_NAMESPACE") or os.getenv("K8S_NAMESPACE") or ""
212-223: Nit: Unused Flask instrumentation in generator requirements.Generator doesn’t use Flask; you can drop opentelemetry-instrumentation-flask and Flask to slim the image.
- opentelemetry-instrumentation-flask==0.41b0 - Flask==2.3.3tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml (1)
236-316: Harden the container security context.Comply with baseline policies: non-root, no privilege escalation, drop caps, read-only FS.
containers: - name: traffic-generator image: python:3.11-slim + securityContext: + runAsNonRoot: true + runAsUser: 10001 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] @@ spec: + securityContext: + fsGroup: 10001tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml (1)
9-13: Template EU flag from env with a safe default.Current inline comment suggests env usage but the value is hardcoded.
newrelic: enabled: true config: nr_account_id: "{{env.NEW_RELIC_ACCOUNT_ID}}" nr_api_key: "{{env.NEW_RELIC_API_KEY}}" - is_eu_datacenter: false # set "{{env.NEW_RELIC_IS_EU}}" with true to use eu + is_eu_datacenter: "{{ env.NEW_RELIC_IS_EU | default(false) }}"tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml (1)
45-47: Align cleanup to the namespace variable.-after_test: | - kubectl delete namespace nr-114 || true +after_test: | + kubectl delete namespace "${NS:-app-122}" || truetests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml (2)
44-46: Comment/sleep mismatch.Message says 45s, code sleeps 20s. Align to avoid flakiness.
- echo "⏰ Letting traffic generator run for 45 seconds to generate requests" - sleep 20 + echo "⏰ Letting traffic generator run for 45 seconds to generate requests" + sleep 45
73-75: Cleanup should delete the chosen namespace.-after_test: | - kubectl delete namespace nr-115 || true +after_test: | + kubectl delete namespace "${NS:-app-121}" || truetests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yaml (2)
3-6: Avoid hardcoding namespace in manifests applied with -n.Drop metadata.namespace for portability and reusability (the test sets -n).
metadata: name: payment-app - namespace: nr-114
312-327: Add container and pod security context.Harden according to baseline policies.
startupProbe: httpGet: path: /health port: 8080 initialDelaySeconds: 60 # allow time for pip install periodSeconds: 5 timeoutSeconds: 3 successThreshold: 1 failureThreshold: 60 # generous budget for slow networks resources: + securityContext: + runAsNonRoot: true + runAsUser: 10001 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] requests: memory: "128Mi" cpu: "50m" limits: memory: "256Mi" cpu: "200m" @@ spec: + securityContext: + fsGroup: 10001tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml (1)
265-332: Harden the container security context.Apply non-root, no privilege escalation, drop caps, read-only FS.
- name: checkout image: python:3.11-slim command: ["/bin/bash", "-c"] args: - | pip install -r /app/requirements.txt && \ python /app/app.py + securityContext: + runAsNonRoot: true + runAsUser: 10001 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] @@ spec: + securityContext: + fsGroup: 10001tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yaml (2)
243-250: Startup probe signals ready before dependencies install.You touch /tmp/ready before pip install, causing premature success. Move touch after pip.
- set -e - touch /tmp/ready && \ - python -m pip install --no-cache-dir --disable-pip-version-check -r /app/requirements.txt && \ + set -e + python -m pip install --no-cache-dir --disable-pip-version-check -r /app/requirements.txt && \ + touch /tmp/ready && \ python /app/app.py
240-307: Harden the container and pod security context.Align with baseline policies.
- name: traffic-generator image: python:3.11-slim command: ["/bin/bash", "-c"] + securityContext: + runAsNonRoot: true + runAsUser: 10001 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] @@ spec: + securityContext: + fsGroup: 10001holmes/plugins/toolsets/newrelic/newrelic.jinja2 (1)
31-32: Tighten guidance and fix typos (“latnecy” → “latency”).Current phrasing is awkward and has a typo.
- - When investigating a trace also look at attributes - - ***When investigating latency ALWAYS look to deliver the specific component or attribute in the span causing significant latnecy*** your investigation is not complete without this + - When investigating a trace, also inspect relevant attributes. + - ***When investigating latency, identify the specific component or span attribute contributing the most latency***; the investigation isn’t complete without this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
holmes/plugins/prompts/_fetch_logs.jinja2(2 hunks)holmes/plugins/toolsets/newrelic/newrelic.jinja2(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/toolsets.yaml
🧰 Additional context used
📓 Path-based instructions (9)
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/123_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.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/123_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.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/123_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.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/123_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yamltests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yamltests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yamltests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml
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
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/122_new_relic_checkout_latency_tracing_rebuild/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/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml
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/newrelic/newrelic.jinja2
🧠 Learnings (7)
📚 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
📚 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 tests/llm/fixtures/**/*.yaml : Each LLM test must use a dedicated Kubernetes namespace 'app-<testid>' to prevent conflicts when running in parallel
Applied to files:
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml
📚 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/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml
📚 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 tests/llm/**/test_case.yaml : Do not include toolset configuration in test_case.yaml; if runbooks are specified, entries must point to .md files in the same directory
Applied to files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml
📚 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 tests/llm/**/toolsets.yaml : 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
Applied to files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml
📚 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/122_new_relic_checkout_latency_tracing_rebuild/toolsets.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yaml
[medium] 244-333: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 244-333: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yaml
[medium] 225-310: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 225-310: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yaml
[medium] 259-348: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 259-348: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yaml
[medium] 225-310: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 225-310: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml
[medium] 251-337: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 251-337: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml
[medium] 236-316: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 236-316: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (12)
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/billing-service.yaml (1)
1-7: Good: scripts are stored in a Secret, not inline manifests/ConfigMaps.Complies with tests/llm/**/*.yaml guideline to use Secrets for scripts.
Please confirm other fixtures in this PR also use Secrets for embedded scripts.
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/test_case.yaml (1)
4-7: Expectation wording is precise and testable. LGTM.Keeps the model from passing by only mentioning promo codes. Good.
tests/llm/fixtures/test_ask_holmes/123_new_relic_checkout_errors_tracing/traffic-generator.yaml (1)
1-7: Good: scripts are stored in a Secret.Compliant with evals guideline.
holmes/plugins/prompts/_fetch_logs.jinja2 (1)
70-71: OK to list ‘newrelic’ as an option here.Naming is consistent with tests/llm toolsets (“newrelic”). No change needed.
tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/traffic-generator.yaml (2)
36-40: Fix OTLP gRPC endpoint: use host:port, not https://.gRPC exporter expects host:port. The https scheme will break exports.
- otlp = "https://otlp.eu01.nr-data.net:4317" if region == "eu" else "https://otlp.nr-data.net:4317" + otlp = "otlp.eu01.nr-data.net:4317" if region == "eu" else "otlp.nr-data.net:4317"#!/bin/bash # Find any remaining incorrect gRPC endpoints across the repo rg -nP 'https://otlp\.[\w.-]+:4317' -C2
148-159: Avoid hardcoded namespace in CHECKOUT_URL.Build service DNS from the pod namespace (Downward API) so this works in app-121 and future tests.
- import time, random + import os, time, random @@ - CHECKOUT_URL = "http://checkout.nr-115.svc.cluster.local:8080/checkout" + NS = os.getenv("KUBERNETES_NAMESPACE", "default") + CHECKOUT_URL = f"http://checkout.{NS}.svc.cluster.local:8080/checkout"tests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/test_case.yaml (2)
17-25: Remove duplicate namespace creation and switch to app-121.Second create is redundant; also adopt required namespace naming.
-before_test: | +before_test: | [ -n "${NEW_RELIC_ACCOUNT_ID:-}" ] && [ -n "${NEW_RELIC_API_KEY:-}" ] && [ -n "${NEW_RELIC_LICENSE_KEY:-}" ] || { for v in NEW_RELIC_ACCOUNT_ID NEW_RELIC_API_KEY NEW_RELIC_LICENSE_KEY; do [ -n "${!v:-}" ] || echo "Missing env var: $v"; done; exit 1; } - - echo "🚀 Setting up test 115 - Creating namespace nr-115" - kubectl create namespace nr-115 || true - echo "✅ Namespace nr-115 created successfully!" + NS=app-121 + echo "🚀 Setting up test 121 - Creating namespace ${NS}" + kubectl create namespace "${NS}" || true + echo "✅ Namespace ${NS} created successfully!" @@ - kubectl apply -f ../../shared/tempo.yaml -n nr-115 + kubectl apply -f ../../shared/tempo.yaml -n "${NS}" - - kubectl create namespace nr-115 - kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n nr-115 + kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n "${NS}"
62-66: Fix log assertion to match actual app log line.Use the correct message to avoid false failures.
- if kubectl logs -n nr-115 -l app=checkout --tail=100 | grep -q "Processing checkout request"; then + if kubectl logs -n "${NS}" -l app=checkout --tail=100 | grep -q "Processing checkout for user"; thentests/llm/fixtures/test_ask_holmes/121_new_relic_checkout_errors_tracing/checkout-service.yaml (1)
37-41: Fix OTLP gRPC endpoint: use host:port, not https://.Same gRPC issue as noted before.
- otlp = "https://otlp.eu01.nr-data.net:4317" if region == "eu" else "https://otlp.nr-data.net:4317" + otlp = "otlp.eu01.nr-data.net:4317" if region == "eu" else "otlp.nr-data.net:4317"holmes/plugins/toolsets/newrelic/newrelic.jinja2 (3)
44-44: Typo: “even type” → “event type”.Same nit was raised previously; applying it here too.
- - If you query any other even type (like Span or Transaction), don't use any embeds - return the results as is. + - If you query any other event type (like Span or Transaction), don't use any embeds—return the results as is.
35-35: Replace Prometheus tool with NRQL tool in New Relic exampleholmes/plugins/toolsets/newrelic/newrelic.jinja2 (line 35) references execute_prometheus_range_query — replace it with the NRQL execution tool used in this toolset (e.g. newrelic_execute_nrql_query). I couldn't verify canonical names in the sandbox (search produced no definitive matches); confirm the embed "type" and tool_name across the repo and update the example.
- - You always need to embed the partial result in your response. Include the "tool_name" and "random_key". For example: << {"type": "newrelicql", "tool_name": "execute_prometheus_range_query", "random_key": "92jf2hf"} >> + - You always need to embed the partial result in your response. Include the "tool_name" and "random_key". For example: << {"type": "newrelicql", "tool_name": "newrelic_execute_nrql_query", "random_key": "92jf2hf"} >>
42-42: Minor grammar fix & DistributedTraceSummary fields verifiedGrammar tweak below; the listed fields are present on DistributedTraceSummary per New Relic's attribute dictionary. (docs.newrelic.com)
- - ALWAYS start by querying all fields using `SELECT * FROM DistributedTraceSummary`. We need as much fields as possible to visualize the traces to the user. However, the trade-off is that we might exceed the context size. In that case, if you need to narrow down your search, follow this strategy: First, select only the essential fields: trace.id, spanCount, root.entity.accountId, root.entity.guid, root.entity.name, root.span.name, timestamp, duration.ms. These are the absolute minimum fields required for the visualization. If that's still failing, add the `LIMIT` keyword to the query. `LIMIT` should always be the second option, we prefer to show the user as much traces as we can. + - ALWAYS start by querying all fields using `SELECT * FROM DistributedTraceSummary`. We need as many fields as possible to visualize traces for the user. However, you might exceed context size. If you need to narrow the result, first select only these essential fields: trace.id, spanCount, root.entity.accountId, root.entity.guid, root.entity.name, root.span.name, timestamp, duration.ms. If that still fails, add `LIMIT` (this is a secondary option).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml (2)
15-16: Harden the setup script with strict mode.Fail fast on errors for reproducible test runs.
Apply this diff:
before_test: | + set -Eeuo pipefail [ -n "${NEW_RELIC_ACCOUNT_ID:-}" ] && [ -n "${NEW_RELIC_API_KEY:-}" ] && [ -n "${NEW_RELIC_LICENSE_KEY:-}" ] || { for v in NEW_RELIC_ACCOUNT_ID NEW_RELIC_API_KEY NEW_RELIC_LICENSE_KEY; do [ -n "${!v:-}" ] || echo "Missing env var: $v"; done; exit 1; }
45-46: Use the same app-122 namespace in cleanup.Define NS locally to avoid drift between blocks.
Apply this diff:
after_test: | - kubectl delete namespace nr-114 || true + NS=app-122 + kubectl delete namespace "${NS}" || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/122_new_relic_checkout_latency_tracing_rebuild/test_case.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/122_new_relic_checkout_latency_tracing_rebuild/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/122_new_relic_checkout_latency_tracing_rebuild/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/122_new_relic_checkout_latency_tracing_rebuild/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/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml
🧠 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 tests/llm/fixtures/**/*.yaml : Each LLM test must use a dedicated Kubernetes namespace 'app-<testid>' to prevent conflicts when running in parallel
Applied to files:
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml
⏰ 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 (4)
tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml (4)
4-7: Expected-output criteria look good.Clear and testable acceptance rules.
1-3: Use the mandated per-test namespace in the user prompt (app-122).Update the prompt to match the required 'app-' convention for test 122.
Apply this diff:
user_prompt: -- "The payment service in namespace nr-114 is experiencing high latency. Investigate why." +- "The payment service in namespace app-122 is experiencing high latency. Investigate why."
15-44: Make namespace configurable (app-122) throughout setup and fix the sleep message.Align with guidelines and keep logs truthful.
Apply this diff:
before_test: | - [ -n "${NEW_RELIC_ACCOUNT_ID:-}" ] && [ -n "${NEW_RELIC_API_KEY:-}" ] && [ -n "${NEW_RELIC_LICENSE_KEY:-}" ] || { for v in NEW_RELIC_ACCOUNT_ID NEW_RELIC_API_KEY NEW_RELIC_LICENSE_KEY; do [ -n "${!v:-}" ] || echo "Missing env var: $v"; done; exit 1; } - echo "🚀 Setting up test 114 - Creating namespace nr-114" - kubectl create namespace nr-114 || true - kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n nr-114 - echo "✅ Namespace nr-114 created successfully!" + [ -n "${NEW_RELIC_ACCOUNT_ID:-}" ] && [ -n "${NEW_RELIC_API_KEY:-}" ] && [ -n "${NEW_RELIC_LICENSE_KEY:-}" ] || { for v in NEW_RELIC_ACCOUNT_ID NEW_RELIC_API_KEY NEW_RELIC_LICENSE_KEY; do [ -n "${!v:-}" ] || echo "Missing env var: $v"; done; exit 1; } + NS=app-122 + echo "🚀 Setting up test 122 - Creating namespace ${NS}" + kubectl create namespace "${NS}" || true + kubectl create secret generic newrelickey --from-literal=key="${NEW_RELIC_LICENSE_KEY}" -n "${NS}" + echo "✅ Namespace ${NS} created successfully!" @@ - kubectl apply -f payment-service.yaml -n nr-114 + kubectl apply -f payment-service.yaml -n "${NS}" @@ - kubectl wait --for=condition=ready pod -l app=payment -n nr-114 --timeout=60s + kubectl wait --for=condition=ready pod -l app=payment -n "${NS}" --timeout=60s @@ - kubectl get pods -n nr-114 -l app=payment + kubectl get pods -n "${NS}" -l app=payment @@ - kubectl apply -f traffic-generator.yaml -n nr-114 + kubectl apply -f traffic-generator.yaml -n "${NS}" @@ - kubectl wait --for=condition=ready pod -l app=traffic-generator -n nr-114 --timeout=60s + kubectl wait --for=condition=ready pod -l app=traffic-generator -n "${NS}" --timeout=60s @@ - kubectl get pods -n nr-114 + kubectl get pods -n "${NS}" @@ - echo "⏰ Letting traffic generator run for 20 seconds to generate requests" - sleep 60 + echo "⏰ Letting traffic generator run for 60 seconds to generate requests" + sleep 60
1-46: Fix namespace and resource-name collisions in test 122
Replace hard-coded namespace "nr-114" → "app-122" (dedicated namespace) in:
- tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/test_case.yaml (lines 2,17–20,23,26,29,32,35,38,46)
- tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/payment-service.yaml (lines 5,94,248,338)
- tests/llm/fixtures/test_ask_holmes/122_new_relic_checkout_latency_tracing_rebuild/traffic-generator.yaml (lines 5,94,152,153,229)
Update hard-coded FQDNs: payment.nr-114.svc.cluster.local → payment.app-122.svc.cluster.local (traffic-generator.yaml:152–153).
Make metadata.name values test-unique (add suffix "-122" or similar) for:
- payment-app, payment (x2) in payment-service.yaml
- traffic-generator-app, traffic-generator in traffic-generator.yaml
Fix setup messaging: change "Setting up test 114" → "Setting up test 122" in test_case.yaml.
Re-run the duplicate-name scan after changes — current scan shows cross-test duplicates for "payment" and "traffic-generator" (must be resolved before parallel runs).
Co-authored-by: Tomer Keshet <tomer@robusta.dev> Co-authored-by: mershal <shahal@robusta.dev> Co-authored-by: Robusta Runner <aantny@gmail.com>
6 participants
No description provided.