Skip to content

fix(prometheus): handle explicit null timeout in instant and range query invoke - #2401

Open
jasimalam wants to merge 1 commit into
HolmesGPT:masterfrom
jasimalam:fix/prometheus-null-timeout
Open

fix(prometheus): handle explicit null timeout in instant and range query invoke#2401
jasimalam wants to merge 1 commit into
HolmesGPT:masterfrom
jasimalam:fix/prometheus-null-timeout

Conversation

@jasimalam

@jasimalam jasimalam commented Aug 17, 2026

Copy link
Copy Markdown

What's wrong

When the LLM emits "timeout": null in the tool arguments, params.get("timeout", default_timeout) still returns None the key exists so the default is never applied. The comparison None > max_timeout then raises:
TypeError: '>' not supported between instances of 'NoneType' and 'int'

This surfaces to the operator as Failed to connect to Prometheus, which is misleading since Prometheus connectivity is fine. The bug is present in both the instant query (ExecuteInstantQuery._invoke, line 1614) and range query (ExecuteRangeQuery._invoke, line 1868) paths.

Fix

Changed params.get("timeout", default_timeout) to params.get("timeout") or default_timeout in both paths so an explicit null falls through to the configured default.

Tests

Added TestNullTimeout in tests/plugins/toolsets/test_prometheus_unit.py with two cases — one for instant query and one for range query — that patch do_request and assert the timeout passed downstream equals DEFAULT_QUERY_TIMEOUT_SECONDS when "timeout": null is in the params.

Fixes #2376

Summary by CodeRabbit

  • Bug Fixes

    • Prometheus instant and range queries now consistently use the configured default timeout when no effective timeout is provided, including 0 or None.
    • Existing maximum and minimum timeout limits remain unchanged.
  • Tests

    • Added regression coverage for default timeout behavior in instant and range queries.

…ery invoke

Signed-off-by: Jasim Alam <jasimalam211@gmail.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 17, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: jasimalam / name: Jasim Alam (e73fa93)

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e8e81650-d0e9-4d37-b3b4-9dc4c745462d

📥 Commits

Reviewing files that changed from the base of the PR and between 0089f60 and e73fa93.

📒 Files selected for processing (2)
  • holmes/plugins/toolsets/prometheus/prometheus.py
  • tests/plugins/toolsets/test_prometheus_unit.py

Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.


Walkthrough

Prometheus instant and range query tools now replace falsy timeout values, including None, with the configured default. Regression tests verify the timeout passed to both mocked query requests.

Changes

Prometheus timeout fallback

Layer / File(s) Summary
Normalize query timeouts and validate null handling
holmes/plugins/toolsets/prometheus/prometheus.py, tests/plugins/toolsets/test_prometheus_unit.py
Instant and range queries now apply the default timeout when the supplied timeout is falsy. Tests verify this behavior for timeout=None.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e73fa

The PR applies the configured timeout when query arguments explicitly contain null and adds coverage for both query paths. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: avi-robusta, naomi-robusta

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the Prometheus timeout fix for instant and range query invocation.
Linked Issues check ✅ Passed The changes apply the default timeout for explicit null values in both query paths and add regression tests, satisfying issue #2376.
Out of Scope Changes check ✅ Passed The code and tests directly address the linked issue and contain no unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@netlify

netlify Bot commented Aug 17, 2026

Copy link
Copy Markdown

Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit e73fa93
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/6a82dcb13188880008d26f37
😎 Deploy Preview https://deploy-preview-2401--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prometheus toolset raises TypeError when timeout param is explicitly null

1 participant