aws / aws-xray-sdk-node

The official AWS X-Ray SDK for Node.js.
Apache License 2.0
266 stars 155 forks source link

[fetch] does not support fetch options like proxy #650

Closed Kruspe closed 1 month ago

Kruspe commented 3 months ago

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 https://github.com/aws/aws-xray-sdk-node/blob/73e1fcaba509a6b6fcfc81254a25da37189bea4b/sdk_contrib/fetch/lib/fetch_p.js#L117 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. https://github.com/aws/aws-xray-sdk-node/blob/73e1fcaba509a6b6fcfc81254a25da37189bea4b/sdk_contrib/fetch/lib/fetch_p.js#L59

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 commented 2 months ago

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: https://github.com/aws/aws-xray-sdk-node/blob/73e1fcaba509a6b6fcfc81254a25da37189bea4b/sdk_contrib/fetch/lib/fetch_p.js#L63-L117

Kruspe commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

Kruspe commented 2 months ago

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 commented 1 month ago

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