@opentelemetry/instrumentation version 0.34.0 breaks @opentelemetry/instrumentation-fs

[Flarna]

What happened?

Steps to Reproduce

"use strict";
const { FsInstrumentation } = require("@opentelemetry/instrumentation-fs");
const { registerInstrumentations } = require("@opentelemetry/instrumentation");
registerInstrumentations({
    instrumentations: [
        new FsInstrumentation()
    ]
});
const fs = require("fs/promises");

Expected Result

no crash

Actual Result

C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation-fs\build\src\instrumentation.js:53
                        if (instrumentation_1.isWrapped(fs.promises[fName])) {
                                                                   ^

TypeError: Cannot read properties of undefined (reading 'access')
    at InstrumentationNodeModuleDefinition.patch (C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation-fs\build\src\instrumentation.js:53:68)
    at FsInstrumentation._onRequire (C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\instrumentation.js:79:35)
    at C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\instrumentation.js:133:29
    at C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\RequireInTheMiddleSingleton.js:54:27
    at Hook._require.Module.require (C:\work\otel-contrib-1305\node_modules\require-in-the-middle\index.js:175:32)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (C:\work\otel-contrib-1305\r.js:9:12)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)

Node.js v18.12.1

Additional Details

This is caused by #3161.
Splitting the module name on / here is not correct for built in modules like fs/promises.
As a result the instrumentation-fs hook for fs is called but gets the exports from fs/promises.

OpenTelemetry Setup Code

"use strict";
const { FsInstrumentation } = require("@opentelemetry/instrumentation-fs");
const { registerInstrumentations } = require("@opentelemetry/instrumentation");
registerInstrumentations({
    instrumentations: [
        new FsInstrumentation()
    ]
});
const fs = require("fs/promises");

package.json

{
  "name": "otel-contrib-1305",
  "version": "1.0.0",
  "main": "r.js",
  "scripts": {
    "start": "node r.js"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@opentelemetry/api": "1.3.0",
    "@opentelemetry/instrumentation-fs": "0.6.0"
  }
}

Relevant log output

C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation-fs\build\src\instrumentation.js:53
                        if (instrumentation_1.isWrapped(fs.promises[fName])) {
                                                                   ^

TypeError: Cannot read properties of undefined (reading 'access')
    at InstrumentationNodeModuleDefinition.patch (C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation-fs\build\src\instrumentation.js:53:68)
    at FsInstrumentation._onRequire (C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\instrumentation.js:79:35)
    at C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\instrumentation.js:133:29
    at C:\work\otel-contrib-1305\node_modules\@opentelemetry\instrumentation\build\src\platform\node\RequireInTheMiddleSingleton.js:54:27
    at Hook._require.Module.require (C:\work\otel-contrib-1305\node_modules\require-in-the-middle\index.js:175:32)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (C:\work\otel-contrib-1305\r.js:9:12)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)

Node.js v18.12.1

[Flarna]

fyi @mhassan1

[mhassan1]

I will take a look at this. I had added a test case especially for fs/promises, but I guess it was not a good test case.

[mhassan1]

Here is what I have discovered:

1. In @opentelemetry/instrumentation@0.33.0, require('fs/promises') was actually not being instrumented on its own without an additional call to require('fs'). This is because RequireInTheMiddle(['fs'], { internals: true }, onRequire) does not call onRequire for fs/promises (this could be considered an unexpected behavior in RITM, since it will call onRequire for sub-path exports in non-core modules).
2. In @opentelemetry/instrumentation@0.34.0, require('fs/promises') causes an attempt to instrument fs/promises, but this !baseDir check causes the patch function to be called with exports of fs/promises, even though it is expecting the exports of fs. A potential fix would be to add an if (module.name === name) check there, since we only want to call the patch function for fs and not for fs/promises. This would return us to the prior state, where we are not calling onRequire for fs/promises.

I will put up a PR to restore the prior behavior.

[Flarna]

I think (1) is actually a bug/missing functionality in the fs instrumentation. it should register a hooks for "fs/promises" and ensure that it is able to handle that either or both hooks are called in arbitrary order.

edit: created open-telemetry/opentelemetry-js-contrib#1313