[fetch] does not support fetch options like proxy

[Kruspe]

When using the aws-xray-sdk-fetch instrumentation it is not possible to supply further options to the fetch call like the agent to supply the undici ProxyAgent.

The capturedFetch call only calls the global available fetch call with one argument

aws-xray-sdk-node/sdk_contrib/fetch/lib/fetch_p.js

Line 117 in 73e1fca

response = await baseFetchFunction(requestClone);

and does not pass down options provided by the user.

I would propose to add a third option to the function that patches the global fetch. This would allow the user to use all fetch options and additionally supply their own segment information if required.

aws-xray-sdk-node/sdk_contrib/fetch/lib/fetch_p.js

Line 59 in 73e1fca

const overridenFetchAsync = async (...args) => {

For this change it would be necessary to think about how not to introduce a breaking change for users that expect the second argument to be used for segment information. But I guess we can write some internal logic to check the supplied args.

Once I receive some feedback I can get started on a PR and implement the changes.

[jj22ee]

Hi, you should be able to pass options into the fetch call. For example, the following code sample will change the method type used in fetch from GET to POST. This will be reflected in the trace as well.

const { captureFetchGlobal } = require('aws-xray-sdk-fetch');
const fetch = captureFetchGlobal();

...

  const result = fetch('https://someURL.com', {
    method: "POST",
    headers: {
      "Content-Type": "application/json",
    },
    body: JSON.stringify({data:123, data2:"234"}),
  });

The options are added into the fetch call through the Request object:

aws-xray-sdk-node/sdk_contrib/fetch/lib/fetch_p.js

Lines 63 to 117 in 73e1fca

	const request = typeof args[0] === 'object' ?
	args[0] :
	new requestClass(...args);
	// Facilitate the addition of Segment information via the request arguments
	const params = args.length > 1 ? args[1] : {};
	// Short circuit if the HTTP is already being captured
	if (request.headers.has('X-Amzn-Trace-Id')) {
	return await baseFetchFunction(...args);
	}
	const url = new URL(request.url);
	const isAutomaticMode = AWSXRay.isAutomaticMode();
	const parent = AWSXRay.resolveSegment(AWSXRay.resolveManualSegmentParams(params));
	const hostname = url.hostname || url.host || 'Unknown host';
	if (!parent) {
	let output = '[ host: ' + hostname +
	(request.method ? (', method: ' + request.method) : '') +
	', path: ' + url.pathname + ' ]';
	if (isAutomaticMode) {
	getLogger().info('RequestInit for request ' + output +
	' is missing the sub/segment context for automatic mode. Ignoring.');
	} else {
	getLogger().info('RequestInit for request ' + output +
	' requires a segment object on the options params as "XRaySegment" for tracing in manual mode. Ignoring.');
	}
	// Options are not modified, only parsed for logging. We can pass in the original arguments.
	return await baseFetchFunction(...args);
	}
	let subsegment;
	if (parent.notTraced) {
	subsegment = parent.addNewSubsegmentWithoutSampling(hostname);
	} else {
	subsegment = parent.addNewSubsegment(hostname);
	}
	subsegment.namespace = 'remote';
	request.headers.set('X-Amzn-Trace-Id',
	'Root=' + (parent.segment ? parent.segment : parent).trace_id +
	';Parent=' + subsegment.id +
	';Sampled=' + (subsegment.notTraced ? '0' : '1'));
	// Set up fetch call and capture any thrown errors
	const capturedFetch = async () => {
	const requestClone = request.clone();
	let response;
	try {
	response = await baseFetchFunction(requestClone);

[Kruspe]

Hey, what you say is true but Request does not support the dispatcher option as far as I know.

With fetch you can set the dispatcher and the request will go through the Proxy.

import { ProxyAgent } from 'undici';

fetch("http://localhost", {
  dispatcher: new ProxyAgent("http://localhost:4444"),
}));

If we create a request with similar options like this:

import { ProxyAgent } from 'undici';

const request = new Request("http://localhost", {
  dispatcher: new ProxyAgent("http://localhost:4444"),
});

fetch(request)

The Request does not contain the dispatcher option and the request will not pass through the proxy.

So inside the capturedFetch function we will not traverse the proxy. While the calls to the baseFetchFunction should be able to use the proxy as they use all the provided args

[jj22ee]

I see. In this case, the fetch function should not automatically pass the arguments into a Request object as it currently does here:
https://github.com/aws/aws-xray-sdk-node/blob/master/sdk_contrib/fetch/lib/fetch_p.js#L65

Instead, if the Fetch options are supplied like in your first example, the original object that is passed into the instrumented fetch client should be used when the baseFetchFunction is called.

We would welcome a PR from your end if you have a fix in mind!

[Kruspe]

Alright sounds good. I'll take a stab at it :)

[Kruspe]

Hey @jj22ee,
just had some time and implemented a small tweak to the code that checks if the dispatcher option is passed to the fetch call and if so, it passes it to the fetch call.
It would also be possible to do some more sophisticated parsing of the arguments but this would require more tweaks to the code base. So I thought I share my first idea and you can give me some feedback if the simple implementation in #653 is enough.

[jj22ee]

Closing issue as PR is merged. This fix will be in the next release.

[jj22ee]

Hi @Kruspe, we have reverted the change in #687 in aws-xray-sdk-fetch@3.10.2 due to issue in #679 (X-Amzn-Trace-Id header not propagated to downstream when not using a request object in Fetch).

I've re-opened this issue. I think we can take another look at the initial approach in your original PR (627b6df) with some additional changes:

• Use apply response = await baseFetchFunction(requestClone, fetchOptions); only if fetchOptions is not undefined, else use previous logic.
• Add tests for ensuring headers have x-amzn-trace-id when using the dispatcher option.

[Kruspe]

I'll take a look tomorrow and see what the issue is. Thanks for the heads up!

[Kruspe]

Hey @jj22ee, the PR is there to implement this again. Have a look at let me know what you think.

During my testing I noticed that we ignore headers that are set by users if they do this:

fetch(new Request("https://somewhere.com"), {headers: {"foo": "bar"}});

These headers will never reach the server as we ignore them. This is different from the original fetch implementation that still respects those headers and will add them to the request.

Maybe it would make more sense to add the X-Amzn-Trace-Id to the header argument of params instead of to the request directly and thus being able to always use the params when calling fetch()