-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proxy support for global fetch #696
base: master
Are you sure you want to change the base?
Conversation
@@ -127,7 +128,7 @@ function enableCapture(baseFetchFunction, requestClass, downstreamXRayEnabled, s | |||
const requestClone = request.clone(); | |||
let response; | |||
try { | |||
response = await baseFetchFunction(requestClone); | |||
response = await baseFetchFunction(requestClone, fetchOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first idea was to pass down the entire args again but this would overwrite the changes we do to the request like adding the trace header as the headers from the options take priority over the headers that are already set for a request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this code:
const agent = new Agent({maxSockets: 1234});
const request = new Request('someurl', { dispatcher: agent });
fetch(request);
Here, fetch is only called with one parameter. The dispatcher is "baked in" into the request. For undici, it's actually a symbol property. When you call baseFetchFunction(requestClone, fetchOptions)
, requestClone
doesn't contain that property, So that per-request-setting wouldn't be passed on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an attempt to fix this. Sorry for the complexity, but it should work more reliably for you.
#698
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#698 fixes a different issue that is there so I would open another issue to point to your PR to.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #696 +/- ##
=======================================
Coverage 83.56% 83.56%
=======================================
Files 36 36
Lines 1813 1813
=======================================
Hits 1515 1515
Misses 298 298 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Issue #650:
Description of changes:
Support setting the dispatcher for the global fetch implementation.
Instead of ignoring the created request, we now use it and pass the dispatcher option if it is available.
Added an integration test with a reverse proxy to check that the trace header is there.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.