Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@opentelemetry/instrumentation-fs` fails to wrap fs promises #1305

Closed
weyert opened this issue Nov 25, 2022 · 9 comments · Fixed by #1375
Closed

@opentelemetry/instrumentation-fs` fails to wrap fs promises #1305

weyert opened this issue Nov 25, 2022 · 9 comments · Fixed by #1375
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies

Comments

@weyert
Copy link

weyert commented Nov 25, 2022

What version of OpenTelemetry are you using?

What version of Node are you using?

18.12.1

What did you do?

I installed Opentelemetry and use the auto-instrumentation meta package via node-sdk to instrument my node backend application but when starting up the application thrown a runtime error

What did you expect to see?

Successfully running my backend application without throwing errors

What did you see instead?

/Users/xxxx/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 (/Users/xxxx/node_modules/@opentelemetry/instrumentation-fs/build/src/instrumentation.js:53:68)

Looks like the fs.promises is undefined in my case which causes the wrapping step to fail. I am not sure why fs.promises is undefined. Maybe the supportsPromises needs to be adapted so it also checks whether fs.promises is defined?

E.g.
const supportsPromises = parseInt(process.versions.node.split('.')[0], 10) > 8 && fs.promises

I can't really think of a reason why fs.promises could be undefined. Could a library maybe override it? If so, what would be a good way to detect that in Node?

Additional context

I am running Express

@weyert weyert added the bug Something isn't working label Nov 25, 2022
@Flarna
Copy link
Member

Flarna commented Nov 26, 2022

Which ohter modules have you loaded in your app? In special that ones loaded before the instrumentation would be insteresting.

Or can you even share a stripped down reproducer?

@bryanchriswhite
Copy link

bryanchriswhite commented Nov 28, 2022

I'm experiencing what appears to be the same error in a NestJS environment. Likewise, I'm trying to use auto-instrumentations in node:

/app/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 (/app/node_modules/@opentelemetry/instrumentation-fs/build/src/instrumentation.js:53:68)
    at FsInstrumentation._onRequire (/app/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js:79:35)
    at /app/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js:133:29
    at /app/node_modules/@opentelemetry/instrumentation/build/src/platform/node/RequireInTheMiddleSingleton.js:54:27
    at Module.Hook._require.Module.require (/app/node_modules/require-in-the-middle/index.js:175:32)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/app/node_modules/rotating-file-stream/dist/cjs/index.js:8:20)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
error Command failed with exit code 1.

For context, the codebase I'm attempting to add instrumentation to is not one I'm very familiar with. I had previously, successfully, attempted this same thing in another nestjs codebase (of similar size and complexity and with some code in common) recently and did not experience this issue.

I attempted to reproduce the issue in a new nest app but have been unsuccessful as of yet.

(Note: bring your own open-telemetry collector, etc.)

@modosc
Copy link

modosc commented Nov 28, 2022

using node 18.12.0 i see the issue with @opentelemetry/auto-instrumentations-node@npm:^0.35.0 but reverting to @opentelemetry/auto-instrumentations-node@npm:^0.34.0 fixes it

@bryanchriswhite
Copy link

Nice one @modosc - that resolved it for me as well!

@Flarna
Copy link
Member

Flarna commented Nov 29, 2022

Created open-telemetry/opentelemetry-js#3448 which includes a simple reproducer and some hints where the problem is located.
replacing any occurrence of require("fs/promises") is a workaround but most likely hard to do as this is spread across a lot npm modules.

@dyladan dyladan added the priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies label Nov 30, 2022
@domharrington
Copy link

This is still broken in @opentelemetry/auto-instrumentations-node@0.36.0. Downgrading to v0.34.0 seems to do the trick. Also seeing lots of these in the logs, which I assume are somewhat related:

no original function cpSync to wrap
no original function cp to wrap
no original function cp to wrap

@Diamondlord
Copy link

I reproduced the issue in this repository
I hope it will help as open-telemetry/opentelemetry-js#3451 still didn't solve the problem

@mhassan1
Copy link
Contributor

This should have been resolved by open-telemetry/opentelemetry-js#3451. If you upgrade to @opentelemetry/auto-instrumentations-node@0.36.1 or higher, you will no longer see the error.

Proper instrumentation of fs/promises will be addressed by #1375.

@blumamir blumamir linked a pull request Feb 13, 2023 that will close this issue
@Diamondlord
Copy link

@opentelemetry/auto-instrumentations-node@0.36.2 is working in my case

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants