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

Add fs module support #777

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Add fs module support #777

merged 1 commit into from
Nov 3, 2022

Conversation

luismiramirez
Copy link
Member

@luismiramirez luismiramirez commented Oct 27, 2022

All fs module usage is now automatically instrumented.

Tests are in the express-redis sample application.

Automatic instrumentation is manually suppresed in the test span processor because it uses the fs module to handle the spans.json file. It is done using the supressTracing helper provided by OpenTelemetry core library.

Part of #763.

@unflxw
Copy link
Contributor

unflxw commented Oct 28, 2022

I have looked into the issue where the Prisma test applications fail to boot when the FS instrumentation is added.

TL;DR: We gotta wait for a release containing the changes in this PR to be published (should be included in the next release) and update the @opentelemetry/instrumentation-fs package to that version.

The core of the issue is that the Prisma client calls util.promisify(fs.exists). It turns out that fs.exists is a bit unusual. It's deprecated by the Node team, because its callback signature does not follow the standard (err, result) => { ... } signature of Node callbacks. Instead, since fs.exists can't fail, its signature is just (result) => { ... }.

Since util.promisify expects the first argument of the callback to be an error, this means that promisifying fs.exists would result on the promise always being rejected, with the actual result being the rejection error.

This is consistent with the error message we see:

error2.clientVersion = this.client._clientVersion;
TypeError: Cannot create property 'clientVersion' on boolean 'true'

Prisma is trying to set the clientVersion property on the error object it received, but the error object isn't an object, but a boolean true, the return value of fs.exists, turned into the rejection value of a promise by util.promisify.

Now, the reason why this bug only happens for the instrumented version of the function is that the original fs.exists provides a custom promisified implementation of itself. If there is an util.promisify.custom symbol as a property of the function, util.promisify will use its value as the promisified implementation. The original fs.exists provides this, but the OpenTelemetry-instrumented version of the function doesn't.

Or rather, it doesn't yet, as there's a merged PR implementing this custom behaviour for the instrumented version of the function. This should fix our problems; we just need to wait for a release including the change.

I have not been able to verify the actual fix directly (npm's "install from GitHub" does not play well with monorepos) but I have confirmed that manually excluding fs.exists from being instrumented by commenting the relevant line of code fixes the issue, so I expect the fix above to also fix the issue.

Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, but let's not merge this until there's an updated version of the @opentelemetry/instrumentation-fs library, or else it will break for Prisma users.

All fs module usage is now automatically instrumented.

Tests are in the express-redis sample application.

Automatic instrumentation is manually suppresed in the test span
processor because it uses the fs module to handle the spans.json file.
It is done using the `supressTracing` helper provided by OpenTelemetry
core library.
@unflxw unflxw self-requested a review November 3, 2022 13:36
@luismiramirez luismiramirez merged commit 48e3958 into main Nov 3, 2022
@luismiramirez luismiramirez deleted the fs-instrumentation branch November 3, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants