Conversation
Thread trace_span through the compaction pipeline so compaction events appear as spans in Braintrust traces. Previously, compaction was invisible in traces because no span was created for the compaction LLM call or the surrounding orchestration logic. - Add trace_span parameter to compact_conversation_history() and wrap the compaction LLM call in a "Compaction LLM Call" span - Add trace_span parameter to limit_input_context_window() and wrap the full compaction flow in a "Conversation History Compaction" span with input/output logging (token counts, compression ratio, status) - Pass trace_span from ToolCallingLLM.call() to limit_input_context_window() - Pass compaction_span in test_compaction.py eval test https://claude.ai/code/session_01KagxBSbTp1AbDQTvbk1KL8 Signed-off-by: Claude <noreply@anthropic.com>
Add test_compaction.py to the eval-regression workflow so compaction tests (with their Braintrust trace spans) run in CI/CD. Tag 001_compaction and 003_cascading_failure with the regression marker so they execute in automatic regression runs. https://claude.ai/code/session_01KagxBSbTp1AbDQTvbk1KL8 Signed-off-by: Claude <noreply@anthropic.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 Run @ ebac52b (#22227862791)✅ Results of HolmesGPT evalsAutomatically triggered by commit ebac52b on branch Results of HolmesGPT evals
|
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| 🚧 | 09_crashpod | — | — | — | — |
| 🚧 | 101_loki_historical_logs_pod_deleted | — | — | — | — |
| ✅ | 111_pod_names_contain_service | 36.4s | 6 | 10 | $0.2267 |
| 🚧 | 112_find_pvcs_by_uuid | — | — | — | — |
| 🚧 | 12_job_crashing | — | — | — | — |
| 🚧 | 161_conversation_compaction | — | — | — | — |
| 🚧 | 163_compaction_follow_up | — | — | — | — |
| 🚧 | 176_network_policy_blocking_traffic_no_runbooks | — | — | — | — |
| 🚧 | 217_prometheus_compaction_heavy_queries | — | — | — | — |
| ✅ | 24_misconfigured_pvc | 36.0s | 5 | 14 | $0.2365 |
| 🚧 | 43_current_datetime_from_prompt | — | — | — | — |
| 🚧 | 61_exact_match_counting | — | — | — | — |
| Total | 36.2s avg | 5.5 avg | 12.0 avg | $0.4633 |
📜 Run @ 1f03b90 (#22065898512)
✅ Results of HolmesGPT evals
Automatically triggered by commit 1f03b90 on branch claude/show-compaction-traces-fH5c2
Results of HolmesGPT evals
- ask_holmes: 9/12 test cases were successful, 0 regressions, 3 setup failures
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 41.0s | 5 | 11 | $0.2320 |
| 🚧 | 101_loki_historical_logs_pod_deleted | — | — | — | — |
| ✅ | 111_pod_names_contain_service | 251.3s | 6 | 13 | $0.2563 |
| ✅ | 112_find_pvcs_by_uuid | 33.8s | 5 | 7 | $0.2197 |
| ✅ | 12_job_crashing | 65.4s | 5 | 12 | $0.2387 |
| 🚧 | 161_conversation_compaction | — | — | — | — |
| ✅ | 163_compaction_follow_up | 150.9s | 3 | 2 | $0.1773 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 72.4s | 6 | 15 | $0.2749 |
| 🚧 | 217_prometheus_compaction_heavy_queries | — | — | — | — |
| ✅ | 24_misconfigured_pvc | 69.7s | 7 | 17 | $0.2751 |
| ✅ | 43_current_datetime_from_prompt | 4.9s | 1 | — | $0.1070 |
| ✅ | 61_exact_match_counting | 21.0s | 4 | 4 | $0.1620 |
| Total | 78.9s avg | 4.7 avg | 10.1 avg | $1.9430 |
📜 Run @ 2e625fd (#22060324393)
✅ Results of HolmesGPT evals
Automatically triggered by commit 2e625fd on branch claude/show-compaction-traces-fH5c2
Results of HolmesGPT evals
- ask_holmes: 1/12 test cases were successful, 0 regressions, 11 setup failures
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| 🚧 | 09_crashpod | — | — | — | — |
| 🚧 | 101_loki_historical_logs_pod_deleted | — | — | — | — |
| ✅ | 111_pod_names_contain_service | 39.1s | 6 | 11 | $0.2338 |
| 🚧 | 112_find_pvcs_by_uuid | — | — | — | — |
| 🚧 | 12_job_crashing | — | — | — | — |
| 🚧 | 161_conversation_compaction | — | — | — | — |
| 🚧 | 163_compaction_follow_up | — | — | — | — |
| 🚧 | 176_network_policy_blocking_traffic_no_runbooks | — | — | — | — |
| 🚧 | 217_prometheus_compaction_heavy_queries | — | — | — | — |
| 🚧 | 24_misconfigured_pvc | — | — | — | — |
| 🚧 | 43_current_datetime_from_prompt | — | — | — | — |
| 🚧 | 61_exact_match_counting | — | — | — | — |
| Total | 39.1s avg | 6.0 avg | 11.0 avg | $0.2338 |
📜 Run @ ced3966 (#22043627765)
✅ Results of HolmesGPT evals
Automatically triggered by commit ced3966 on branch claude/show-compaction-traces-fH5c2
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 30.8s | 5 | 11 | $0.2319 |
| ✅ | 101_loki_historical_logs_pod_deleted | 51.6s | 7 | 14 | $0.3072 |
| ✅ | 111_pod_names_contain_service | 31.6s | 5 | 11 | $0.2229 |
| ✅ | 112_find_pvcs_by_uuid | 38.5s | 7 | 9 | $0.2714 |
| ✅ | 12_job_crashing | 25.1s | 4 | 8 | $0.2096 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 46.6s | 6 | 17 | $0.2994 |
| ✅ | 24_misconfigured_pvc | 34.6s | 6 | 14 | $0.2400 |
| ✅ | 43_current_datetime_from_prompt | 4.5s | 1 | — | $0.0111 |
| ✅ | 61_exact_match_counting | 16.4s | 4 | 4 | $0.1623 |
| Total | 31.1s avg | 5.0 avg | 11.0 avg | $1.9558 |
✅ Results of HolmesGPT evals
Automatically triggered by commit 92e438a on branch claude/show-compaction-traces-fH5c2
Results of HolmesGPT evals
- ask_holmes: 1/12 test cases were successful, 0 regressions, 11 setup failures
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Output | Cached | Non-cached | Reasoning | Max output | Compactions |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 🚧 | 09_crashpod | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 101_loki_historical_logs_pod_deleted | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 111_pod_names_contain_service | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 112_find_pvcs_by_uuid | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 12_job_crashing | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 161_conversation_compaction | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 163_compaction_follow_up | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 176_network_policy_blocking_traffic_no_runbooks | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 217_prometheus_compaction_heavy_queries | — | — | — | — | — | — | — | — | — | — | — | — |
| ✅ | 24_misconfigured_pvc | 33.9s | 5 | 14 | $0.1505 | 106,541 | 104,532 | 2,009 | 95,757 | 8,775 | — | 587 | — |
| 🚧 | 43_current_datetime_from_prompt | — | — | — | — | — | — | — | — | — | — | — | — |
| 🚧 | 61_exact_match_counting | — | — | — | — | — | — | — | — | — | — | — | — |
| Total | 33.9s avg | 5.0 avg | 14.0 avg | $0.1505 | 106,541 | 104,532 | 2,009 | 95,757 | 8,775 | — | 587 | — |
Benchmark Comparison Details
Baseline: latest ci-benchmark experiment on master
Status: Success - 61 test/model combinations loaded
Benchmark experiment:
- ci-benchmark-22534723634 (created: 2026-03-01)
Time comparison (seconds):
| Test case | This branch | master | Diff |
|---|---|---|---|
| 09_crashpod (opus-4.5) | — | 29.5s | — |
| 101_loki_historical_logs_pod_deleted (opus-4.5) | — | — | — |
| 111_pod_names_contain_service (opus-4.5) | — | 34.2s | — |
| 112_find_pvcs_by_uuid (opus-4.5) | — | 31.0s | — |
| 12_job_crashing (opus-4.5) | — | 29.7s | — |
| 161_conversation_compaction (opus-4.5) | — | — | — |
| 163_compaction_follow_up (opus-4.5) | — | — | — |
| 176_network_policy_blocking_traffic_no_runbooks (opus-4.5) | — | 42.7s | — |
| 217_prometheus_compaction_heavy_queries (opus-4.5) | — | — | — |
| 24_misconfigured_pvc (opus-4.5) | 33.9s | 38.7s | ↓12% |
| 43_current_datetime_from_prompt (opus-4.5) | — | — | — |
| 61_exact_match_counting (opus-4.5) | — | — | — |
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run.
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/show-compaction-traces-fH5c2 -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
filter: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
filter |
Pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals in automatic regression runs:
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
Examples: evals-tag-easy, evals-id-09_crashpod
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana-dashboard, hard, integration, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/show-compaction-traces-fH5c2 -f markers=regression -f filter=
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:0084ca7f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:0084ca7f me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:0084ca7f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:0084ca7f
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:0084ca7f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:0084ca7f me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:0084ca7f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:0084ca7fPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:0084ca7f \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:0084ca7fRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:0084ca7f \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:0084ca7f |
WalkthroughThis PR adds distributed tracing support to the conversation history compaction pipeline by propagating trace_span parameters through tool calling, context window limiter, and compaction functions. It includes new test fixtures and validation logic to verify compaction occurrence during test execution. Changes
Sequence DiagramsequenceDiagram
participant App as ToolCallingLLM
participant Limiter as InputContextWindowLimiter
participant Compaction as CompactionService
participant TracingSpan as TracingSpan
participant LLM as LLM
App->>Limiter: call(llm, messages, trace_span)
Limiter->>TracingSpan: start_span("Conversation History Compaction")
TracingSpan->>Limiter: compaction_span
Limiter->>Compaction: compact_conversation_history(history, llm, trace_span=compaction_span)
Compaction->>TracingSpan: start_span(trace_span)
TracingSpan->>LLM: completion(compaction_prompt)
LLM-->>Compaction: response
Compaction->>TracingSpan: log(success/failure)
Compaction-->>Limiter: CompactionResult(compacted_history, compacted=true)
Limiter->>TracingSpan: log_output(metrics)
Limiter->>Limiter: emit(CONVERSATION_HISTORY_COMPACTED)
Limiter->>Limiter: emit(AI_MESSAGE)
Limiter-->>App: ContextWindowLimiterOutput(messages, metadata={conversation_history_compacted: true})
App->>App: set metadata["conversation_history_compacted"] = True
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
This reverts commit 438e211.
- Tag evals 161 and 163 with `regression` so they run in CI - Create eval 217: Prometheus-heavy workload (35 pods) with a small context window (OVERRIDE_MAX_CONTEXT_SIZE=30000) that forces compaction to trigger naturally at the default 95% threshold - Add `assert_compaction` field to HolmesTestCase so evals can fail if compaction did not occur - Propagate `conversation_history_compacted` flag into LLMResult metadata so tests can inspect it https://claude.ai/code/session_01KagxBSbTp1AbDQTvbk1KL8 Signed-off-by: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/llm/fixtures/test_ask_holmes/217_prometheus_compaction_heavy_queries/manifest.yaml (2)
7-21: Unused ConfigMapcpu-script.The
cpu-scriptConfigMap is defined but none of the three deployments mount or reference it. Thepayment-processorsandorder-handlersdeployments use inline scripts instead. Remove it to avoid confusion.🧹 Remove unused ConfigMap
--- -# ConfigMap with CPU load script -apiVersion: v1 -kind: ConfigMap -metadata: - name: cpu-script - namespace: app-217 -data: - cpu-load.sh: | - #!/bin/sh - CPU_PERCENT=${CPU_PERCENT:-10} - BUSY_TIME=$(echo "scale=3; $CPU_PERCENT / 100" | bc) - IDLE_TIME=$(echo "scale=3; 1 - $BUSY_TIME" | bc) - while true; do - timeout ${BUSY_TIME}s sh -c 'while true; do echo "scale=5000; 4*a(1)" | bc -l > /dev/null; done' 2>/dev/null - sleep ${IDLE_TIME} - done ---- # 20 low-CPU pods to generate many series in prometheus
72-81:apk addat container startup adds fragility and latency.Both
payment-processorsandorder-handlersinstallbcviaapk addon every container start. If the Alpine package mirror is unavailable, all pods will fail. Consider using an image that already includesbc, or building a small custom image. For a test fixture this is acceptable, but it adds ~minutes of setup time across 15 pods and is a flaky failure point.tests/llm/fixtures/test_ask_holmes/217_prometheus_compaction_heavy_queries/test_case.yaml (1)
41-41: Barekubectl waitright after unrelated resource creation.Line 41 waits on Prometheus pods that already exist (not newly created), so this isn't as risky as waiting on just-created resources. The
|| truefallback makes it non-fatal. Still, per coding guidelines, prefer a retry loop over barekubectl waitfor robustness.Based on learnings: "Never use bare
kubectl waitimmediately after resource creation. Use retry loops to handle race conditions."
- Test 161: add toolsets.yaml (kubernetes only) to avoid Loki prerequisite failure; reduce pod resource requests so all 10 pods can schedule on KIND cluster - Test 217: replace kubectl run with kubectl exec for metric verification (avoids pod timeout); add retry loop - Add [EVAL <id>] prefix to SetupFailureError messages - Add test_id parameter to ToolsetPrerequisiteError - Annotate all exceptions with eval ID via add_note() in both test_ask_holmes.py and test_investigate.py https://claude.ai/code/session_01KagxBSbTp1AbDQTvbk1KL8 Signed-off-by: Claude <noreply@anthropic.com>
…1:49862/git/HolmesGPT/holmesgpt into claude/show-compaction-traces-fH5c2
There was a problem hiding this comment.
Actionable comments posted: 2
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/161_conversation_compaction/setup.sh (1)
231-260:⚠️ Potential issue | 🟡 MinorCPU request values for Pods 8–9 now duplicate Pods 6–7, breaking the stated "lowest to highest" ordering.
Line 11 says the pods go "from lowest to highest" CPU, but after these changes the CPU requests are: 5, 25, 50, 75, 100, 150, 200, 150, 200, 250. Pods 8 and 9 duplicate the requests of Pods 6 and 7 respectively. The comments are also misleading — Pod 8 is labeled "Very high CPU (150m)" while Pod 6 is "High CPU (150m)" with the same value.
If the test only needs 10 pods generating load for conversation compaction, this may be acceptable. But if the scenario relies on a distinct, ascending CPU profile across all pods, consider using unique request values (e.g., 250m, 300m, 350m for Pods 8–10).
Also applies to: 262-291, 293-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/161_conversation_compaction/setup.sh` around lines 231 - 260, Pods 8–9 duplicate CPU request values of earlier pods, breaking the intended ascending "lowest to highest" ordering; update the resource requests for the pods defined by metadata.name search-indexer (and the other two similar pod manifests referenced later) so all ten pods have unique, strictly increasing cpu requests (for example change Pod 8 from 150m to 250m and Pod 9 from 200m to 300m or pick another ascending sequence like 250m/300m/350m), and update the inline comment labels to match the new cpu values so the comments accurately describe each pod's CPU level.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@tests/llm/fixtures/test_ask_holmes/161_conversation_compaction/setup.sh`:
- Around line 231-260: Pods 8–9 duplicate CPU request values of earlier pods,
breaking the intended ascending "lowest to highest" ordering; update the
resource requests for the pods defined by metadata.name search-indexer (and the
other two similar pod manifests referenced later) so all ten pods have unique,
strictly increasing cpu requests (for example change Pod 8 from 150m to 250m and
Pod 9 from 200m to 300m or pick another ascending sequence like 250m/300m/350m),
and update the inline comment labels to match the new cpu values so the comments
accurately describe each pod's CPU level.
In
`@tests/llm/fixtures/test_ask_holmes/217_prometheus_compaction_heavy_queries/test_case.yaml`:
- Around line 36-91: The test uses a fixed "sleep 180" and two retry loops (the
pod readiness loop in the block that checks RUNNING_COUNT and the metrics
verification loop that checks METRICS_OUTPUT) which together can exceed the
declared setup_timeout (setup_timeout: 480); update either the setup_timeout to
~600 or reduce the 180s sleep and/or shorten loop retries so total worst‑case
time stays below setup_timeout, and replace the "kubectl wait
--for=condition=Ready pod -l app.kubernetes.io/name=prometheus -n default
--timeout=60s || true" with a non‑silent check (log a warning or fail
explicitly) so Prometheus readiness problems are surfaced before the metrics
checks run.
In `@tests/llm/utils/mock_toolset.py`:
- Around line 75-84: ToolsetPrerequisiteError's __init__ accepts test_id but no
callers pass it, so remove the dead parameter: change the constructor signature
in class ToolsetPrerequisiteError from def __init__(self, toolset_name: str,
error_detail: str, test_id: str = "") to def __init__(self, toolset_name: str,
error_detail: str), remove any logic that builds the prefix using test_id, and
construct the message using only toolset_name and error_detail before calling
super().__init__(message); keep all existing instantiations (they already omit
test_id) unchanged.
| before_test: | | ||
| # Verify Prometheus is available in the cluster | ||
| [ "$(kubectl get svc robusta-kube-prometheus-st-prometheus -n default -o jsonpath='{.spec.ports[?(@.port==9090)].port}' 2>/dev/null)" = "9090" ] \ | ||
| || { echo "❌ Prometheus Service missing or port 9090 not found"; exit 1; } | ||
|
|
||
| kubectl wait --for=condition=Ready pod -l app.kubernetes.io/name=prometheus -n default --timeout=60s || true | ||
|
|
||
| # Deploy workloads | ||
| kubectl apply -f ./manifest.yaml | ||
|
|
||
| # Wait for all pods to be running | ||
| POD_READY=false | ||
| for attempt in {1..120}; do | ||
| RUNNING_COUNT=$(kubectl get pods -n app-217 --field-selector=status.phase=Running --no-headers 2>/dev/null | wc -l) | ||
| echo "⏳ Attempt $attempt/120: $RUNNING_COUNT/35 pods running..." | ||
| if [ "$RUNNING_COUNT" -ge 35 ]; then | ||
| echo "✅ All pods are running!" | ||
| POD_READY=true | ||
| break | ||
| fi | ||
| sleep 2 | ||
| done | ||
|
|
||
| if [ "$POD_READY" = false ]; then | ||
| echo "❌ Not all pods became ready" | ||
| kubectl get pods -n app-217 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for CPU metrics to accumulate | ||
| echo "Waiting 3 minutes for metrics to accumulate..." | ||
| sleep 180 | ||
|
|
||
| # Verify metrics are available using kubectl exec on an existing pod | ||
| WORKER_POD=$(kubectl get pods -n app-217 -l app=api-worker -o jsonpath='{.items[0].metadata.name}') | ||
| METRICS_CHECK=false | ||
| for i in {1..10}; do | ||
| METRICS_OUTPUT=$(kubectl exec -n app-217 "$WORKER_POD" -- \ | ||
| wget -q -O- "http://robusta-kube-prometheus-st-prometheus.default.svc.cluster.local:9090/api/v1/query?query=count(rate(container_cpu_usage_seconds_total\{namespace=%22app-217%22\}[2m]))" 2>&1) || true | ||
|
|
||
| if echo "$METRICS_OUTPUT" | grep -q '"status":"success"'; then | ||
| if ! echo "$METRICS_OUTPUT" | grep -q '"result":\[\]'; then | ||
| echo "✅ CPU metrics available for app-217" | ||
| METRICS_CHECK=true | ||
| break | ||
| fi | ||
| fi | ||
| echo "⏳ Metrics check attempt $i/10..." | ||
| sleep 10 | ||
| done | ||
|
|
||
| if [ "$METRICS_CHECK" = false ]; then | ||
| echo "❌ No CPU metrics found for app-217 after retries" | ||
| echo "Last response: $METRICS_OUTPUT" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
setup_timeout: 480 with sleep 180 — the fixed 3-minute sleep dominates setup time.
The setup includes a hard 3-minute sleep for metrics accumulation. Combined with the pod readiness loop (up to 240s) and metrics verification loop (up to 100s), the total could approach 520s — exceeding the 480s setup_timeout. Consider either increasing setup_timeout to ~600 or reducing the sleep if feasible.
Also, line 41's kubectl wait ... || true silently continues if Prometheus isn't ready, which could lead to confusing failures during the metrics verification step. Consider logging a warning instead of silently ignoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/llm/fixtures/test_ask_holmes/217_prometheus_compaction_heavy_queries/test_case.yaml`
around lines 36 - 91, The test uses a fixed "sleep 180" and two retry loops (the
pod readiness loop in the block that checks RUNNING_COUNT and the metrics
verification loop that checks METRICS_OUTPUT) which together can exceed the
declared setup_timeout (setup_timeout: 480); update either the setup_timeout to
~600 or reduce the 180s sleep and/or shorten loop retries so total worst‑case
time stays below setup_timeout, and replace the "kubectl wait
--for=condition=Ready pod -l app.kubernetes.io/name=prometheus -n default
--timeout=60s || true" with a non‑silent check (log a warning or fail
explicitly) so Prometheus readiness problems are surfaced before the metrics
checks run.
| def __init__(self, toolset_name: str, error_detail: str, test_id: str = ""): | ||
| self.toolset_name = toolset_name | ||
| self.error_detail = error_detail | ||
| self.test_id = test_id | ||
| prefix = f"[EVAL {test_id}] " if test_id else "" | ||
| message = ( | ||
| f"Toolset '{toolset_name}' was explicitly enabled in toolsets.yaml " | ||
| f"{prefix}Toolset '{toolset_name}' was explicitly enabled in toolsets.yaml " | ||
| f"but failed prerequisites check: {error_detail}" | ||
| ) | ||
| super().__init__(message) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'ToolsetPrerequisiteError' --type=py -C2Repository: HolmesGPT/holmesgpt
Length of output: 4421
🏁 Script executed:
rg 'ToolsetPrerequisiteError\s*\(' --type=py -A2 | grep -v "^tests/llm/utils/mock_toolset.py"Repository: HolmesGPT/holmesgpt
Length of output: 397
The test_id parameter is accepted but never passed by any caller.
All ToolsetPrerequisiteError(...) instantiations across the codebase (only in this file at lines 832, 842, 871, 878, 901, 908) omit test_id, so the message prefix will always be empty. The parameter has a default value, ensuring backward compatibility, but it is dead code. Consider removing it unless it is required for future use or external integration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/llm/utils/mock_toolset.py` around lines 75 - 84,
ToolsetPrerequisiteError's __init__ accepts test_id but no callers pass it, so
remove the dead parameter: change the constructor signature in class
ToolsetPrerequisiteError from def __init__(self, toolset_name: str,
error_detail: str, test_id: str = "") to def __init__(self, toolset_name: str,
error_detail: str), remove any logic that builds the prefix using test_id, and
construct the message using only toolset_name and error_detail before calling
super().__init__(message); keep all existing instantiations (they already omit
test_id) unchanged.
…ion-traces-fH5c2 Signed-off-by: Claude <noreply@anthropic.com> # Conflicts: # holmes/core/truncation/compaction.py # holmes/core/truncation/input_context_window_limiter.py # tests/llm/test_compaction.py # tests/llm/utils/mock_toolset.py # tests/llm/utils/test_case_utils.py
…1:38850/git/HolmesGPT/holmesgpt into claude/show-compaction-traces-fH5c2
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
holmes/core/tool_calling_llm.py (1)
1009-1016: Consider addingtrace_spanparameter tocall_stream()for consistency.Unlike
call(),call_stream()does not accept or propagate atrace_spanparameter tolimit_input_context_window. The TODO at line 1150 acknowledges streaming doesn't support tracing yet. Consider creating a follow-up issue to add tracing support to the streaming path for full observability.Would you like me to open an issue to track adding tracing support to
call_stream()?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 1009 - 1016, call_stream currently doesn't accept or forward the trace_span used by call, so tracing isn't propagated into limit_input_context_window; update the call_stream signature to accept an optional trace_span parameter (matching call), pass that trace_span into the limit_input_context_window call inside call_stream, and ensure any downstream propagation (e.g., when yielding events or composing metadata) preserves/merges the trace_span info; also add a short TODO or open an issue to cover streaming-specific tracing details if further instrumentation is needed.holmes/core/truncation/input_context_window_limiter.py (1)
152-157: Add type annotation and consider module-level singleton fortrace_spandefault.The
trace_spanparameter lacks a type hint, which is required per coding guidelines. Additionally, Ruff B008 flags theDummySpan()call in the default argument—whileDummySpanis stateless and safe here, using a module-level singleton is more idiomatic.♻️ Proposed fix
Add a module-level singleton and type annotation:
from holmes.core.tracing import DummySpan +from holmes.core.tracing import DummySpan as DummySpanType # For typing from holmes.core.truncation.compaction import CompactionUsage, compact_conversation_history + +_DUMMY_SPAN = DummySpan()Then update the signature:
`@sentry_sdk.trace` def limit_input_context_window( llm: LLM, messages: list[dict], tools: Optional[list[dict[str, Any]]], - trace_span=DummySpan(), + trace_span: "DummySpanType" = _DUMMY_SPAN, ) -> ContextWindowLimiterOutput:Alternatively, if a proper
Spanprotocol/base class exists intracing.py, use that for the type annotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/truncation/input_context_window_limiter.py` around lines 152 - 157, The function limit_input_context_window has an untyped parameter trace_span and uses DummySpan() as a mutable default; add an explicit type annotation for trace_span (preferably the Span protocol/class from tracing.py if available) and replace the inline DummySpan() default with a module-level singleton (e.g., MODULE_DUMMY_SPAN = DummySpan()) then update the signature to use that singleton as the default (or use Optional[Span] and set the default to None and assign the module singleton inside the function); reference limit_input_context_window, trace_span, and DummySpan when making the change.holmes/core/truncation/compaction.py (1)
62-64: Add type annotation and consider module-level singleton fortrace_spandefault.Same issue as in
input_context_window_limiter.py: thetrace_spanparameter lacks a type hint and triggers Ruff B008.♻️ Proposed fix
from holmes.core.tracing import DummySpan + +_DUMMY_SPAN = DummySpan()def compact_conversation_history( - original_conversation_history: list[dict], llm: LLM, trace_span=DummySpan() + original_conversation_history: list[dict], llm: LLM, trace_span=_DUMMY_SPAN ) -> CompactionResult:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/truncation/compaction.py` around lines 62 - 64, The parameter trace_span on compact_conversation_history lacks a type annotation and uses a fresh DummySpan() as a default which triggers Ruff B008; add an explicit type hint (e.g., trace_span: Span or whatever tracing interface type is used across the codebase) and replace the inline DummySpan() default with a module-level singleton (e.g., SINGLETON_TRACE_SPAN = DummySpan()) or use trace_span: Optional[Span] = None and then inside compact_conversation_history set trace_span = trace_span or SINGLETON_TRACE_SPAN to avoid creating a new object at import time and satisfy the linter; reference compact_conversation_history and DummySpan when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/truncation/compaction.py`:
- Around line 85-90: The span status logging in the Compaction LLM Call block is
too lax (it logs "success" when response and response.choices are truthy) but
the actual success path checks response.choices[0] and
response.choices[0].message; update the llm_span.log call inside
trace_span.start_span (the block around trace_span.start_span/name "Compaction
LLM Call") so the output status uses the same condition as the later success
check (i.e., ensure response is truthy, response.choices has elements,
response.choices[0] exists, and response.choices[0].message is present) and log
"success" only when that full condition is met, otherwise "failure".
---
Nitpick comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 1009-1016: call_stream currently doesn't accept or forward the
trace_span used by call, so tracing isn't propagated into
limit_input_context_window; update the call_stream signature to accept an
optional trace_span parameter (matching call), pass that trace_span into the
limit_input_context_window call inside call_stream, and ensure any downstream
propagation (e.g., when yielding events or composing metadata) preserves/merges
the trace_span info; also add a short TODO or open an issue to cover
streaming-specific tracing details if further instrumentation is needed.
In `@holmes/core/truncation/compaction.py`:
- Around line 62-64: The parameter trace_span on compact_conversation_history
lacks a type annotation and uses a fresh DummySpan() as a default which triggers
Ruff B008; add an explicit type hint (e.g., trace_span: Span or whatever tracing
interface type is used across the codebase) and replace the inline DummySpan()
default with a module-level singleton (e.g., SINGLETON_TRACE_SPAN = DummySpan())
or use trace_span: Optional[Span] = None and then inside
compact_conversation_history set trace_span = trace_span or SINGLETON_TRACE_SPAN
to avoid creating a new object at import time and satisfy the linter; reference
compact_conversation_history and DummySpan when making the change.
In `@holmes/core/truncation/input_context_window_limiter.py`:
- Around line 152-157: The function limit_input_context_window has an untyped
parameter trace_span and uses DummySpan() as a mutable default; add an explicit
type annotation for trace_span (preferably the Span protocol/class from
tracing.py if available) and replace the inline DummySpan() default with a
module-level singleton (e.g., MODULE_DUMMY_SPAN = DummySpan()) then update the
signature to use that singleton as the default (or use Optional[Span] and set
the default to None and assign the module singleton inside the function);
reference limit_input_context_window, trace_span, and DummySpan when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf8b36ca-b13e-4b32-81c4-d029e40c9e98
📒 Files selected for processing (3)
holmes/core/tool_calling_llm.pyholmes/core/truncation/compaction.pyholmes/core/truncation/input_context_window_limiter.py
| with trace_span.start_span(name="Compaction LLM Call", type="llm") as llm_span: | ||
| llm_span.log(input={"message_count": len(conversation_history)}) | ||
| response: ModelResponse = llm.completion( | ||
| messages=conversation_history, drop_params=True | ||
| ) # type: ignore | ||
| llm_span.log(output={"status": "success" if response and response.choices else "failure"}) |
There was a problem hiding this comment.
Status logging may be inconsistent with actual outcome.
The span logs "success" when response and response.choices is truthy, but the actual success check (lines 96-102) is more stringent—it also verifies response.choices[0] and response.choices[0].message. If the response has choices but the first choice lacks a message, the span logs "success" while the function returns the original history (a failure).
Consider aligning the status check:
🐛 Proposed fix
response: ModelResponse = llm.completion(
messages=conversation_history, drop_params=True
) # type: ignore
- llm_span.log(output={"status": "success" if response and response.choices else "failure"})
+ has_valid_response = (
+ response
+ and response.choices
+ and response.choices[0]
+ and response.choices[0].message
+ )
+ llm_span.log(output={"status": "success" if has_valid_response else "failure"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with trace_span.start_span(name="Compaction LLM Call", type="llm") as llm_span: | |
| llm_span.log(input={"message_count": len(conversation_history)}) | |
| response: ModelResponse = llm.completion( | |
| messages=conversation_history, drop_params=True | |
| ) # type: ignore | |
| llm_span.log(output={"status": "success" if response and response.choices else "failure"}) | |
| with trace_span.start_span(name="Compaction LLM Call", type="llm") as llm_span: | |
| llm_span.log(input={"message_count": len(conversation_history)}) | |
| response: ModelResponse = llm.completion( | |
| messages=conversation_history, drop_params=True | |
| ) # type: ignore | |
| has_valid_response = ( | |
| response | |
| and response.choices | |
| and response.choices[0] | |
| and response.choices[0].message | |
| ) | |
| llm_span.log(output={"status": "success" if has_valid_response else "failure"}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/truncation/compaction.py` around lines 85 - 90, The span status
logging in the Compaction LLM Call block is too lax (it logs "success" when
response and response.choices are truthy) but the actual success path checks
response.choices[0] and response.choices[0].message; update the llm_span.log
call inside trace_span.start_span (the block around trace_span.start_span/name
"Compaction LLM Call") so the output status uses the same condition as the later
success check (i.e., ensure response is truthy, response.choices has elements,
response.choices[0] exists, and response.choices[0].message is present) and log
"success" only when that full condition is met, otherwise "failure".
This branch has not been deployed
2 participants
Summary
This PR adds distributed tracing instrumentation to the conversation history compaction feature, enabling better observability and debugging of the compaction process in production environments.
Key Changes
limit_input_context_window()with a trace span that captures input parameters (initial tokens, max context size, compaction threshold) and output metrics (compacted tokens, compression ratio, status)compact_conversation_history()with a dedicated span that logs message count and completion statustrace_spanparameter (defaulting toDummySpan()) to bothlimit_input_context_window()andcompact_conversation_history()functions to support tracingtool_calling_llm.pyto pass the trace span through tolimit_input_context_window()regressiontag and addedtest_compaction.pyto the evaluation regression test suite to ensure compaction functionality is continuously validatedtest_compaction.pyto pass the trace span tocompact_conversation_history()Implementation Details
with trace_span.start_span()) to cleanly scope trace spansDummySpandefault allows the functions to work without tracing infrastructure while maintaining backward compatibilityhttps://claude.ai/code/session_01KagxBSbTp1AbDQTvbk1KL8
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores