Feature Request: support for tracing NodeJs 18 native fetch calls

[RaphaelManke]

Expected Behaviour

In our Team we use the native nodeJS 18 fetch module to make api calls.

We were expecting that fetch calls would be traced the same as they where when we used node-fetch.

Current Behaviour

The fetch calls are not traced whereas the node-fetch calls are traced.

The x-ray traces show only the api calls made with node-fetch.

Code snippet

import { LambdaInterface } from "@aws-lambda-powertools/commons";
import { Tracer } from "@aws-lambda-powertools/tracer";
import nodeFetch from "node-fetch";

const tracer = new Tracer();

class Lambda implements LambdaInterface {
  // Decorate your handler class method

  @tracer.captureMethod()
  private async nativeFetch() {
    const response = await fetch("https://api.github.com/users/aws-samples");
    const json = await response.json();
    return json;
  }

  @tracer.captureMethod()
  private async nodeFetch() {
    const response = await nodeFetch(
      "https://api.github.com/users/aws-samples"
    );
    const json = await response.json();
    return json;
  }

  public async handler(_event: unknown, _context: unknown): Promise<void> {
    await this.nativeFetch();
    await this.nodeFetch();
  }
}

const handlerClass = new Lambda();
export const handler = handlerClass.handler.bind(handlerClass); //

Steps to Reproduce

1. clone the bug repo https://github.com/RaphaelManke/aws-powertools-node-18-tracing-fetch-bug
2. npm install
3. npm run cdk deploy
4. invoke the lambda

Possible Solution

No response

Powertools for AWS Lambda (TypeScript) version

1.11.1

AWS Lambda function runtime

18.x

Packaging format used

npm

Execution logs

No response

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[am29d]

Hey @RaphaelManke, thanks for raising the issue. After a closer look it seems like this is x-ray SDK specific, since we are using it. The related issue and discussion is here aws/aws-xray-sdk-node#531 .

[dreamorosi]

We have been tracking this for a while and at the moment we are blocked by the X-Ray SDK that we use for our Tracer utility, which does not support this.

We are aware of a potential workaround (mentioned in the issue Alex mentioned), however since fetch is still behind feature flag (EDIT see here) marked as experimental in Node.js 18, and up until now we didn't see significant demand, we decided to not prioritize this.

I'm adding the need-customer-feedback label to the issue so that others can add a 👍 to the original post to show their interest in the feature. If you are not comfortable doing so and/or prefer to describe your use case privately, you can also send us an email at [email protected]. Both those actions will help us to document demand and hopefully prioritize implementation.

[felixmagnus]

This probably won't make much of a difference in terms of your prioritization, but:

[...] however since fetch is still behind feature flag in Node.js 18, a [...]

If I'm not mistaken, fetch is explicitly not behind a feature flag in node 18 - it is available by default but can still be disabled using a feature flag (--no-experimental-fetch) Source. In any case the feature is still marked as experimental.

[dreamorosi]

I stand corrected, you are definitely right - I have updated my comment above and linked it to yours. Thank you for clarifying!

Additionally, to provide more context on why node-fetch is traced and native fetch isn't: it appears that node-fetch uses the http and https node modules (source), which are supported by the X-Ray SDK. On the other hand, native fetch uses different modules altogether, which are not instrumented by the SDK.

[RaphaelManke]

I totally get the limitation now and thank you for clarifying this.

I think that there should be a statement in the info box highlighting this limitation in the info box on the tracer page.
https://docs.powertools.aws.dev/lambda/typescript/latest/core/tracer/#tracing-http-requests

In the business context I see two kind of people who will run into the same issue.
First those who start new projects and start using the NodeJS 18 runtime and therefore don't use the node-fetch library because its build in the runtime.
These people will be surprised that they don't get traces.

The other group are those people that are upgrading there NodeJS runtime to NodeJS 18 and get rid of unneeded dependencies and therefore remove the node-fetch library.
These people would als be surprised that api call traces are missing. For those it might be an even "bigger" problem because they would notice the missing traces not right away because functionality still works after the upgrade.

For both use cases a hint in the documentation would be helpful.

[dreamorosi]

I totally agree, we'll be adding a notice to the docs in the next few hours. Thanks for the suggestion!

[RaphaelManke]

So I did came up with a workaround. It's just a POC and I am not sure if its something the powertools would implement or x-ray.

The workaround uses the undici diagnostic channels to add subsegments.
https://github.com/nodejs/undici/blob/main/docs/api/DiagnosticsChannel.md

const segments = new Map<
  any,
  { subsegment: Subsegment; parentSubsegment: Segment | Subsegment }
>();

function addNativeFetchTracing(tracer: Tracer) {
  /**
   * This is the first event emitted when a request is created.
   * Based on this event a new subsegment is created.
   * To have a reference to the subsegment created, it is stored in a map.
   */
  diagnosticsChannel
    .channel("undici:request:create")
    .subscribe(({ request }: any) => {
      const parentSubsegment = tracer.getSegment(); // This is the subsegment currently active
      let subsegment;
      if (parentSubsegment) {
        const [_, baseUrl] = request.origin.split("//");

        subsegment = parentSubsegment.addNewSubsegment(baseUrl);
        tracer.setSegment(subsegment);
        subsegment.addAttribute("namespace", "remote");
      }
      segments.set(request, {
        subsegment: subsegment!,
        parentSubsegment: parentSubsegment!,
      });
    });

  /**
   * When the response is received, the response data and headers are added to the subsegment.
   */
  diagnosticsChannel
    .channel("undici:request:headers")
    .subscribe(async ({ request, response }: any) => {
      const { subsegment, parentSubsegment } = segments.get(request)!;

      if (parentSubsegment && subsegment) {
        const [protocol, host] = request.origin.split("//");

        // Undici returns the headers as an buffer array of key-value pairs.
        const headers = arrayToObject(response.headers);
        // The expected request does not match and need to be modified 
        subsegment.addRemoteRequestData(
          {
            ...request,
            host,
            agent: {
              protocol,
            },
          },
          {
            ...response,
            headers: headers,
          }
        );
      }
    });

  /**
   * The subsegment is closed when the response is finished.
   */
  diagnosticsChannel
    .channel("undici:request:trailers")
    .subscribe(({ request }: any) => {
      const { subsegment, parentSubsegment } = segments.get(request)!;

      if (parentSubsegment && subsegment) {
        subsegment.close();
        tracer.setSegment(parentSubsegment);
        segments.delete(request);
      }
    });
}

function arrayToObject(arr: any[]): { [key: string]: any } {
  const result: { [key: string]: any } = {};

  if (arr.length % 2 !== 0) {
    throw new Error("Array length should be even to form key-value pairs.");
  }

  for (let i = 0; i < arr.length; i += 2) {
    const key = arr[i].toString().toLowerCase();
    const value = arr[i + 1].toString();
    result[key] = value;
  }

  return result;
}

Its also added to the reproduction repo RaphaelManke/aws-powertools-node-18-tracing-fetch-bug@823864f

What do you think? Is this something that should go into powertools or is it too experimental 😓

[dreamorosi]

Hi @RaphaelManke thank you for sharing this!

Coincidentally I was also looking into a workaround and was able to find one, although taking a different route (similar to the workaround described in the X-Ray issue linked above). You can see the implementation here: https://github.com/aws-powertools/powertools-lambda-typescript/blob/feat/tracer/nativefetch/packages/tracer/src/provider/ProviderService.ts#L170

All things considered it seems we have already two options, so we are considering implementing one of them in the Tracer in the next release or the one afterwards (we are planning on releasing today or Monday, and we might not make it in time).

Let me spend some time looking at your implementation.

[RaphaelManke]

I also found an open PR at the x-ray-node repo going into that direction.
aws/aws-xray-sdk-node#590

[dreamorosi]

Hi Raphael, I just wanted to share an update before checking out for the day.

First of all, I really like your implementation! I think using the diagnostics_channel module is an elegant solution and a non-intrusive one at that. To be honest I wasn't aware of the module before today, so thank you for sharing it.

My two main concerns around that implementation are: 1/ support to the API is listed as experimental, and 2/ according to the undici page you linked the module is available in Node.js 16+.

Regarding point 1: I have used undici as a user in the past, and I know that the native fetch implementation in Node.js is based on it. I am not however familiar (yet) with how changes from the former are propagated to Node.js and so at the moment I am not able to assess the stability/pace of the diagnostics_channel support in native fetch.

If major changes to fetch in Node.js, including its support to diagnostic_channel, are made only between major versions then we could consider adopting an implementation that relies on it, if not then we need to be careful. I would like to avoid a situation in which changes to undici or to its support to diagnostic_channel are introduced in a minor/patch version of Node.js, which could cause issues to our customers.

Regarding point 2: I am not clear on whether diagnostics_channel is available in Node.js 14 or not. As mentioned, the page you linked says it shouldn't be present. However the Node.js docs say it was added in Node.js 14.17.0 (see here under History). If the module is not available at all in Node.js 14, then it could be a dealbreaker for us at least until we drop support for this Lambda runtime.

Both points might be mitigated with some additional investigation and tests, however at the moment I'm not able to provide an ETA. For context: our current stance is to support all current AWS Lambda runtimes (nodejs14x, nodejs16x, nodejs18x at the time of writing) and thus, the presence/absence of a module in any of them is a deciding factor.

We are going to need a bit more time to continue investigating a solution that fits all runtimes (which might be among the two discussed so far) and after that we'll need to run some manual testing and implement automated ones. At that point we will be in a better position to share an ETA.

For now, I have moved the issue to the backlog and changed its status to reflect the fact that we are going to actively look into this and try to find a solution. This feature is a priority for us, so if we find a viable implementation we'd be happy to include it in Powertools.

[thomasfjortoft]

@dreamorosi do you have a update on implementation plan?

[jasonterando]

I also found an open PR at the x-ray-node repo going into that direction. aws/aws-xray-sdk-node#590

I'll try and get the PR moving again. I had run out of time/bandwidth before. Maybe I can shepherd it through this time.

[RaphaelManke]

I found an implementation for otel. It might be helpful
https://github.com/gas-buddy/service/blob/main/src/telemetry/fetchInstrumentation.ts#L44

[ilmarivacklinsc]

Looks like aws/aws-xray-sdk-node#590 was merged 🎉 This didn't get into 2.0, but can we get it for a patch release? Thanks!

[dreamorosi]

The AWS SDK did merge that PR but it was released under a separate package aws-xray-sdk-fetch, Powertools uses aws-xray-sdk-core only to keep bundle small.

Additionally their implementation is based on the same logic as the patching for the http & https modules, which uses require. I still believe the implementation proposed by @RaphaelManke make above is much better.

I'll try to prioritize this feature since there's a lot of demand, alternatively if anyone is interested in contributing I'd be happy to advise on the design and provide timely reviews to get it merged.

[jasonterando]

Additionally their implementation is based on the same logic as the patching for the http & https modules, which uses require. I still believe the implementation proposed by @RaphaelManke make above is much better.

It should work with recent Node's "built-in" fetch as well as the older node-fetch. Are you having issues with it?

[dreamorosi]

Are you having issues with it?

No, the instrumentation works.

I'm mainly concerned about ESM support, and overall I would prefer to move away from CJS-only code as much as possible.

The captureFetchGlobal() you contributed in the SDK repo would work but I think the implementation that relies on the diagnostics channel shown above is cleaner and easier to maintain - but I recognize this comes down to personal preference.

Additionally, we are not going to target node-fetch at all, customers who want to use the fetch API can do so either using the global fetch or undici, which will both work using the instrumentation based on diagnostics channel.

In the long term I'd like to move the tracing of http and https to that pattern as well since they also emit events on their own channel.

[RaphaelManke]

I like that this topic gets some traction 🙂
@dreamorosi i am not sure how the priorities are made but I think if the xray solution is feasible with less effort it could be a good starting point to deliver the feature to the customer.
It still can be replaced with an alternative implementation in the future.
Unless the efforts are the same then it probably makes sense to go with sustainable solution.

[dreamorosi]

Hi everyone, I understand that this is an in demand feature and I acknowledge that it's overdue on our side. Unfortunately due to circumstances outside of my control the team did not have the bandwidth to work on this sooner, so I appreciate everyone's patience and continued support for the project.

I have started working on a PR (#2293) that uses an implementation based on diagnostics_channel and it's currently in draft. I still need to finish cleaning up the code and write integration tests for it, but the implementation is already working and is close enough to be done.

I hope this shows that I want to prioritize this and that I want to get it merged as soon as possible but without compromising on quality and long term vision for Powertools.

The code in the PR is already working and generating traces for requests made using fetch, below you can see an example of a request:

In the meantime, if you want to adopt the version that was merged in the X-Ray SDK core, you can already do so like this:

import { Tracer } from "@aws-lambda-powertools/tracer";
import { captureLambdaHandler } from "@aws-lambda-powertools/tracer/middleware";
import { captureFetchGlobal } from "aws-xray-sdk-fetch"; // npm i aws-xray-sdk-fetch
import middy from "@middy/core";

const tracer = new Tracer();
captureFetchGlobal();

export const handler = middy(async (event: unknown) => {
  await fetch("http://httpbin.org/status/500");
}).use(captureLambdaHandler(tracer));

I have tested it and it seem to work well with Tracer, you should be able to make it work by only installing the module and calling captureFetchGlobal() right after you instantiate the Tracer class.

[dreamorosi]

Just to manage everyone's expectations, the team is based in EMEA and we're starting a long weekend - so assuming everything works and we can write integration tests for it - the PR won't be merged before mid next week at the earliest.

[dreamorosi]

Just a small update from our side: I've completed the implementation, tests, and docs and the PR is now ready for review.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

This is now released under v2.0.4 version!