Refactor HTTP clients to use shared utilities and proper error handling - #168
Refactor HTTP clients to use shared utilities and proper error handling#168gshikhar2021 wants to merge 1 commit into
Conversation
GitHub Actions
Failure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a common HTTP utility package to standardize request handling and error management across the codebase, refactoring the Docling and embedding clients to use these shared utilities. Feedback focuses on improving the robustness of the new HTTP package, including fixing a logic error in authorization header construction, handling nil clients, supporting the full 2xx success range, and truncating large error bodies in logs. Additionally, suggestions were made to avoid blocking reconciliation loops with sleeps and to correct JSON tags on internal client fields.
| if apiKey != "" && authFormat != "" { | ||
| req.Header.Set("Authorization", fmt.Sprintf("%s %s", authFormat, apiKey)) | ||
| } |
There was a problem hiding this comment.
The current logic prevents the Authorization header from being set if authFormat is empty. This breaks the retry mechanism in the docling client, which attempts to send the API key without a prefix (e.g., for services that do not expect 'Bearer').
if apiKey != "" {
authHeader := apiKey
if authFormat != "" {
authHeader = fmt.Sprintf("%s %s", authFormat, apiKey)
}
req.Header.Set("Authorization", authHeader)
}| func Do(ctx context.Context, client *http.Client, req *http.Request) (int, []byte, error) { | ||
| logger := log.FromContext(ctx) | ||
|
|
||
| resp, err := client.Do(req) |
There was a problem hiding this comment.
If the client parameter is nil, this call will cause a panic. It is safer to check for nil and use http.DefaultClient as a fallback.
| resp, err := client.Do(req) | |
| if client == nil { | |
| client = http.DefaultClient | |
| } | |
| resp, err := client.Do(req) |
| if resp.StatusCode != http.StatusOK { | ||
| return resp.StatusCode, body, &HTTPError{ | ||
| StatusCode: resp.StatusCode, | ||
| Body: body, | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoding http.StatusOK (200) as the only success condition makes this utility less reusable. Many APIs return other 2xx status codes (e.g., 201 Created, 202 Accepted, 204 No Content) to indicate success. Checking for the 2xx range is more robust for a shared utility.
| if resp.StatusCode != http.StatusOK { | |
| return resp.StatusCode, body, &HTTPError{ | |
| StatusCode: resp.StatusCode, | |
| Body: body, | |
| } | |
| } | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| return resp.StatusCode, body, &HTTPError{ | |
| StatusCode: resp.StatusCode, | |
| Body: body, | |
| } | |
| } |
| } | ||
|
|
||
| func (e *HTTPError) Error() string { | ||
| return fmt.Sprintf("HTTP %d: %s", e.StatusCode, string(e.Body)) |
There was a problem hiding this comment.
Including the full response body in the error string can lead to excessively large log entries if the API returns a large payload (e.g., a large HTML error page). This can impact performance and log storage efficiency. Consider truncating the body.
| return fmt.Sprintf("HTTP %d: %s", e.StatusCode, string(e.Body)) | |
| bodyStr := string(e.Body) | |
| if len(bodyStr) > 1024 { | |
| bodyStr = bodyStr[:1024] + "... (truncated)" | |
| } | |
| return fmt.Sprintf("HTTP %d: %s", e.StatusCode, bodyStr) |
References
- Avoid logging a potentially large list of items to prevent excessively large log entries, which can impact performance and log storage efficiency. Instead, log a summary such as the count of items.
| logger.Info("usage limit exceeded, will retry after 10 seconds", "batchStart", batchStart, "batchEnd", batchEnd) | ||
| if err != nil && commonhttp.IsStatusTooManyRequests(err) { | ||
| logger.Info("rate limit exceeded (429), will retry after 5 seconds", "batchStart", batchStart, "batchEnd", batchEnd) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Using time.Sleep inside a reconciliation loop blocks the worker thread and prevents other resources from being processed. In controller-runtime, it is recommended to return ctrl.Result{RequeueAfter: ...} to handle rate limiting or temporary failures. However, since this is inside a batch loop, you would need to persist the current progress in the CR status to resume correctly after a requeue.
| } | ||
|
|
||
| type Client struct { | ||
| Client *http.Client `json:"doclingclient"` |
There was a problem hiding this comment.
The *http.Client field should not have a json tag unless you specifically intend to serialize the client's internal state, which is usually not desired and often fails to produce useful output. If the Client struct is marshaled to JSON, this field should be ignored.
| Client *http.Client `json:"doclingclient"` | |
| Client *http.Client "json:\"-\"" |
1d30870 to
0ed8be6
Compare
📝 WalkthroughWalkthroughShared HTTP request creation, execution, timeout configuration, and typed status errors are added. Docling and embedding clients use the shared utilities, while controllers classify Docling 422 responses and retry embedding batches after 429 responses. ChangesHTTP error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/vectorembeddingsgenerator_controller.go (1)
221-238: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUnbounded, blocking retry on 429 can hang the reconcile.
On rate-limit, the loop synchronously sleeps 5s and retries with no maximum attempt count and no
ctx.Done()check. If the embedding API keeps returning 429, this blocks the reconcile goroutine (and a controller-runtime worker) indefinitely instead of returning and letting the controller requeue with backoff; it also ignores reconciler/context cancellation during the sleep.🔧 Proposed fix: bound retries and respect context cancellation
+ const maxRateLimitRetries = 5 + rateLimitRetries := 0 for batchStart := 0; batchStart < len(texts); batchStart += batchSize { batchEnd := min(batchStart+batchSize, len(texts)) batch := texts[batchStart:batchEnd] logger.Info("processing batch", "batchStart", batchStart, "batchEnd", batchEnd, "batchSize", len(batch)) embeddingResult, err := embeddingClient.GenerateEmbeddings(ctx, batch, encodingFormat) if err != nil && commonhttp.IsStatusTooManyRequests(err) { + rateLimitRetries++ + if rateLimitRetries > maxRateLimitRetries { + return false, fmt.Errorf("exceeded max retries after rate limiting: %w", err) + } logger.Info("rate limit exceeded (429), will retry after 5 seconds", "batchStart", batchStart, "batchEnd", batchEnd) - time.Sleep(5 * time.Second) + select { + case <-ctx.Done(): + return false, ctx.Err() + case <-time.After(5 * time.Second): + } batchStart -= batchSize continue } else if err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/vectorembeddingsgenerator_controller.go` around lines 221 - 238, Bound the 429 retry path in the batch loop around GenerateEmbeddings by limiting attempts and returning the error once the retry budget is exhausted, allowing reconciliation to requeue with backoff. Replace the blocking time.Sleep with a context-aware wait that exits promptly when ctx.Done() is triggered, while preserving normal batch advancement and non-429 error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/docling/client.go`:
- Around line 159-187: Update the 403 fallback in createDoclingRequest so the
retry request retains c.ClientConfig.Key by manually setting its Authorization
header to the raw API key after commonhttp.CreateHTTPRequest returns. Keep the
existing retry condition, request creation, and error handling unchanged.
In `@pkg/http/client.go`:
- Around line 50-52: Update the Authorization-header logic in CreateHTTPRequest
to set the header whenever apiKey is non-empty, using the formatted auth scheme
when authFormat is provided and the raw API key when it is empty. Preserve the
existing formatted Bearer-style behavior while enabling the fallback call from
the docling client.
In `@pkg/http/errors.go`:
- Around line 31-33: Update HTTPError.Error() to omit the complete
server-controlled Body from its returned message, using only the status code or
a bounded sanitized preview. Keep Body available for explicitly controlled
diagnostics, while preserving the existing HTTPError formatting context.
---
Outside diff comments:
In `@internal/controller/vectorembeddingsgenerator_controller.go`:
- Around line 221-238: Bound the 429 retry path in the batch loop around
GenerateEmbeddings by limiting attempts and returning the error once the retry
budget is exhausted, allowing reconciliation to requeue with backoff. Replace
the blocking time.Sleep with a context-aware wait that exits promptly when
ctx.Done() is triggered, while preserving normal batch advancement and non-429
error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c173e0b9-5021-4581-9f01-95ec8887ed1a
📒 Files selected for processing (6)
internal/controller/documentprocessor_controller.gointernal/controller/vectorembeddingsgenerator_controller.gopkg/docling/client.gopkg/embedding/client.gopkg/http/client.gopkg/http/errors.go
| func (c *Client) createDoclingRequest(ctx context.Context, method, endpoint string, payload []byte) ([]byte, error) { | ||
| logger := log.FromContext(ctx) | ||
| client := &http.Client{ | ||
| Timeout: 15 * time.Second, | ||
| } | ||
|
|
||
| req, err := c.createHTTPRequest(ctx, method, endpoint, payload, "Bearer %s") | ||
| logger.Info("sending request to docling service", "url", endpoint) | ||
|
|
||
| req, err := commonhttp.CreateHTTPRequest(ctx, method, endpoint, payload, "Bearer", c.ClientConfig.Key) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| logger.Info("sending request to docling service", "url", endpoint) | ||
| resp, err := client.Do(req) | ||
| statusCode, body, err := commonhttp.Do(ctx, c.Client, req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to send request: %w", err) | ||
| } | ||
|
|
||
| if resp.StatusCode == http.StatusForbidden && c.ClientConfig.Key != "" { | ||
| req, err = c.createHTTPRequest(ctx, method, endpoint, payload, "%s") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
| resp, err = client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to send request: %w", err) | ||
| // If we get 403 with Bearer auth, try without Bearer prefix | ||
| if statusCode == http.StatusForbidden && c.ClientConfig.Key != "" { | ||
| logger.Info("retrying with raw API key auth", "url", endpoint) | ||
| req, err = commonhttp.CreateHTTPRequest(ctx, method, endpoint, payload, "", c.ClientConfig.Key) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
| _, body, err = commonhttp.Do(ctx, c.Client, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return body, nil | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| logger.Error(errors.New("received non-200 OK response from endpoint"), | ||
| "docling request returned non-200 status", | ||
| "statusCode", resp.StatusCode, | ||
| "url", endpoint) | ||
| return nil, fmt.Errorf("failed to process request: status code %d", resp.StatusCode) | ||
| } | ||
|
|
||
| return resp.Body, nil | ||
| return body, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
403-retry silently drops the API key instead of sending it without the Bearer prefix.
commonhttp.CreateHTTPRequest only sets the Authorization header when both authFormat and apiKey are non-empty:
if apiKey != "" && authFormat != "" {
req.Header.Set("Authorization", fmt.Sprintf("%s %s", authFormat, apiKey))
}Calling it here with authFormat="" on the 403-retry path (line 174) therefore produces a request with no Authorization header at all, not the intended "raw API key auth". The retry silently sends an unauthenticated request, defeating the purpose of the fallback and likely resulting in a confusing second failure (401/403) that masks the real auth-format issue.
🐛 Proposed fix: set the header manually after creating the request
if statusCode == http.StatusForbidden && c.ClientConfig.Key != "" {
logger.Info("retrying with raw API key auth", "url", endpoint)
req, err = commonhttp.CreateHTTPRequest(ctx, method, endpoint, payload, "", c.ClientConfig.Key)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
}
+ req.Header.Set("Authorization", c.ClientConfig.Key)
_, body, err = commonhttp.Do(ctx, c.Client, req)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/docling/client.go` around lines 159 - 187, Update the 403 fallback in
createDoclingRequest so the retry request retains c.ClientConfig.Key by manually
setting its Authorization header to the raw API key after
commonhttp.CreateHTTPRequest returns. Keep the existing retry condition, request
creation, and error handling unchanged.
- The embedding and docling clients had a lot of duplicated code for making HTTP requests - same logic for creating requests, setting headers, handling responses. This was getting messy, especially when we needed to check for specific error codes like rate limits (429) or validation errors (422)
0ed8be6 to
5e23d0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/embedding/client.go`:
- Around line 53-58: Restore or retain the exported embedding.HTTPClientTimeout
compatibility alias alongside NewHTTPClient, mark it deprecated, and ensure it
maps to the shared HTTP client timeout value so existing downstream references
continue to compile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eb4558e-773c-4632-b5ca-fb642cc3d90a
📒 Files selected for processing (6)
internal/controller/documentprocessor_controller.gointernal/controller/vectorembeddingsgenerator_controller.gopkg/commonhttp/client.gopkg/commonhttp/errors.gopkg/docling/client.gopkg/embedding/client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/vectorembeddingsgenerator_controller.go
- internal/controller/documentprocessor_controller.go
- pkg/docling/client.go
| func NewHTTPClient(config *HTTPClientConfig) *HTTPClient { | ||
| return &HTTPClient{ | ||
| Client: &http.Client{ | ||
| Timeout: HTTPClientTimeout, | ||
| Timeout: commonhttp.HTTPClientTimeout, | ||
| }, | ||
| Config: config, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)pkg/embedding/|(^|/)commonhttp/' || true
echo '--- pkg/embedding/client.go ---'
cat -n pkg/embedding/client.go | sed -n '1,180p'
echo '--- search for HTTPClientTimeout ---'
rg -n 'HTTPClientTimeout' .Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 4879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n pkg/commonhttp/client.go | sed -n '1,120p'Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 3025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git diff --unified=0 -- pkg/embedding/client.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 185
Preserve embedding.HTTPClientTimeout as a compatibility alias
pkg/embedding is public API, so removing the exported timeout constant will break downstream builds. Keep a deprecated alias here, or make the breaking change explicit in a version bump/release note.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/embedding/client.go` around lines 53 - 58, Restore or retain the exported
embedding.HTTPClientTimeout compatibility alias alongside NewHTTPClient, mark it
deprecated, and ensure it maps to the shared HTTP client timeout value so
existing downstream references continue to compile.
| embeddingResult, err := embeddingClient.GenerateEmbeddings(ctx, batch, encodingFormat) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "status 429") { | ||
| logger.Error(err, "embedding API rate limited (429), will retry on next reconciliation", "file", chunksFilePath, "batchStart", batchStart) | ||
| } else { | ||
| logger.Error(err, "failed to generate embeddings for batch", "file", chunksFilePath, "batchStart", batchStart, "batchEnd", batchEnd) | ||
| } | ||
| if err != nil && commonhttp.IsStatusTooManyRequests(err) { | ||
| logger.Info("rate limit exceeded (429), will retry after 5 seconds", "batchStart", batchStart, "batchEnd", batchEnd) | ||
| time.Sleep(5 * time.Second) | ||
| batchStart -= batchSize | ||
| continue | ||
| } else if err != nil { |
There was a problem hiding this comment.
I think just returning the error and let the controller's runtime work queue requeue it
if err != nil && commonhttp.IsStatusTooManyRequests(err) {
logger.Info("rate limited (429), will retry on next reconciliation",
"batchStart", batchStart, "batchEnd", batchEnd)
return false, err
} else if err != nil {
logger.Error(err, "failed to generate embeddings for batch",
"batchStart", batchStart, "batchEnd", batchEnd)
return false, err
}3 participants
Summary by CodeRabbit
Bug Fixes
Refactor