Add filtering and limit parameters to list active metrics tool - #1466
Conversation
|
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR adds regex-based filtering and a result limit to the DataDog ListActiveMetrics tool, introduces ACTIVE_METRICS_DEFAULT_LIMIT = 500, implements client-side filtering, sorting, truncation and related validation/errors, and updates test fixtures to include a datadog tag. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Added limit and metric_name_filter parameters to prevent the tool from returning unbounded data that causes token overflow errors in large Datadog environments. Changes: - Added `limit` parameter (default 500) to cap number of returned metrics - Added `metric_name_filter` parameter for client-side substring filtering - Added truncation notice when results are limited - Updated one-liner to include filter and limit info Slack thread: https://robustaco.slack.com/archives/C08J4QBJ2H5/p1769151924987039?thread_ts=1769149831.651569&cid=C08J4QBJ2H5 https://claude.ai/code/session_01PRaYUq5hoWotLDre1UGMTU Signed-off-by: Claude <noreply@anthropic.com>
c107358 to
a7301be
Compare
|
✅ 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:234331c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:234331c me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:234331c
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:234331cPatch 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:234331cRobusta 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:234331c |
📂 Previous Runs📜 Run @ 9e12cf5 (#21781860436)✅ Results of HolmesGPT evalsAutomatically triggered by commit 9e12cf5 on branch Results of HolmesGPT evals
📜 Run @ 138f951 (#21779333080)✅ Results of HolmesGPT evalsAutomatically triggered by commit 138f951 on branch Results of HolmesGPT evals
|
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 32.0s | 5 | 11 | $0.2383 |
| ✅ | 101_loki_historical_logs_pod_deleted | 36.7s | 6 | 8 | $0.2261 |
| ✅ | 111_pod_names_contain_service | 34.8s | 6 | 11 | $0.2322 |
| ✅ | 112_find_pvcs_by_uuid | 40.1s | 7 | 9 | $0.2698 |
| ✅ | 12_job_crashing | 43.3s | 7 | 17 | $0.2818 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 35.3s | 5 | 15 | $0.2723 |
| ✅ | 24_misconfigured_pvc | 38.6s | 7 | 13 | $0.2436 |
| ✅ | 43_current_datetime_from_prompt | 5.6s | 1 | — | $0.0124 |
| ✅ | 61_exact_match_counting | 17.0s | 4 | 4 | $0.1596 |
| Total | 31.5s avg | 5.3 avg | 11.0 avg | $1.9362 |
📜 Run @ e1aa79b (#21769148560)
✅ Results of HolmesGPT evals
Automatically triggered by commit e1aa79b on branch claude/slack-fix-holmes-errors-M9eVu
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 33.4s | 5 | 11 | $0.2300 |
| ✅ | 101_loki_historical_logs_pod_deleted | 54.0s | 7 | 12 | $0.3086 |
| ✅ | 111_pod_names_contain_service | 34.3s | 5 | 11 | $0.2211 |
| ✅ | 112_find_pvcs_by_uuid | 37.0s | 7 | 9 | $0.2554 |
| ✅ | 12_job_crashing | 34.0s | 5 | 12 | $0.2405 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 37.7s | 5 | 15 | $0.2600 |
| ✅ | 24_misconfigured_pvc | 33.4s | 5 | 13 | $0.2290 |
| ✅ | 43_current_datetime_from_prompt | 5.0s | 1 | — | $0.1051 |
| ✅ | 61_exact_match_counting | 16.2s | 4 | 4 | $0.1589 |
| Total | 31.7s avg | 4.9 avg | 10.9 avg | $2.0084 |
📜 Run @ a09dac8 (#21744194535)
✅ Results of HolmesGPT evals
Automatically triggered by commit a09dac8 on branch claude/slack-fix-holmes-errors-M9eVu
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 34.1s | 5 | 11 | $0.2286 |
| ✅ | 101_loki_historical_logs_pod_deleted | 50.4s | 6 | 12 | $0.2873 |
| ✅ | 111_pod_names_contain_service | 32.2s | 5 | 11 | $0.2207 |
| ✅ | 112_find_pvcs_by_uuid | 33.6s | 6 | 7 | $0.2476 |
| ✅ | 12_job_crashing | 34.5s | 5 | 12 | $0.2417 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 41.5s | 6 | 16 | $0.2836 |
| ✅ | 24_misconfigured_pvc | 30.7s | 5 | 11 | $0.2117 |
| ✅ | 43_current_datetime_from_prompt | 4.9s | 1 | — | $0.1050 |
| ✅ | 61_exact_match_counting | 17.1s | 4 | 4 | $0.1599 |
| Total | 31.0s avg | 4.8 avg | 10.5 avg | $1.9862 |
✅ Results of HolmesGPT evals
Automatically triggered by commit a5301dd on branch claude/slack-fix-holmes-errors-M9eVu
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 36.2s | 6 | 11 | $0.2353 |
| ✅ | 101_loki_historical_logs_pod_deleted | 37.6s | 6 | 8 | $0.2320 |
| ✅ | 111_pod_names_contain_service | 31.6s | 5 | 11 | $0.2170 |
| ✅ | 112_find_pvcs_by_uuid | 38.2s | 7 | 8 | $0.2683 |
| ✅ | 12_job_crashing | 26.5s | 4 | 7 | $0.1960 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 50.1s | 7 | 18 | $0.3064 |
| ✅ | 24_misconfigured_pvc | 34.6s | 5 | 15 | $0.2382 |
| ✅ | 43_current_datetime_from_prompt | 5.4s | 1 | — | $0.1047 |
| ✅ | 61_exact_match_counting | 17.5s | 4 | 4 | $0.1586 |
| Total | 30.9s avg | 5.0 avg | 10.2 avg | $1.9564 |
📖 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/slack-fix-holmes-errors-M9eVu -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/slack-fix-holmes-errors-M9eVu -f markers=regression -f filter=
🔬 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 156-161: The current limit application in the metrics retrieval
block (using params.get("limit", ACTIVE_METRICS_DEFAULT_LIMIT)) lets 0 or
negative values return all metrics; change the logic in the limit handling
inside the function that processes params (the block referencing limit,
ACTIVE_METRICS_DEFAULT_LIMIT, and metrics) to enforce limit > 0 and if limit is
missing or <= 0 fall back to ACTIVE_METRICS_DEFAULT_LIMIT, then sort and slice
metrics by that positive limit; update only the limit-check branch so invalid
limits no longer produce unbounded results.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
78-82: Nit: description says "prefix or substring" but only substring matching is implemented.The
inoperator on line 145 performs substring matching, which inherently includes prefix matching. Saying "prefix or substring" might imply two distinct modes. Consider simplifying to just "substring" for accuracy.Proposed fix
"metric_name_filter": ToolParameter( - description="Filter metrics by name prefix or substring. Example: 'kubernetes' matches 'kubernetes.cpu.usage', 'system.kubernetes.memory'. Use this to narrow down large metric lists.", + description="Filter metrics by name substring (case-insensitive). Example: 'kubernetes' matches 'kubernetes.cpu.usage', 'system.kubernetes.memory'. Use this to narrow down large metric lists.", type="string", required=False, ),
|
/eval |
This comment was marked as outdated.
This comment was marked as outdated.
#1507) Switch all Datadog eval toolsets, helper scripts, and unit tests from the EU endpoint (api.datadoghq.eu) to US5 (api.us5.datadoghq.com) to match our current API keys. Remove test 93_calling_datadog which used stale mock data files that no longer match the current toolset API. https://claude.ai/code/session_011dbj3Cy5ApJHTWPnQ6Mpq3 Signed-off-by: Claude <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Switched default Datadog endpoints from EU to US5 across test configurations for metrics, logs, and traces. * **Tests** * Simplified and pruned multiple test fixtures and prompts. * Removed several Datadog conversation and log fixture files. * Added a new test scenario focused on Datadog metrics-only behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_no_k8s/test_case.yaml`:
- Line 7: The namespace "space-projects" in the bash invocation does not follow
the required app-<testid> pattern; update the first argument to "app-91a" and
make the pod names unique for this test by appending the test id (e.g., change
"spaceship-launch-counter" to "spaceship-launch-counter-91a" for both pod args
or otherwise ensure uniqueness), and update any user prompt or test metadata
that references those names to match the new "app-91a" and pod name(s).
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_no_k8s/test_case.yaml (1)
11-17: Expected output uses a generic structural pattern rather than a query-specific value.The
expected_outputchecks for the embed format but doesn't verify any specific metric data that can only be discovered by actually querying Datadog (e.g., a specific metric value, time range, or count). Consider adding a check for a discoverable value like the metric namecontainer.cpu.usagein the output to strengthen the assertion.As per coding guidelines, "For LLM eval tests, use specific values in expected_output that can only be discovered by querying (like unique IDs, injected error codes, exact counts) rather than generic output patterns."
Tagged 13 Datadog-related eval tests with 'regression' marker so they run automatically in CI/CD regression testing. https://claude.ai/code/session_01PRaYUq5hoWotLDre1UGMTU Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
|
/eval |
This comment was marked as outdated.
This comment was marked as outdated.
…metric filter - Remove regression tag from all Datadog eval test cases (per review feedback) - Change metric_name_filter from substring match to regex pattern matching - Add error handling for invalid regex patterns https://claude.ai/code/session_01PRaYUq5hoWotLDre1UGMTU Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 157-162: Replace the inconsistent use of the data= field for error
messages with error= so error paths are uniform: in the block that returns a
StructuredToolResult when no metrics match (the if not metrics branch) change
the returned StructuredToolResult to set error= with the existing message
instead of data=; keep the same status=StructuredToolResultStatus.ERROR and
params=params and retain the message text referencing metric_name_filter so it
matches the regex error path that already uses error=.
When limit=0 or negative, now falls back to ACTIVE_METRICS_DEFAULT_LIMIT instead of returning all metrics unbounded. https://claude.ai/code/session_01PRaYUq5hoWotLDre1UGMTU Signed-off-by: Claude <noreply@anthropic.com>
|
/eval |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 256-260: get_parameterized_one_liner currently reads the raw limit
from params and can show invalid values (e.g., 0 or negative) even though
_invoke falls back to ACTIVE_METRICS_DEFAULT_LIMIT; modify
get_parameterized_one_liner to validate/coerce the limit the same way _invoke
does (ensure limit is a positive integer, otherwise use
ACTIVE_METRICS_DEFAULT_LIMIT) before building the one-liner string for
toolset_name_for_one_liner(self.toolset.name): List Active Metrics (...,
limit={limit}).
🧹 Nitpick comments (1)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
142-153: Consider adding a timeout or complexity guard for user-supplied regex.The regex pattern is compiled directly from
metric_name_filterwithout any protection against catastrophic backtracking (ReDoS). While the input comes from the LLM rather than a direct end-user, a complex pattern like(a+)+$applied against a large metric list could cause significant CPU usage.A simple mitigation would be to use
re.searchwith a timeout (Python 3.11+ has no native timeout forre, but you could use theregexlibrary), or fall back to simple substring matching and only use regex when special characters are detected.Given that the LLM is the source of these patterns, this is low risk but worth noting.
|
@aantn Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
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: |
3 participants
Summary
Enhanced the
ListActiveMetricstool in the Datadog metrics toolset with client-side filtering and result limiting capabilities to improve usability when dealing with large metric lists.Key Changes
ACTIVE_METRICS_DEFAULT_LIMITconstant (500) to define default result limitmetric_name_filterparameter to filter metrics by name prefix or substring (case-insensitive)limitparameter to control maximum number of returned metricsget_parameterized_one_liner()to include filter and limit information in the summaryImplementation Details
https://claude.ai/code/session_01PRaYUq5hoWotLDre1UGMTU
Summary by CodeRabbit
New Features
Tests