fix: refine proxy and timeout handling 🏓 #2
Conversation
- Improve environment variable handling for proxy and timeout settings. - Remove default timeout.
WalkthroughThe changes in the pull request update the OpenAI module in Changes
Sequence Diagram(s)sequenceDiagram
participant O as OpenAI Module
participant E as Environment
participant B as HTTP Client Builder
participant C as HTTP Client
O->>E: Retrieve OPENAI_REQUEST_TIMEOUT
alt Value provided & valid
E-->>O: Timeout value (ms)
O->>B: Set timeout(Duration::from_millis(timeout))
else No value/invalid
E-->>O: No timeout set
end
O->>B: Build HTTP Client
B-->>O: Return HTTP Client instance
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mingcheng
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/openai.rs (3)
69-69: Check for potential typo in environment variable name.The environment variable
OPENAI_APT_PROXYmight have a typo - it could be intended to beOPENAI_API_PROXYinstead. Please verify this is the correct environment variable name.
75-82: Improve timeout handling implementation.The timeout parsing implementation has several areas for improvement:
- The log message uses the original string value instead of the parsed numeric value, which could be misleading if the string contained non-numeric characters.
- There's no validation for reasonable timeout ranges.
- No feedback is provided if parsing fails.
- The variable name doesn't indicate the unit (milliseconds).
Here's a suggested implementation:
-let request_timeout = - env::var("OPENAI_REQUEST_TIMEOUT").unwrap_or_else(|_| String::from("")); -if !request_timeout.is_empty() { - if let Ok(timeout) = request_timeout.parse::<u64>() { - trace!("Setting request timeout to: {}ms", request_timeout); - http_client_builder = http_client_builder.timeout(Duration::from_millis(timeout)); - } -} +let request_timeout_ms = + env::var("OPENAI_REQUEST_TIMEOUT").unwrap_or_else(|_| String::from("")); +if !request_timeout_ms.is_empty() { + match request_timeout_ms.parse::<u64>() { + Ok(timeout_ms) => { + trace!("Setting request timeout to: {}ms", timeout_ms); + http_client_builder = http_client_builder.timeout(Duration::from_millis(timeout_ms)); + }, + Err(e) => { + debug!("Failed to parse OPENAI_REQUEST_TIMEOUT value '{}': {}", request_timeout_ms, e); + } + } +}
85-85: Consider handling potential build failures.The use of
unwrap()here could cause a panic if the HTTP client fails to build for any reason. In a production application, it would be better to handle this error case more gracefully.-let http_client = http_client_builder.build().unwrap(); +let http_client = match http_client_builder.build() { + Ok(client) => client, + Err(e) => { + debug!("Failed to build HTTP client: {}", e); + // Consider a fallback option or propagate the error + panic!("Failed to build HTTP client: {}", e); + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/openai.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Rust project - latest (nightly)
- GitHub Check: Rust project - latest (beta)
- GitHub Check: Rust project - latest (stable)
#1
Summary by CodeRabbit