fix: set opaque on request #560
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing error handling, timing metrics, and logging functionalities. Key changes include the addition of a Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #560 +/- ##
==========================================
+ Coverage 94.62% 94.65% +0.03%
==========================================
Files 11 11
Lines 1227 1235 +8
Branches 296 298 +2
==========================================
+ Hits 1161 1169 +8
Misses 62 62
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (5)
test/options.timing.test.ts (1)
55-55: Remove commented console.log.Instead of keeping commented debug code, consider removing it entirely.
- // console.log(res.timing);src/fetch.ts (1)
250-251: Consider structuring debug log for better readabilityThe debug log combines multiple objects which might be hard to read in logs.
Consider splitting into multiple log lines:
- debug('Request#%d got response, status: %s, headers: %j, timing: %j, socket: %j', - requestId, urllibResponse.status, urllibResponse.headers, timing, urllibResponse.socket); + debug('Request#%d got response, status: %s', requestId, urllibResponse.status); + debug('Request#%d headers: %j', requestId, urllibResponse.headers); + debug('Request#%d timing: %j', requestId, timing); + debug('Request#%d socket: %j', requestId, urllibResponse.socket);src/HttpClient.ts (3)
Line range hint
1009-1019: Consider enhancing error messages with more contextThe error handling could be more informative by including the request URL and timeout values in the error messages.
Consider this improvement:
if (err.name === 'HeadersTimeoutError') { - err = new HttpClientRequestTimeoutError(headersTimeout, { cause: err }); + err = new HttpClientRequestTimeoutError( + `Headers timeout of ${headersTimeout}ms exceeded for ${requestUrl.href}`, + { cause: err } + ); } else if (err.name === 'BodyTimeoutError') { - err = new HttpClientRequestTimeoutError(bodyTimeout, { cause: err }); + err = new HttpClientRequestTimeoutError( + `Body timeout of ${bodyTimeout}ms exceeded for ${requestUrl.href}`, + { cause: err } + ); }
Line range hint
1020-1027: Consider implementing exponential backoff for retriesThe current retry mechanism could be enhanced with exponential backoff to prevent overwhelming the server during issues.
Consider adding exponential backoff:
if (args.socketErrorRetry > 0 && requestContext.socketErrorRetries < args.socketErrorRetry) { requestContext.socketErrorRetries++; + const backoffDelay = Math.min(1000 * Math.pow(2, requestContext.socketErrorRetries - 1), 10000); + await sleep(backoffDelay); debug('Request#%d retry on socket error, socketErrorRetries: %d', requestId, requestContext.socketErrorRetries); return await this.#requestInternal(url, options, requestContext); }
Line range hint
1028-1056: Ensure response body is always consumedThere's a potential memory leak when errors occur. The response body should be consumed even in error cases to properly clean up resources.
Add response body cleanup in the error handling:
} catch (rawError: any) { debug('Request#%d throw error: %s, socketErrorRetry: %s, socketErrorRetries: %s', requestId, rawError, args.socketErrorRetry, requestContext.socketErrorRetries); let err = rawError; + // Ensure response body is consumed even in error cases + if (response?.body) { + try { + await response.body.arrayBuffer(); + } catch (consumeError) { + debug('Error consuming response body: %s', consumeError); + } + } // ... rest of error handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/HttpClient.ts(1 hunks)src/Response.ts(1 hunks)src/diagnosticsChannel.ts(8 hunks)src/fetch.ts(6 hunks)src/symbols.ts(1 hunks)test/fetch.test.ts(3 hunks)test/options.timing.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/diagnosticsChannel.ts
[warning] 122-122: src/diagnosticsChannel.ts#L122
Added line #L122 was not covered by tests
🔇 Additional comments (9)
src/symbols.ts (1)
20-20: LGTM! Verify symbol usage across codebase.
The new symbol follows the established naming pattern and aligns with the PR's objective of fixing opaque request handling.
✅ Verification successful
Let me check the usage context in diagnosticsChannel.ts to ensure proper implementation.
Symbol is correctly implemented and used consistently
The symbol is properly used throughout diagnosticsChannel.ts for managing request opaque data across different diagnostic events (sendHeaders, bodySent, headers, trailers). The implementation follows a consistent pattern:
- Setting opaque data using Reflect.set
- Retrieving opaque data using Reflect.get
- Proper null checks and request ID validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new kRequestInternalOpaque symbol
# Expected: Should find references in request handling code
rg "kRequestInternalOpaque" --type ts
Length of output: 601
Script:
#!/bin/bash
# Get the context around the symbol usage in diagnosticsChannel.ts
rg "kRequestInternalOpaque" -B 3 -A 3 --type ts src/diagnosticsChannel.ts
Length of output: 2128
test/fetch.test.ts (2)
57-63: LGTM: Keep-alive test implementation
Good addition of keep-alive testing by adding a sleep between requests and validating socket reuse through handledRequests and handledResponses counters.
88-96: Improved error handling test pattern
Good refactoring to use assert.rejects instead of try-catch, making the test more concise and focused on the expected error conditions.
src/fetch.ts (2)
143-143: LGTM: Added DNS lookup timing metric
Good addition of the dnslookup timing metric to track DNS resolution performance.
224-226: Enhanced error handling with debug logging
Good improvement in error handling by updating socket info and adding debug logging before re-throwing the error.
src/diagnosticsChannel.ts (3)
92-94: LGTM: Improved opaque data handling
Good improvement using Reflect.set to store the opaque data on the request object, making it accessible throughout the request lifecycle.
147-147: LGTM: Consistent opaque data access
Good refactoring to use Reflect.get(request, symbols.kRequestInternalOpaque) consistently across all event handlers.
Also applies to: 172-172, 186-186, 211-211
119-124: Verify error logging coverage
The error logging path at line 122 is not covered by tests according to codecov.
Would you like me to help create a test case that covers this error scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-122: src/diagnosticsChannel.ts#L122
Added line #L122 was not covered by tests
src/HttpClient.ts (1)
303-303: LGTM: Enhanced timing metrics with DNS lookup timing
The addition of dnslookup timing metric improves observability by providing insights into DNS resolution performance.
| @@ -28,7 +28,7 @@ export type Timing = { | |||
| // socket assigned | |||
| queuing: number; | |||
| // dns lookup time | |||
| // dnslookup: number; | |||
| dnslookup: number; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
DNS lookup timing metric is not properly implemented
While the dnslookup property is added to the Timing type, there's no actual implementation to measure and set this value. The codebase shows:
- Timing measurements are done using
performance.now() - DNS lookup is performed in
HttpAgent.tsusing thelookupfunction - However, there's no code connecting the DNS lookup duration to the timing object
The following locations need attention:
src/HttpAgent.ts: Add timing measurement around the DNS lookup operationsrc/fetch.tsandsrc/HttpClient.ts: Ensure the DNS lookup timing is properly propagated
🔗 Analysis chain
Verify dnslookup timing implementation.
The dnslookup timing property has been properly added to the Timing type. Its position in the interface correctly reflects the chronological order of HTTP request phases.
Let me gather more information to verify the timing implementation.
Let me try one more search pattern to find the timing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of dnslookup timing
# Expected: Should find where this timing metric is calculated
# Search for dnslookup timing calculation
rg "dnslookup.*=|timing\.dnslookup" --type ts
# Search for DNS-related code that might set this timing
ast-grep --pattern 'lookup($$$)'
Length of output: 1593
Script:
#!/bin/bash
# Search for timing-related code around DNS lookup
rg "timing.*dns|dns.*timing" -B 2 -A 2 --type ts
# Look for timing initialization and calculation patterns
ast-grep --pattern 'timing = {
$$$
}'
# Search for timing-related code in the HttpAgent
rg "timing" --type ts src/HttpAgent.ts -B 2 -A 2
Length of output: 141
Script:
#!/bin/bash
# Search for files that might contain timing implementation
rg "new Date\(\)|performance\.now\(\)" -B 2 -A 2 --type ts
# Look for timing-related code in the client implementation
fd "client" --type f --extension ts --exec rg "timing" -B 2 -A 2 {}
# Search for timing property assignments
ast-grep --pattern '$_.$_.timing = $_'
Length of output: 7594
| @@ -32,7 +32,7 @@ describe('options.timing.test.ts', () => { | |||
| assert(res.timing.contentDownload > 0); | |||
| assert(res.timing.contentDownload > res.timing.waiting); | |||
| assert(res.timing.contentDownload <= res.rt); | |||
| assert(res.socket.handledResponses === 1); | |||
| assert.equal(res.socket.handledResponses, 1); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for dnslookup timing.
While the assertion changes improve precision, consider adding assertions for the newly added dnslookup timing property to ensure it's working as expected.
Add these assertions after the existing timing checks:
assert(res.timing.queuing > 0);
+assert(res.timing.dnslookup >= 0);
assert(res.timing.connected > 0);Also applies to: 48-48, 54-54
[skip ci] ## [4.6.5](v4.6.4...v4.6.5) (2024-12-07) ### Bug Fixes * set opaque on request ([#560](#560)) ([8cf5c3b](8cf5c3b))
Summary by CodeRabbit
Release Notes
New Features
dnslookup.Bug Fixes
Tests
Chores