-
Couldn't load subscription status.
- Fork 183
tempo traces test error 115 #925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces a new test fixture suite for case 115 that deploys a Flask-based Checkout service with OpenTelemetry tracing to Tempo, a traffic generator that produces mixed promo/non-promo requests, toolset configuration for Kubernetes and Tempo, and an end-to-end test harness that provisions, validates logs/traces, and cleans up. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TG as Traffic Generator
participant CS as Checkout Service (Flask)
participant DB as Simulated DB (span)
participant Tempo as Tempo (OTLP)
TG->>CS: POST /checkout (payload)
activate CS
note over CS: start span: process_checkout
CS->>DB: start span: database_query
alt promo_code present
DB-->>CS: Simulated DB error
note over CS: recordException, setStatus(ERROR)
CS--)Tempo: OTLP spans (error)
CS-->>TG: 500 {"error": "..."}
else no promo_code
DB-->>CS: Simulated success (rates/discount)
CS-->>TG: 200 {"order_id", "subtotal","shipping","total"}
CS--)Tempo: OTLP spans (success)
end
deactivate CS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (10)
tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/traffic-generator.yaml (3)
125-160: Harden the container (non-root and no privilege escalation).Address CKV_K8S_20/23 by adding a minimal securityContext.
containers: - name: traffic-generator image: python:3.11-slim + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 + runAsGroup: 10001 command: ["/bin/bash", "-c"]
101-103: Fix comment wording.Minor grammar nit.
- # Wait 10ms to 50ms second before next request + # Wait 10–50 ms before the next request
12-12: Remove unused import.
datetimeisn’t used.- from datetime import datetimetests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/checkout-service.yaml (4)
135-169: Add basic container hardening.Run as non-root and disable privilege escalation.
containers: - name: checkout image: python:3.11-slim + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 + runAsGroup: 10001 command: ["/bin/bash", "-c"]
153-161: Consider adding a readinessProbe.You have a startupProbe; a readinessProbe on /health improves service gating.
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: 3
9-19: Drop unused import.
osisn’t referenced.- import os
68-69: Nit: placeholder style vs declared DB.Query uses
?placeholders (SQLite style) whiledb.systemispostgresql. It’s fine for a dummy, but consider aligning for clarity ($1, $2) or add a comment.tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/test_case.yaml (3)
70-72: Align message and wait time (reduce flakiness).Message says 45s but sleeps 20s. Either change text or actually wait longer to ensure promo traffic and traces.
- 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
74-86: Avoid--tail=-1for portability.Some kubectl versions reject negative tails. Default returns all logs.
- if kubectl logs -n app-115 -l app=traffic-generator --tail=-1 | grep -q "WITH promo_code"; then + if kubectl logs -n app-115 -l app=traffic-generator | grep -q "WITH promo_code"; then @@ - if kubectl logs -n app-115 -l app=traffic-generator --tail=-1 | grep -q "WITHOUT promo_code"; then + if kubectl logs -n app-115 -l app=traffic-generator | grep -q "WITHOUT promo_code"; then
1-1: Optional: neutralize fixture directory name.Guideline prefers neutral names. Consider
115_checkout_tracinginstead of115_checkout_errors_tracing(update references accordingly).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/checkout-service.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/toolsets.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/traffic-generator.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/115_checkout_errors_tracing/test_case.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/115_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/toolsets.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/checkout-service.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/115_checkout_errors_tracing/test_case.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/toolsets.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/traffic-generator.yamltests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/checkout-service.yaml
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/115_checkout_errors_tracing/toolsets.yaml
🧠 Learnings (3)
📚 Learning: 2025-08-24T07:21:02.611Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.611Z
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/115_checkout_errors_tracing/toolsets.yaml
📚 Learning: 2025-08-24T07:21:02.611Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.611Z
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/115_checkout_errors_tracing/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/115_checkout_errors_tracing/toolsets.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/traffic-generator.yaml
[MEDIUM] 111-163: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 111-163: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/checkout-service.yaml
[MEDIUM] 121-173: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 121-173: 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). (7)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (1)
tests/llm/fixtures/test_ask_holmes/115_checkout_errors_tracing/toolsets.yaml (1)
8-10: Good: tool-specific settings are nested underconfig.
No description provided.