Repository navigation
Conversation
WalkthroughThis PR adds environment-configurable LLM request timeout and TCP keepalive settings, applies the timeout to LLM completion calls, implements a utility to patch socket.create_connection with keepalive options, and conditionally applies that patch during server startup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server
participant ConnectionUtils as ConnectionUtils (patch)
participant Socket as socket.create_connection
participant Remote as Remote Host
Note over Server,ConnectionUtils: Server startup (conditional)
Server->>ConnectionUtils: call patch_socket_create_connection()
ConnectionUtils-->>Server: monkey-patches socket.create_connection
Note right of ConnectionUtils: sets SO_KEEPALIVE and platform TCP_* options
Server->>Socket: application calls socket.create_connection(...)
Socket->>ConnectionUtils: wrapped create_connection applies keepalive options
ConnectionUtils->>Socket: returns configured socket
Socket->>Remote: establish TCP connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
holmes/core/tcp_utils.py (1)
33-35: Consider reducing log verbosity.Logging at INFO level for every connection created could be verbose in production, especially with high LLM request rates. Consider using DEBUG level instead.
🔎 Proposed fix
- logging.info( + logging.debug( f"Creating patched connection to {address} with timeout {timeout} and source address {source_address}" )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d8ec062 and c059d3135760738280f16cf853356385f686e78a.
📒 Files selected for processing (4)
Dockerfileholmes/common/env_vars.pyholmes/core/llm.pyholmes/core/tcp_utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/common/env_vars.pyholmes/core/tcp_utils.pyholmes/core/llm.py
🧠 Learnings (1)
📚 Learning: 2025-12-21T13:17:48.366Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T13:17:48.366Z
Learning: Applies to holmes/core/tool_calling_llm.py : Use LiteLLM for multi-provider support (OpenAI, Anthropic, Azure, etc.) in LLM integration
Applied to files:
holmes/core/llm.py
🧬 Code graph analysis (3)
holmes/common/env_vars.py (1)
holmes/utils/cache.py (1)
get(64-74)
holmes/core/tcp_utils.py (1)
holmes/common/env_vars.py (1)
load_bool(16-21)
holmes/core/llm.py (1)
holmes/core/tcp_utils.py (1)
keepalive_create_connection(18-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build
🔇 Additional comments (4)
holmes/common/env_vars.py (1)
116-118: LGTM!The new
LLM_REQUEST_TIMEOUTconstant follows the existing pattern and provides a sensible default of 120 seconds for LLM request timeouts.holmes/core/llm.py (2)
24-24: LGTM!The imports are correctly placed at the top of the file as per coding guidelines and follow the existing import structure.
Also applies to: 35-35
416-430: Timeout parameter is properly supported in litellm. The repository uses litellm version 1.77.1, which fully supports thetimeoutparameter in the completion method (default 600s). The code correctly passes thetimeout=LLM_REQUEST_TIMEOUTparameter to handle request timeouts. No changes needed.holmes/core/tcp_utils.py (1)
1-14: LGTM!The imports, constants, and configuration setup follow coding guidelines with type hints and clear environment-based configuration.
|
Dev Docker images are ready for this commit: Use this tag to pull the image for testing. gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:90ae886
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:90ae886 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:90ae886
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:90ae886Patch Helm values in one line (choose the chart you use):
helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:90ae886
helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:90ae886 |
9a29c09 to
5d7716f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server.py (1)
5-5: Consider applying the socket patch earlier for full coverage.The socket patch is applied at lines 92-93, after many imports (lines 13-65) that could initialize HTTP client pools. If any library initializes connection pools at import time, those connections won't use the patched
socket.create_connection.For consistency with the certificate handling pattern (lines 7-9), consider applying the patch immediately after importing it:
🔎 Suggested change
from holmes.utils.cert_utils import add_custom_certificate from holmes.utils.connection_utils import patch_socket_create_connection +from holmes.common.env_vars import ENABLE_CONNECTION_KEEPALIVE as _ENABLE_KEEPALIVE ADDITIONAL_CERTIFICATE: str = os.environ.get("CERTIFICATE", "") if add_custom_certificate(ADDITIONAL_CERTIFICATE): print("added custom certificate") + +if _ENABLE_KEEPALIVE: + patch_socket_create_connection() # DO NOT ADD ANY IMPORTS OR CODE ABOVE THIS LINEThen remove the import of
ENABLE_CONNECTION_KEEPALIVEfrom line 33 and the patching block at lines 92-93.Also applies to: 91-93
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c059d3135760738280f16cf853356385f686e78a and ba1bde19947609168563fab0b8fc2218b2d03df3.
📒 Files selected for processing (4)
holmes/common/env_vars.pyholmes/core/llm.pyholmes/utils/connection_utils.pyserver.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting code
Type hints are required for all Python code
Files:
holmes/utils/connection_utils.pyholmes/common/env_vars.pyholmes/core/llm.pyserver.py
🧬 Code graph analysis (1)
server.py (1)
holmes/utils/connection_utils.py (1)
patch_socket_create_connection(7-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: llm_evals
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build
🔇 Additional comments (4)
holmes/common/env_vars.py (1)
116-122: LGTM!The new environment variables are well-structured with appropriate types and reasonable defaults. The keepalive settings (2s idle, 2s interval, 5 probes) are aggressive but appropriate for detecting dead connections quickly in cloud environments.
holmes/core/llm.py (2)
24-24: LGTM!The import is correctly placed within the existing
env_varsimport block.
415-428: LGTM!The timeout parameter is correctly passed to
litellm.completion, enabling configurable request timeouts to prevent indefinite blocking on LLM calls.holmes/utils/connection_utils.py (1)
14-31: LGTM on the socket patching implementation.The inner function correctly:
- Preserves the original
create_connectionsignature- Enables
SO_KEEPALIVEon all sockets- Uses
hasattrchecks for platform-specific TCP options (Linux-onlyTCP_KEEPIDLE,TCP_KEEPINTVL,TCP_KEEPCNT)- Provides debug logging for visibility
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
## Summary - add warning about 30-day retention for temporary dev Docker images - include commands to copy temporary images into permanent registries ## Testing - not run (not needed) ------ [Codex Task](https://chatgpt.com/codex/tasks/task_b_694a33345d548327b15a2875460fff3f) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated CI/CD workflow messages to use shorter image tags with a consistent marker prefix. * Enhanced PR/commit comment handling to update existing comments when the marker exists, avoiding duplicates. * Expanded message content with guidance that temporary images are deleted after 30 days and how to push to a permanent registry. * Added Helm patch instructions for two charts to set image tags and registries. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Codex <codex@openai.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
…s for testing nonstandard labels. (#1237) Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
…1236) Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
add default read permission to some kubernetes tools --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Checklist for the toolset: - [ ] Toolset has unit tests where relevant - [ ] Toolset has both ask_holmes and investigate evals - [ ] Toolset has a documentation PR opened against the [robusta](https://github.com/robusta-dev/robusta) repo - [ ] Toolset has the correct is_default flag - [ ] Toolset returns a correct `get_example_config` - [ ] Toolset does a live health check in addition to checking for correct configuration - [ ] Create a demo video (if relevant) similar to robusta-dev/robusta#1954 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced deployment workflow with improved container registry integration and security enhancements for artifact distribution. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: drfaust92 <ilia.lazebnik@gmail.com> Co-authored-by: arik <alon.arik@gmail.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
…sts (#1241) Signed-off-by: Tomer Keshet <tomer@robusta.dev> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a "regression" test marker to identify critical regression tests. * **Chores** * CI test gating updated: consolidated pre-check determines whether regression tests run; environment and cluster setup now conditional. * Test cluster networking enhanced for Calico and increased pod capacity. * Test tooling forced to use bash for command execution. * Several tests retagged as regression; one test timeout increased. * **Documentation** * Updated marker guidance and parallel-run/infrastructure notes. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server.py (1)
92-93: Consider adding error handling around the patch.While the current placement is appropriate (after logging setup, before config loading), adding error handling would make the server more resilient. If the keepalive patch fails, it shouldn't prevent the server from starting, since it's an optimization rather than core functionality.
🔎 Proposed defensive error handling
if ENABLE_CONNECTION_KEEPALIVE: - patch_socket_create_connection() + try: + patch_socket_create_connection() + except Exception: + logging.warning("Failed to apply connection keepalive patch", exc_info=True)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ba1bde19947609168563fab0b8fc2218b2d03df3 and 7285df79c1069ffa60d3173aa3d2a4c5c742b5bf.
📒 Files selected for processing (2)
holmes/utils/connection_utils.pyserver.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting code
Type hints are required for all Python code
Files:
holmes/utils/connection_utils.pyserver.py
🧬 Code graph analysis (1)
server.py (1)
holmes/utils/connection_utils.py (1)
patch_socket_create_connection(7-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build
🔇 Additional comments (2)
holmes/utils/connection_utils.py (1)
1-4: LGTM!Import structure follows coding guidelines with all imports at the top of the file.
server.py (1)
21-21: LGTM!Import is properly placed after the certificate setup block and follows the coding guidelines.
7285df7 to
566e65d
Compare
…er-returned-a-final-result-in-chat
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7285df79c1069ffa60d3173aa3d2a4c5c742b5bf and 90ae886.
📒 Files selected for processing (4)
holmes/common/env_vars.pyholmes/core/llm.pyholmes/utils/connection_utils.pyserver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/core/llm.py
- holmes/utils/connection_utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting code
Type hints are required for all Python code
Files:
holmes/common/env_vars.pyserver.py
🧬 Code graph analysis (1)
server.py (1)
holmes/utils/connection_utils.py (1)
patch_socket_create_connection(7-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (2)
holmes/common/env_vars.py (1)
117-117: LGTM!The LLM request timeout with a 10-minute default is reasonable for long-running LLM API calls.
server.py (1)
21-21: LGTM!The imports are correctly placed and structured.
Also applies to: 33-33
…er-returned-a-final-result-in-chat
Results of HolmesGPT evals
Legend
|
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configurable LLM request timeout (env var; default 600s). * Optional TCP keepalive support with configurable idle, interval, and probe count. * Keepalive can be enabled/disabled at runtime via an environment flag. * **Chores** * Runtime now applies keepalive patching when enabled. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mohse Morad <moshemorad12340@gmail.com> Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Codex <codex@openai.com> Signed-off-by: Roi Glinik <groi.tech@gmail.com> Signed-off-by: drfaust92 <ilia.lazebnik@gmail.com> Co-authored-by: Tomer <tomer@robusta.dev> Co-authored-by: Natan Yellin <aantn@users.noreply.github.com> Co-authored-by: Roi Glinik <groi.tech@gmail.com> Co-authored-by: Ilia Lazebnik <Ilia.lazebnik@gmail.com> Co-authored-by: arik <alon.arik@gmail.com> Signed-off-by: Filip Grebowski <grebowskifilip@gmail.com>
6 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.