nodejs 18: Support native fetch

[ehtb]

Node.js 18+ support the native fetch API. The current fetch instrumentation (instrumentation-fetch) only works in a web / browser environment. This is because it is importing sdk-trace-web and depending on location being globally defined.

Before native support this was not a problem, because solutions like node-fetch were using http under the hood, and were therefore wrapped correctly. Now with native fetch support this is not happening anymore.

I can think of these two solutions:

1. Change instrumentation-fetch to be platform agnostic. This has the benefit of using one module consistently in multiple environments
2. Create a new instrumentation, for instance instrumentation-fetch-node which is tailored to the node.js / server environment. This would allow for easier support of some node-only utilities. Support would be for node >= 18.

Could you please let me know whether support for this is in the pipeline, and perhaps from which nodejs version onward?

[dyladan]

Hmm the instrumentation shouldn't depend on the SDK package anyway so that needs to be updated. I'm not sure how easy it would be to make the browser fetch instrumentation work in both places, but that would be my preferred solution.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[Fryuni]

This is still relevant.
I'll try to allocate some time to contribute to this if no one else steps in first.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[ktrz]

Hi @Fryuni
Did you have any time to work on this issue? Maybe I could help?

[Fryuni]

I started working on a patch that would work for us, but it breaks other scenarios. I still need to work on a proper solution, but time has been eluding me.

If you want to tackle this, here is what I got:

The instrumentation-fetch package uses sdk-trace-web, which relies on browser-only APIs, like the global scope being window and the presence of the global objects location, navigation, and navigator.

It uses only 5 functions from sdk-trace-web. As far as I can tell, the semantics of this function for the caller is entirely agnostic of the browser-only elements, so providing the same functions for Node should suffice.

I used patch-package to comment out everything that used the browser APIs from sdk-trace-web or to replace with a return null or return ''. Just to get it working.

Those 5 functions do all the heavy work for the browser, like creating an empty anchor and moving a value through it to normalize things, but it doesn't seem to me like there would be a lot of work to do when in Node, so it should be quite straightforward to implement those. It could be provided as a NodeFetchInstrumentation or it could even be the same FetchInstrumentation that validates upon load which environment it is in.

I've been analyzing this slowly since my previous comment when I have some spare time, mostly while commuting, so I don't even have a branch for you to work from, sorry about that. 😄

[ktrz]

Thanks for the update! I'll see if I can follow up on your research

[RichiCoder1]

Following this too! Would be great for this instrumentation to also work for other serverside WinterCG environments (Vercel, CloudFlare Workers)

[Grunet]

For reference for Deno I just had to make location.href not crash and the fetch autoinstrumentation seemed to work for a modified version of the basic-tracer-node example

// Monkeypatching to get past FetchInstrumentation's dependence on sdk-trace-web, which depends on some browser-only constructs
globalThis.location = {}; //For this line in parseUrl - https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-web/src/utils.ts#L310

And Grunet/otel-js-deno-experimentation@8cbcc12 for the change to the modified example I've been working with

[Grunet]

Also wanted to cross-document here that the fetch autoinstrumentation package is currently using Date.now() instead of performance.now(), which may explain small offsets in traces (I was seeing them for Deno)

More references for this at #3719 (comment)

[Fryuni]

Also wanted to cross-document here that the fetch autoinstrumentation package is currently using Date.now() instead of performance.now(), which may explain small offsets in traces

Date.now() returns milliseconds since unix epoch, performance.now() returns milliseconds since performance.timeOrigin with microseconds precision. They have very different meanings.

If you see offsets between distributed traces, it is probably due to clock skew between the services. performance.now() would provide better precision for the duration of each trace but not for the relation between them.

And if Date.now() gets replaced, I'd suggest using performance.now() only for the web. Node has process.hrtime.bigint() for nanoseconds precision, which would be even better.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[Multiply]

Is anyone currently working on this?
We switched back to using node-fetch package to force it back to using http/https packages for tracing.

I might have a few days available to work on this during the summer, if noone else is actively on it.

[drewcorlin1]

@Fryuni @ktrz is there any progress made on this? Could I help out to push this along?

[Fryuni]

I haven't progressed on this since my comment on March, kinda left my hacky workaround there since It Just Works™️.

If you wanna replicate it, replace all the functions on sdk-trace-web that touch the navigator, the DOM or the window with either return null or return '' as appropriate for the type. Certainly those functions serve a purpose, but I haven't missed any functionality without them, so I think they are not even necessary for a Node's fetch.

[drewcorlin1]

Thanks for the context! I'll try to find a bit of time to work on this 😄

[drewcorlin1]

While I work on fixing this, I want to share what I ran into: In order to make sure that trace context (via the propogation headers) was shared across services, so that an HTTP request from client -> service A -> service B showed up as 1 trace, I had to do the following with the fetch instrumentation

// @ts-ignore
globalThis.location = {};
// @ts-ignore
globalThis.navigator = {};

registerInstrumentations({
  tracerProvider: provider,
  instrumentations: [
    // other instrumentations...,
    new FetchInstrumentation({
      propagateTraceHeaderCorsUrls: /.*/,
    }),
  ],
});

full snippet of my setup

import { hostname } from 'os';
import { context } from '@opentelemetry/api';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import { FetchInstrumentation } from '@opentelemetry/instrumentation-fetch';
import { Resource } from '@opentelemetry/resources';
import { BatchSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';

context.setGlobalContextManager(new AsyncHooksContextManager().enable());

const provider = new NodeTracerProvider({
  resource: new Resource({ [SemanticResourceAttributes.SERVICE_NAME]: hostname() }),
});

const otlpTraceExporter = new OTLPTraceExporter({ url: process.env.OPEN_TELEMETRY_DESTINATION_URL });
provider.addSpanProcessor(new BatchSpanProcessor(otlpTraceExporter));

// @ts-ignore
globalThis.location = {};
// @ts-ignore
globalThis.navigator = {};

registerInstrumentations({
  tracerProvider: provider,
  instrumentations: [
    // other instrumentations...,
    new FetchInstrumentation({
      propagateTraceHeaderCorsUrls: /.*/,
    }),
  ],
});

provider.register();

[sonewman]

another issue i want to add to this thread an issue i've encountered in the code, the _patchConstructor
https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts#L295-L405

it seems a conscious choice was made to explicitly ignore the second argument passed to the function if the first is an instanceof Request but that actually breaks functionality should you choose to supply RequestInit options in addition to a Request which a perfectly valid usecase and seems explicit in specification.

You can see this handled here in the undici (node native impl) https://github.com/nodejs/undici/blob/main/lib/fetch/index.js#L125-L137
the resulting request is the result of instantiating a new request with the options, which extends the request with the additional options.

it might be necessary for this to be pulled into a bug of it's own since this would also affect the web/browser version i would imagine.

[drewcorlin1]

#4063 Has now been merged, should this issue be closed? @sonewman may be best to open a new issue for what you're seeing?

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[mbrevda]

seems #4063 is only for the web (or am I misunderstanding)?

[Fryuni]

It makes the web SDK work when missing the web-only globals. So although it changes only the web SDK it should now work with Node (and others non-browser runtimes)

[mbrevda]

it should now work

Just tried it, it doesn't seem to be instrumenting thing I'd expect to see the way opentelemetry-instrumentation-fetch-node does

[david-luna]

There is a new instrumentation released a couple of weeks ago. This instrumentation works for undici but also for the fetch global Node.js API (since the API uses undici under the hood).
https://www.npmjs.com/package/@opentelemetry/instrumentation-undici

@ehtb I think this covers point 2 right? Could you give it a try?

[JacksonWeber]

@david-luna @ehtb Given the existence of the undici/native-fetch instrumentation, this issue can be closed.

[david-luna]

Closing the issue. @ehtb ping me or open a new issue if needed :)