Repository navigation
coralogix fixes - #1469
Closed
aantn wants to merge 51 commits into
Closed
coralogix fixes#1469aantn wants to merge 51 commits into
aantn wants to merge 51 commits into
Conversation
The CoralogixLabelsConfig and related flatten functions were never actually used - the API returns raw JSON that is cleaned up directly by _cleanup_coralogix_results in api.py without any label-based flattening. Removed: - FlattenedLog, CoralogixQueryResult, CoralogixLabelsConfig classes - labels field from CoralogixConfig - extract_field, flatten_structured_log_entries, stringify_flattened_logs, parse_json_objects functions - Related tests for extract_field - Documentation reference to optional labels config Signed-off-by: Claude <noreply@anthropic.com>
Replace the slow Kubernetes-based Coralogix evals (15+ min setup) with a fast cloud-only approach that writes logs directly via REST API. Changes: - Replace 173_coralogix_logs: Now sends logs via REST API to Coralogix ingestion endpoint, no K8s required (~5 min setup including indexing) - Delete 174_coralogix_traces_ad and 175_coralogix_metrics_frontend: These required complex K8s+OTLP setup. Can be re-added later using OTLP HTTP endpoints if needed. - Add shared/coralogix/coralogix_test_utils.sh: Utilities for sending logs and querying Coralogix DataPrime API - Update conftest.py: Allow all Coralogix domains (not just eu2) - Delete unused coralogix_app.py and traffic_generator.py The new 173 test uses anti-hallucination error codes (ERR-7291, ERR-4058, ERR-9463) that the LLM can only find by actually querying Coralogix. Signed-off-by: Claude <noreply@anthropic.com>
Add Python-based scripts and eval tests for Coralogix traces and metrics that send data directly via OTLP gRPC (no K8s required): - 174_coralogix_traces: Sends traces via OTLP, queries via DataPrime - Uses send_traces.py with OpenTelemetry SDK - Injects ERR-5847 error code for anti-hallucination - 175_coralogix_metrics: Sends metrics via OTLP, queries via Prometheus API - Uses send_metrics.py with OpenTelemetry SDK - Creates eval175_* prefixed metrics for anti-hallucination Both tests complete in ~5 minutes (vs 15+ min for old K8s-based tests). Signed-off-by: Claude <noreply@anthropic.com>
Coralogix requires different API keys for sending data (SendData permissions) vs querying data (DataQuerying permissions). This change introduces CORALOGIX_SEND_API_KEY for data ingestion in eval tests, while keeping CORALOGIX_API_KEY for queries (used by Holmes toolset). Changes: - coralogix_test_utils.sh: validate CORALOGIX_SEND_API_KEY, use it in cx_send_logs() - send_traces.py: default to CORALOGIX_SEND_API_KEY env var - send_metrics.py: default to CORALOGIX_SEND_API_KEY env var - test_case.yaml files: update requirements documentation https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The team_hostname config field is now optional - it's only needed to generate
clickable UI permalink URLs in tool output (e.g., https://{team_hostname}.{domain}/#/query-new/...).
Core API functionality (querying logs/traces/metrics) only requires:
- api_key: with DataQuerying permissions
- domain: e.g., "eu2.coralogix.com"
This simplifies setup from 4 env vars to 3 for Coralogix evals.
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
The field represents the team's URL slug (e.g., "my-team" from https://my-team.eu2.coralogix.com), not a hostname. Renamed for clarity. Backwards compatible: existing configs using team_hostname still work via Pydantic model validator that maps old field name to new one. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The curl -sf flag combined with set -e caused the before_test script to exit early when curl got no response. Changed to curl -s with || true to allow the retry loop to work correctly. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The Coralogix REST API /singles endpoint expects an ARRAY of log entries
where each entry includes applicationName and subsystemName, not a wrapper
object with logEntries field.
Wrong format (was causing "Dropped" errors):
{"applicationName":"...", "subsystemName":"...", "logEntries":[...]}
Correct format:
[{"applicationName":"...", "subsystemName":"...", "severity":3, "text":"..."}]
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
- Use DataPrime filter for applicationname (metadata field) instead
of Lucene AND - applicationname is in labels, not searchable text
- Add -k flag to all curl commands for SSL bypass in sandbox
environments with TLS interception proxies
- Update shared utilities with consistent SSL handling
Query format changed from:
lucene 'holmes-eval-173 AND ERR-7291'
To:
filter $l.applicationname == 'holmes-eval-173' | lucene 'ERR-7291'
Tested manually:
- Log ingestion works ({"message":"OK"})
- Logs are indexed within ~60 seconds
- Query with filter syntax returns expected results
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
Merge conflicts resolved: - utils.py: Combined new CoralogixConfig (with team_slug, backwards compat) with master's new classes (FlattenedLog, CoralogixQueryResult, CoralogixLabelsConfig) - toolset_coralogix.py: Kept get_example_config method from feature branch https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The Coralogix evals require three environment variables: - CORALOGIX_API_KEY (query) - CORALOGIX_SEND_API_KEY (ingestion) - CORALOGIX_DOMAIN (e.g., eu2.coralogix.com) The domain was missing from the workflow. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Instead of requiring CORALOGIX_DOMAIN as an environment variable, hardcode it directly in the test configurations and utilities. This simplifies the CI/CD setup - only two secrets needed: - CORALOGIX_API_KEY (query) - CORALOGIX_SEND_API_KEY (ingestion) https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The send_traces.py and send_metrics.py scripts require OpenTelemetry packages (opentelemetry-api, opentelemetry-sdk, opentelemetry-exporter-otlp-proto-grpc) to send test data to Coralogix. These were missing from dev dependencies, causing setup failures in CI/CD. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The verification queries were using `filter $l.applicationname` which may not work because: 1. Field names in Coralogix can be case-sensitive 2. The field path may differ from what was assumed 3. Per Coralogix best practices, prefer lucene over filter for text search Changes: - Test 173 (logs): Use `lucene 'app AND error_code'` instead of filter - Test 174 (traces): Same fix for spans queries - Test 175 (metrics): Add debug output (already uses Prometheus API) - Add debug output to print first query response for troubleshooting - Better error checking for log ingestion response https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The DataPrime API may use a default time range that excludes recently ingested logs. Adding explicit startDate and endDate (1 hour window) ensures we query the correct time range where our test data exists. Changes: - Test 173 (logs): Add startDate/endDate to all queries - Test 174 (traces): Add startDate/endDate to queries - Print debug output every 10 attempts instead of just the first https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Remove unused CoralogixQueryResult and FlattenedLog classes from utils.py - Fix variable shadowing in send_metrics.py (endpoint -> api_endpoint) - Remove unused timestamp variables TS1-TS6 in test 173 - Fix SC2155 shell violations in coralogix_test_utils.sh (separate declaration from assignment, handle /dev/urandom errors) - Add return type hint to get_example_config() method - Update test fixtures to use team_slug instead of deprecated team_hostname https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The function now fails on non-2xx HTTP responses (401, 403, 500, etc.) using curl's -f flag, in addition to checking for "Dropped" entries. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Replace hint-giving resource names with neutral business names:
- Test 173: holmes-eval-173 -> order-service-3k9p
- Test 174: holmes-eval-174 -> checkout-flow-8m2n
- Test 175: holmes-eval-175 -> catalog-service-7x4m, eval175 -> ctlgsvc7x4m
- coralogix_test_utils.sh improvements:
- Add jq requirement check and documentation
- Use jq for JSON escaping in cx_query to handle special characters
- Add cx_prometheus_url helper function
- Test structure improvements:
- Move set -e before source to ensure failures abort immediately
- Use helper functions (cx_ingress_url, cx_query_url, cx_prometheus_url)
instead of hardcoded URLs
- Make expected_output more specific with exact error codes and messages
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
The base Toolset class now provides get_config_example() which automatically derives examples from config_classes using build_config_example(). Since CoralogixConfig already has examples defined in its Field definitions, the custom method is redundant. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Root cause: The lucene search in DataPrime only searches the log TEXT body, not label fields like applicationname. The previous queries used `lucene 'app-name AND error-code'` but the app name is stored in $l.applicationname label, not in the text. Fix: Use `filter $l.applicationname == 'app' | lucene 'error-code'` to filter by application label AND search text for the error code. Also use jq to build JSON payloads for proper escaping of special characters like $l.applicationname. Tested locally with actual Coralogix credentials - queries now return results correctly. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
These tests only need the Coralogix toolset. Explicitly disable kubernetes/core, kubernetes/logs, and helm/core to prevent failures when those tools aren't installed in the test environment. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Updated the Coralogix jinja2 instructions to clarify: - lucene searches TEXT BODY only (log message content) - filter is needed for LABEL FIELDS (applicationname, subsystemname) - Application name must always be filtered with: filter $l.applicationname == 'name' Also added CLAUDE.md guidance to test evals with Opus 4.5 via OpenRouter. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Fixed gRPC metadata keys to be lowercase (Authorization -> authorization) gRPC requires lowercase metadata keys, causing "Illegal header key" errors - Fixed traces query: spans don't have $l.applicationname like logs Use lucene search for error code instead (unique to test) https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Test 173 (logs): Use lucene-only query (remove $l.applicationname filter) - Test 174 (traces): Replace OTLP gRPC with REST API log ingestion - Test 175 (metrics): Replace OTLP gRPC with REST API log ingestion The OTLP gRPC exporters fail with SSL certificate verification errors in sandbox environments with TLS interception. Using REST API with curl -k bypasses this issue. All three tests now pass setup reliably. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Required for Prometheus RemoteWrite compression in test 175. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Renamed tests to avoid conflicts with master's K8s-based Coralogix tests: - 173_coralogix_logs -> 211_coralogix_logs_fast - 174_coralogix_traces -> 212_coralogix_traces_fast - 175_coralogix_metrics -> 213_coralogix_metrics_fast Added new 'fast' marker for cloud-only tests that don't require Kubernetes infrastructure. Updated metric prefix to eval213_ for test isolation. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Coralogix log indexing can take longer in CI/CD environments. Increased setup_timeout from 300s to 600s and improved debug output to help diagnose query response issues. Also increased curl max-time to 60s and polling iterations to 120 to handle slower Coralogix indexing. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Restore the original Kubernetes-based Coralogix eval tests (173, 174, 175) that use pods to generate telemetry. These coexist with the new fast cloud-only tests (211, 212, 213). Test summary: - 173_coralogix_logs: K8s pods send logs, tests log search - 174_coralogix_traces_ad: K8s pods send traces, tests span search - 175_coralogix_metrics_frontend: K8s pods send metrics, tests PromQL The K8s tests require: CORALOGIX_API_KEY, CORALOGIX_DOMAIN, CORALOGIX_TEAM_HOSTNAME The fast tests require: CORALOGIX_SEND_API_KEY, CORALOGIX_API_KEY https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
This commit adds a test framework feature that generates a unique run ID
for each test execution, preventing tests from succeeding on cached data
from previous runs.
Changes:
- Generate 8-character hex EVAL_RUN_ID before before_test runs
- Make EVAL_RUN_ID available as env var in before_test scripts
- Add {{ env.VAR_NAME }} templating support for user_prompt
- Store run_id on test_case for use during test execution
Updated fast Coralogix tests (211, 212, 213):
- Use EVAL_RUN_ID in app names/service names/metric prefixes
- Add HTTP status code validation for write operations
- Add better error messages explaining possible failures
- Simplify DataPrime queries to use lucene search
This ensures each test run creates unique test data that can only be
found by actually querying the Coralogix API, ruling out hallucinations
and cache hits.
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
- Test 211 (logs): Change from lucene 'ERR-7291' to filter text.contains() because Coralogix logs require explicit field specification for lucene queries - Test 212 (traces): Update comment to clarify lucene works for spans via full-text search across all fields The logs source requires filter text.contains() or lucene 'text:ERR-7291' while spans support plain lucene 'ERR-5847' for full-text search. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Increase setup_timeout from 600s to 900s (15 minutes) for tests 211-213 - Increase retry loop iterations from 120 to 180 to match the timeout - Add CORALOGIX_DOMAIN and CORALOGIX_TEAM_HOSTNAME env vars to CI/CD workflow for K8s-based Coralogix tests (173, 174, 175) Coralogix indexing can take >10 minutes in some cases, and there's no API to force refresh. The increased timeout should handle most delays. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
- Remove 'fast' tag from tests 211-213 (no longer fast due to indexing delays) - Hardcode domain to "eu2.coralogix.com" in tests 173-175 (K8s tests) - Remove CORALOGIX_DOMAIN and CORALOGIX_TEAM_HOSTNAME from CI/CD workflow - Simplify env var validation in K8s tests (only CORALOGIX_API_KEY needed) All Coralogix tests now use consistent configuration with hardcoded domain. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The text field is inside userData JSON string, not a top-level field. lucene does full-text search across all fields including the userData JSON content, so 'lucene ERR-7291' works while 'filter text.contains()' fails with "keypath does not exist: text". https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Resolved conflict in coralogix/utils.py: - Keep ToolsetConfig base class from master - Keep ConfigDict(extra="allow") pattern for backwards compatibility - Keep team_hostname -> team_slug migration with deprecation warning - Add FlattenedLog and CoralogixQueryResult types from master https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
These files were deleted in 8436422 when replacing K8s tests with cloud-only tests, but tests 173-175 still reference them. Restoring so CI/CD can run those tests. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
|
Collaborator
Author
|
/eval |
Contributor
📂 Previous Runs📜 Run @ 59dea29 (#21631400430)✅ Results of HolmesGPT evalsAutomatically triggered by commit 59dea29 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/coralogix-key-separation-ipGKM' Status: Success - 83 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 8b4375c (#21631162111)✅ Results of HolmesGPT evalsAutomatically triggered by commit 8b4375c on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/coralogix-key-separation-ipGKM' Status: Success - 82 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 375da98 (#21630343556)✅ Results of HolmesGPT evalsAutomatically triggered by commit 375da98 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/coralogix-key-separation-ipGKM' Status: Success - 58 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 41f1649 (#21628874554)✅ Results of HolmesGPT evalsAutomatically triggered by commit 41f1649 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/coralogix-key-separation-ipGKM' Status: Success - 47 test/model combinations loaded Experiments compared (30):
Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit f3a56f2 on branch Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
Contributor
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3d79623
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3d79623 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3d79623
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3d79623Patch 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:3d79623Robusta 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:3d79623 |
Contributor
WalkthroughRenames Coralogix config field to team_slug with migration, adds per-run EVAL_RUN_ID and prompt rendering, introduces Coralogix test tooling (bash + Python senders) and cloud-only fixtures pointing to eu2.coralogix.com, expands passthroughs, adds OpenTelemetry/snappy dev deps, and adds CLAUDE OpenRouter Opus 4.5 examples. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant Sender as Sender (send_traces.py / send_metrics.py)
participant Ingest as Coralogix Ingress/API
participant Query as Coralogix DataPrime / Prometheus API
TR->>Sender: invoke send_traces/send_metrics (EVAL_RUN_ID, API keys)
Sender->>Ingest: POST OTLP / RemoteWrite (Auth headers + CX metadata)
Ingest-->>Sender: 200 OK (ingestion ack)
Note right of Ingest: Indexing/processing window
TR->>Query: poll DataPrime / Prometheus with lucene/PromQL (Auth)
Query-->>TR: query results (indexed spans/metrics)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 199-200: The docs in CLAUDE.md set MODEL and CLASSIFIER_MODEL
environment variables (MODEL and CLASSIFIER_MODEL) to an Anthropic Claude model
for MODEL but to openrouter/openai/gpt-4.1 for CLASSIFIER_MODEL, which violates
the Claude-only guideline; either (A) update the doc to explicitly state and
justify the exception (explain "Anthropic not supported as classifier" and why)
and link to the coding guideline, or (B) switch CLASSIFIER_MODEL to a
Claude-compatible classifier (e.g., claude-sonnet-4.5) and add a compatibility
note and testing instructions; update the single-line example and add one
sentence clarifying which option is used and why so readers know whether the
exception is intentional or a real incompatibility.
In `@tests/llm/fixtures/shared/coralogix/send_traces.py`:
- Around line 19-38: The imports for OpenTelemetry and urllib3 currently inside
send_traces() must be hoisted to module scope: add a top-of-file try/except that
imports opentelemetry.trace, OTLPSpanExporter, Resource, TracerProvider,
BatchSpanProcessor, Status/StatusCode and urllib3 (or urllib3.disable_warnings)
and sets a flag or exposes None if missing, then update send_traces() to use
those module-level symbols (and early-return False if the imports failed);
reference the send_traces function and the imported names (OTLPSpanExporter,
TracerProvider, BatchSpanProcessor, Resource, Status, StatusCode, urllib3) so
you locate the code to change.
In `@tests/llm/fixtures/test_ask_holmes/173_coralogix_logs/test_case.yaml`:
- Line 20: Update the echo setup message so it references the correct test
number: change the string literal in the setup command that currently reads "🚀
Setting up test 163 - Creating namespace app-173" to "🚀 Setting up test 173 -
Creating namespace app-173" to match the namespace and avoid confusion (look for
the echo invocation in the test setup).
In `@tests/llm/fixtures/test_ask_holmes/174_coralogix_traces_ad/test_case.yaml`:
- Line 17: Add validation for the CORALOGIX_DOMAIN environment variable
alongside the existing CORALOGIX_API_KEY check so the curl URL using
${CORALOGIX_DOMAIN} isn't malformed; update the test fixture's pre-check to
ensure CORALOGIX_DOMAIN is set (or, if the domain is fixed per toolsets.yaml,
replace occurrences of ${CORALOGIX_DOMAIN} in the curl/URL code with the
concrete host eu2.coralogix.com) so the trace query URL is always valid.
In `@tests/llm/fixtures/test_ask_holmes/211_coralogix_logs_fast/test_case.yaml`:
- Around line 62-71: The test currently writes curl output to a shared file
/tmp/cx_response.txt causing parallel-test collisions; change the HTTP call flow
(the curl invocation that sets HTTP_CODE and the subsequent RESPONSE assignment)
to use a unique temporary file (e.g., created with mktemp) instead of
/tmp/cx_response.txt, read RESPONSE from that temp file, and ensure the temp
file is deleted after use so INGRESS_URL / HTTP_CODE / RESPONSE logic remains
the same but safe for parallel runs.
In `@tests/llm/fixtures/test_ask_holmes/211_coralogix_logs_fast/toolsets.yaml`:
- Around line 1-2: Update the header comment to reference the correct test id
211 instead of 173: edit the top comment lines in this YAML (the two comment
lines shown) so the first line reads "# Coralogix toolset configuration for test
211" (and keep the second line as appropriate), ensuring the header matches the
file's 211_* filename.
- Around line 7-9: The api_key template in the toolset config is using
"{{env.CORALOGIX_API_KEY}}" which won’t be recognized by our env templating;
update the value for the api_key under the config block to use the spaced env
template "{{ env.CORALOGIX_API_KEY }}" so the environment variable resolves
correctly, ensuring the toolset config (config: api_key) follows the expected
'config' field format that supports '{{ env.VARIABLE_NAME }}' references.
In `@tests/llm/fixtures/test_ask_holmes/212_coralogix_traces_fast/test_case.yaml`:
- Line 84: The echo message uses $((i * 5)) which is misleading because i starts
at 1; update the log to reflect actual elapsed wait time by computing elapsed as
$(((i - 1) * 5)) or by introducing an explicit elapsed variable incremented each
loop, and replace the echo in the test (the line containing echo "✅ Spans are
queryable after $((i * 5)) seconds") to use that corrected expression so the
reported seconds match the real wait time.
- Around line 50-54: The exit-status check block (the "if [ $? -ne 0 ]" ...
"exit 1") is dead code because the script enables "set -e" earlier; remove this
unreachable block to clean up the script or alternatively remove "set -e" if you
want explicit error handling—so either delete the if/fi block following the
python3 send traces invocation or add explicit error-capturing around that
python3 call (e.g., capture its exit code into a variable and handle it) to make
the check reachable; refer to the "set -e" flag and the "python3" send-traces
invocation when locating the code to change.
In `@tests/llm/fixtures/test_ask_holmes/212_coralogix_traces_fast/toolsets.yaml`:
- Around line 1-2: Update the header comment that currently reads "Coralogix
toolset configuration for test 174" to reference the correct test id "212" so
the comment matches the file's test identifier; specifically change the header
line in the toolset YAML from "test 174" to "test 212" (the comment at the top
of tests/llm/fixtures/test_ask_holmes/212_coralogix_traces_fast/toolsets.yaml).
- Around line 7-9: The config block's api_key uses the non-resolving template
"{{env.CORALOGIX_API_KEY}}"; update the template to use the supported env
reference format with spaces, e.g. "{{ env.CORALOGIX_API_KEY }}", so the api_key
under config resolves at runtime; ensure the toolset's config field (config,
api_key, domain) follows the '{{ env.VARIABLE_NAME }}' convention consistently.
In
`@tests/llm/fixtures/test_ask_holmes/213_coralogix_metrics_fast/test_case.yaml`:
- Around line 51-55: Remove the unreachable exit check that follows the metrics
send because the script uses set -e (referenced on line with "set -e"), so
failures already abort; specifically delete the if block that tests the previous
command's exit status (the block printing "❌ Failed to send metrics" and
exiting) or replace it with a more appropriate trap/ERR handler if you need
custom error messaging; ensure you remove or refactor the block that begins with
"if [ $? -ne 0 ]" so no dead code remains.
In `@tests/llm/fixtures/test_ask_holmes/213_coralogix_metrics_fast/toolsets.yaml`:
- Around line 13-16: The Authorization header template lacks the required
spacing around the env token so the variable won't be interpolated; update the
headers Authorization value to use the spaced template format (e.g., "{{
env.CORALOGIX_API_KEY }}") so the env.CORALOGIX_API_KEY is substituted correctly
when loading the toolset config (look for the prometheus_url/headers block and
the Authorization key).
🧹 Nitpick comments (7)
tests/llm/fixtures/shared/coralogix/send_traces.py (1)
108-138: Add an explicit return type formain().This keeps the script aligned with the project’s typing requirements.
✏️ Suggested tweak
-def main(): +def main() -> None:As per coding guidelines "Use type hints throughout Python code and run 'mypy' for type checking".
tests/llm/fixtures/shared/coralogix/coralogix_test_utils.sh (1)
18-22: Consider deferringjqcheck to function invocation.The
jqavailability check runs at source time, causing immediate script exit ifjqis missing. This may be unexpected behavior when a test sources this file but doesn't use functions that requirejq. Consider moving this check into functions that actually needjq(likecx_query), or intocx_setup().♻️ Alternative: Move check into cx_setup or cx_query
-# Check for required tools -if ! command -v jq &> /dev/null; then - echo "❌ jq is required but not installed. Please install jq." - exit 1 -fi +# Check for required tools (called by functions that need jq) +_cx_require_jq() { + if ! command -v jq &> /dev/null; then + echo "❌ jq is required but not installed. Please install jq." + exit 1 + fi +}Then call
_cx_require_jqincx_queryandcx_send_logswherejqis actually used.tests/llm/fixtures/test_ask_holmes/175_coralogix_metrics_frontend/test_case.yaml (1)
47-57: Reduce readiness retry intervals/timeouts to keep evals fast.Current loops wait up to 780s with 5s sleeps. Guidelines call for ~60s readiness with 1s sleeps.
⚡ Proposed adjustment (apply similarly to both loops)
- echo "⏳ Waiting for frontend-service pod to be ready (timeout 780s)" + echo "⏳ Waiting for frontend-service pod to be ready (timeout 60s)" FRONTEND_POD_READY=false - for i in {1..156}; do + for i in {1..60}; do if kubectl wait --for=condition=ready pod -l app=frontend-service -n app-175 --timeout=5s 2>/dev/null; then echo "✅ frontend-service pod is ready!" FRONTEND_POD_READY=true break else - echo "⏳ Attempt $i/156: frontend-service pod not ready yet, waiting 5s..." - sleep 5 + echo "⏳ Attempt $i/60: frontend-service pod not ready yet, waiting 1s..." + sleep 1 fi doneAs per coding guidelines: Use
sleep 1instead ofsleep 5for most retry loops and reduce readiness timeouts to ~60s.Also applies to: 82-92
tests/llm/utils/test_case_utils.py (2)
549-556: Side effect: Mutates globalos.environ.Setting
os.environ["EVAL_RUN_ID"](line 556) has process-wide side effects. While this is documented as intentional, consider whether this could cause issues ifrender_user_promptis called multiple times with different test cases, or in parallel test execution scenarios.Also, per Python best practices (TRY004), use
TypeErrorfor invalid type errors.♻️ Proposed fix for exception type
if not isinstance(prompt, str): # For list prompts, this should be the specific variant - raise ValueError(f"Expected string user_prompt, got {type(prompt)}") + raise TypeError(f"Expected string user_prompt, got {type(prompt).__name__}")
559-564: Exception handling may silently hide templating errors.The broad
except ValueErrorcatch returns the original prompt whenget_env_replacementfails. However, looking at theget_env_replacementimplementation inholmes/utils/env.py, it raisesValueErrorspecifically when an environment variable is missing. Returning the original prompt in this case could lead to tests running with unrendered{{ env.VAR }}patterns, causing silent failures.Consider being more explicit about which failures should fall through:
♻️ Proposed fix for clearer error handling
# Apply env var templating using the same mechanism as toolset config try: rendered = get_env_replacement(prompt) - return rendered if rendered else prompt + if rendered is not None: + return rendered + return prompt except ValueError: - # If env var not found but no {{ env. }} patterns, return original + # Re-raise if prompt contains env patterns - this is a real error + if "{{ env." in prompt: + raise return prompttests/llm/fixtures/shared/coralogix/send_metrics.py (2)
29-77: Custom protobuf encoding works but is maintenance-sensitive.The manual protobuf encoding implementation is correct for the simple RemoteWrite schema, but it's tightly coupled to the exact field numbers and structure. This avoids adding a dependency on
protobuflibrary, which is a reasonable trade-off for a test utility.Consider adding a comment noting the Prometheus RemoteWrite protobuf schema version this targets, so future maintainers know what spec to reference if changes are needed.
84-85: Global warning suppression affects entire process.
urllib3.disable_warnings()disables SSL warnings globally for the entire Python process, which could mask legitimate SSL issues in other parts of the test infrastructure. Consider scoping this more narrowly or accepting this as acceptable for test utilities.
Contributor
|
@aantn Your eval run has finished. 🧪 Manual Eval Results
Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'master' Status: Success - 38 test/model combinations loaded Experiments compared (30):
Comparison indicators:
|
| 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/coralogix-key-separation-ipGKM -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
markers: regression
Or with more options (one per line):
/eval
model: gpt-4o
markers: regression
filter: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
markers: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
markers |
Pytest 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"
🏷️ Valid markers
benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, 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/coralogix-key-separation-ipGKM -f markers=regression -f filter=
K8s tests 173-175: - Update coralogix_app.py to send OTLP directly to Coralogix ingress (removes dependency on local OTLP collector) - Add CORALOGIX_DOMAIN and CORALOGIX_SEND_API_KEY env vars to deployments - Use envsubst to substitute API key in yaml before kubectl apply - Fix test 173 setup message (was "163", now "173") CodeRabbit fixes: - Test 211: Use mktemp instead of shared /tmp/cx_response.txt for parallel test safety - send_traces.py: Hoist OpenTelemetry imports to module scope https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Replace ${CORALOGIX_DOMAIN} with hardcoded domain in the trace
verification curl command to prevent malformed URLs.
https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n
Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/llm/fixtures/test_ask_holmes/174_coralogix_traces_ad/test_case.yaml (1)
104-105:⚠️ Potential issue | 🟡 MinorRemove unused
TRAFFIC_READYvariable.The
TRAFFIC_READYvariable is declared but never used. The script usesTRACES_READYfor the actual polling logic. This appears to be dead code.🧹 Proposed fix to remove dead code
echo "⏰ Waiting for ad traffic to produce traces..." - TRAFFIC_READY=false echo "⏳ Waiting for at least 10 traces in Coralogix (poll every 5s)"
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/174_coralogix_traces_ad/test_case.yaml (1)
4-6: Consider using more specific, discoverable values in expected_output.The expected output mentions generic concepts like "long-duration trace" and "database slowness/connection issues." These patterns could potentially be hallucinated by an LLM without actually querying the traces. Per coding guidelines, expected_output should use specific, discoverable values (e.g., exact span names, specific latency values, or unique identifiers from the trace data) to rule out hallucinations.
If specific values aren't practical here, consider adding
include_tool_calls: trueto verify the LLM actually invoked the Coralogix tracing tools. As per coding guidelines: "Use 'include_tool_calls: true' in test configuration only when expected output values are too generic to be hallucination-proof."
- Test 211: Change "test 173" -> "test 211" - Test 212: Change "test 174" -> "test 212" https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
The `if [ $? -ne 0 ]` checks after python3 calls are unreachable because `set -e` causes the script to exit immediately on failure. https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Collaborator
Author
|
/eval |
Resolved conflict by using master's keyword argument style while keeping the rendered_prompt from HEAD (for env var templating). https://claude.ai/code/session_01LBDt5463hB3g3aUr5mo38n Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
@aantn Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'master' Status: Success - 54 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Bug Fixes
New Features / Tests
Chores