-
Notifications
You must be signed in to change notification settings - Fork 544
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
feat: implement auto-instrumentation for fs
#872
Conversation
Codecov Report
@@ Coverage Diff @@
## main #872 +/- ##
=======================================
Coverage 95.91% 95.91%
=======================================
Files 13 13
Lines 856 856
Branches 178 178
=======================================
Hits 821 821
Misses 35 35 |
fs
d70e735
to
ca4c3ea
Compare
d4d4ed8
to
ec00508
Compare
…trib into feat/instrumentation-fs
Thanks a lot @vmarchaud and @blumamir. Really valuable feedback! I addressed everything in one way or another and took the liberty to resolve all the threads I found trivial and resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you decide on the suppressing. I'm going to approve, but not merge until you've made that decision.
@@ -0,0 +1,67 @@ | |||
{ | |||
"name": "@opentelemetry/instrumentation-fs", | |||
"version": "0.27.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why starting at version 27?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of a habit. We used to version-match packages. I guess it can now be 0.1.0
, which makes a lot more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work. Was waiting for instrumentation for this package, thanks for taking it 🤩
@rauno56 you can merge this if you're happy with it. I think with release-please updated we should no longer need to manually add it to the manifest. |
…metry-js-contrib into feat/instrumentation-fs
I think that's a good point. I'll change that to be strict for now. I'll convert the PR to draft since I will be fixing this next week. Note to myself: test to make sure user calls in callbacks aren't suppressed. |
Which problem is this PR solving?
This implements partial auto-instrumentation for the native
fs
module.Short description of the changes
This instruments most of the
fs
functions. Full list inconstants.ts
file.Provides two hooks:
createHook: (functionName: string, info: { args: ArrayLike<unknown> }) => boolean;
endHook: (functionName: string, info: { args: ArrayLike<unknown>; span: api.Span }) => void;
Returning
false
fromcreateHook
skips creating the trace.Checklist
Rannpm run test-all-versions
for the edited package(s) on the latest commit if applicable.