fix(instrumentation-fs): allow realpath.native and realpathSync.native #1332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JakubKoralewski
changed the title
|
Will this also fix the issue with using `fs/promises? |
|
No, sorry |
blumamir
reviewed
Jan 2, 2023
change indexFs return types and object properties of return
previously ['realpath', 'native'] now 'realpath.native' also moved exported yet private functions to utils file fixed tests to follow these changes refactored patching code to function with added comment
|
@blumamir sorry for the delay, I tried to address all your suggestions let me know if I missed anything |
|
Hi @JakubKoralewski , I apologize for the this PR is open. Would it be possible for you to jump on a quick zoom call to discuss this? I searched for your name in CNCF slack but couldn't find you there. |
blumamir
approved these changes
Feb 8, 2023
blumamir
reviewed
Feb 8, 2023
…trib into fix-realpathsync-native # Conflicts: # plugins/node/instrumentation-fs/src/instrumentation.ts # plugins/node/instrumentation-fs/src/types.ts
JakubKoralewski
commented
Feb 8, 2023
blumamir
approved these changes
Feb 12, 2023
|
@JakubKoralewski please fix the CI and I'll merge |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1332 +/- ##
==========================================
+ Coverage 96.08% 96.21% +0.12%
==========================================
Files 14 15 +1
Lines 895 951 +56
Branches 192 196 +4
==========================================
+ Hits 860 915 +55
- Misses 35 36 +1
|
|
@JakubKoralewski - I pushed a few commits to your branch to attempt to fix the build and lint issues. Please take a look and approve if you are ok with them |
Merged
mhassan1
added a commit
to mhassan1/opentelemetry-js-contrib
that referenced
this pull request
Feb 17, 2023
Abinet18
pushed a commit
to Abinet18/opentelemetry-js-contrib
that referenced
this pull request
Feb 25, 2023
open-telemetry#1332) * test(instrumentation-fs): add failing test for bug * fix(instrumentation-fs): allow .native fs funcs * refactor(instrumentation-fs): indexFs change indexFs return types and object properties of return * refactor(instr-fs): use string for two level fns previously ['realpath', 'native'] now 'realpath.native' also moved exported yet private functions to utils file fixed tests to follow these changes refactored patching code to function with added comment * refactor(instrumentation-fs): remove memberToDisplayName * refactor(instrumentation-fs): simplify splitTwoLevels signature * fix(instrumentation-fs): lint * fix(instrumentation-fs): lint (typeof fs) * fix(instrumentation-fs): test types * chore: fix compile and lint * fix(fs): exclude undefined in generic type * test(fs): skip fs functions in node14 which were added later --------- Co-authored-by: Amir Blum <amirgiraffe@gmail.com> Co-authored-by: Amir Blum <amir@aspecto.io>
haddasbronfman
added a commit
that referenced
this pull request
Feb 27, 2023
* fix(instrumentation-fs): fix instrumentation of `fs/promises` * Revert "fix(instrumentation-fs): fix instrumentation of `fs/promises`" This reverts commit 90d9f0d. * fix(instrumentation-fs): fix instrumentation of `fs/promises` * chore(instrumentation-fs): fix lint error * chore(instrumentation-fs): hard-code `R_OK` value for node 14 * chore(instrumentation-fs): fix supported version for `fs/promises` * chore(instrumentation-fs): incorporate changes from #1332 * chore(instrumentation-fs): consolidate common test utilities --------- Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi! First PR here 🎅 Let me know if I did something wrong.
Which problem is this PR solving?
Fixes #1315
Short description of the changes
Maybe I overengineered it a little by adding ['realpath', 'native'], and ['realpathSync', 'native'] to
CALLBACK_FUNCTIONSandSYNC_FUNCTIONSrespectively, instead of just adding if statements, but I guess it's future-proof. The crux of the matter is the call toObject.assign(rv, original);that assigns properties from the original function onto the new one, thenativeproperty gets assigned, but the function specific stuff is read-only so the function itself doesn't get overwritten.