@opentelemetry/instrumentation-fs: Node service crashes when 'native' function property is used

[arnoreyvar]

What version of OpenTelemetry are you using?

API	Stable Packages	Experimental Packages
1.3.x	1.8.x	0.34.x

What version of Node are you using?

16.18.1

What did you do?

Updated OTel dependencies this included the addition of instrumentation-fs. The service instrumented is using node auto instrumentation with all enabled.
The service imports fs-extra@10.0.1 , which internally extends fs evaluating fs.realpath.native.name in the process.
Starting the service caused a crash.

Re-tested by calling fs.realpath.native(..args) directly instead of using fs-extra and this also caused a crash.

What did you expect to see?

Service up and running being instrumented with all automatic instrumentation applied.

What did you see instead?

Service crashed with following exception:
{ "error": { "message": "Cannot read properties of undefined (reading 'name')", "name": "TypeError" }, "level": "error", "message": "Uncaught exception", "name": "root", "stack": "TypeError: Cannot read properties of undefined (reading 'name') at exports.fromCallback (/services/node_modules/universalify/index.js:15:26) at Object.<anonymous> (/services/node_modules/fs-extra/lib/fs/index.js:57:27) at Module._compile (node:internal/modules/cjs/loader:1155:14) .... }
If called directly the message would be along the lines of : fs.realpath.native is not a function

Additional context

The issue seems to be that shimmer used for patching does not seem to include function properties like native when patching. It exists on the __original property but not in the root. This might be known as it was previously discussed in other issue: remove shimmer, implement new one #618.

I worked around this issue on my end first by disabling instrumentation-fs but then noticed that fs-extra@10.1.0 has a workaround for this as well.

[dyladan]

@rauno56 can you take a look at this since you wrote the fs instrumentation? I think this might be caused by the instrumentation module also.

[Flarna]

I think that fs.realpath.native needs to be also wrapped and set on the wrapped fs.realpath function. Same for realpathSync. This is similar as #1222 where the promisify.custom was wrapped away.

[puckpuck]

The OpenTelemetry demo frontend, created with Next.js has a similar issue when we tried to upgrade the SDKs to the latest version (1.7/0.33 -> 1.8/0.34).

See this comment for more details.

[diestrin]

Coming from SigNoz, I was trying to get my node app to be monitored following this guide https://signoz.io/docs/instrumentation/express/#using-the-all-in-one-auto-instrumentation-library

In order to make the monitoring work I had to change

const { getNodeAutoInstrumentations } = require('@opentelemetry/auto-instrumentations-node')

const sdk = new opentelemetry.NodeSDK({
  traceExporter,
  instrumentations: [getNodeAutoInstrumentations()],
  resource,
})

With a manual list of instrumentations ignoring FsInstrumentation, like this:

const instrumentations = [
  new DnsInstrumentation(),
  // new FsInstrumentation(),
  new GenericPoolInstrumentation(),
  new GraphQLInstrumentation(),
  new HttpInstrumentation(),
  new KnexInstrumentation(),
  new KoaInstrumentation(),
  new LruMemoizerInstrumentation(),
  new MemcachedInstrumentation(),
  new PgInstrumentation(),
  new RouterInstrumentation(),
]

But I guess I'm not getting file system events being tracked?

[joadr]

@diestrin You could also disable the fs instrumentation alone

const sdk = new opentelemetry.NodeSDK({
  traceExporter,
  instrumentations: [getNodeAutoInstrumentations({
    '@opentelemetry/instrumentation-fs': {enabled: false}
  })],
  resource: new Resource({
    [SemanticResourceAttributes.SERVICE_NAME]: 'profile',
  }),
});