Skip to content

feat: Instrument calls to libraries - #128

Closed
zermelo-wisen wants to merge 0 commit into
mainfrom
feat/instrument-calls-to-libraries
Closed

feat: Instrument calls to libraries#128
zermelo-wisen wants to merge 0 commit into
mainfrom
feat/instrument-calls-to-libraries

Conversation

@zermelo-wisen

Copy link
Copy Markdown
Contributor

Fixes#83

@dividedminddividedmind left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you're on the right track. Couple of comments:

  • Will this work for ESM? I'm worried the whole approach will need to be different. Unless we can use the same trick for CJS-loading and hoping the hooked module will be cached across the two loaders...
  • The Error approach is a no-go; it will have an unacceptably massive performance hit. Also, it's maybe a bit too complicated; appmap-ruby skips tracing if current_package.shallow? && last_package == current_package (where package is just an appmap.ymlpackage: entry), which is extremely simple and works well in practice. This also allows shallow mode to work for user (eg. path:) packages, a feature occasionally useful.

Comment threadsrc/PackageMatcher.ts
Comment threadsrc/hooks/standardLibs.ts Outdated
@@ -0,0 +1,152 @@
import assert from "node:assert";

import { ESTree } from "meriyah";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please try to use import type when applicable to make it clear when code is being imported vs. just types.

Comment threadsrc/requireHook.ts Outdated
Comment threadsrc/hooks/standardLibs.ts Outdated
@dividedmind

Copy link
Copy Markdown
Collaborator
  • Will this work for ESM? I'm worried the whole approach will need to be different. Unless we can use the same trick for CJS-loading and hoping the hooked module will be cached across the two loaders...

BTW, I think if ESM support is going to be non-trivial it's ok to just do this for CJS for now and open a separate issue to add ESM support later.

@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch 6 times, most recently from b3e4888 to 970d733CompareMarch 29, 2024 07:58
@zermelo-wisen

Copy link
Copy Markdown
ContributorAuthor
  • Will this work for ESM? I'm worried the whole approach will need to be different. Unless we can use the same trick for CJS-loading and hoping the hooked module will be cached across the two loaders...

BTW, I think if ESM support is going to be non-trivial it's ok to just do this for CJS for now and open a separate issue to add ESM support later.

It was trivial for built-ins. For third party libs I managed to do it by implementing resolve function in our custom loader.

@zermelo-wisen
zermelo-wisen marked this pull request as ready for review March 29, 2024 12:44
@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch from 46de9c8 to 988deebCompareMarch 30, 2024 10:30
@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch from 988deeb to 6132680CompareApril 15, 2024 10:16
@zermelo-wisen
zermelo-wisen marked this pull request as draft April 16, 2024 07:36
@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch 2 times, most recently from 1c5375a to b13d2e4CompareApril 16, 2024 08:52
@zermelo-wisen
zermelo-wisen marked this pull request as ready for review April 16, 2024 09:01
Comment threadsrc/hooks/libraries.ts Outdated
Comment threadsrc/hooks/libraries.ts Outdated
Comment threadsrc/recorder.ts Outdated
Comment threadsrc/recorder.ts Outdated
@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch 5 times, most recently from 6a1f9e5 to 153af06CompareApril 24, 2024 12:55
@zermelo-wisen
zermelo-wisenforce-pushed the feat/instrument-calls-to-libraries branch from efa6a7b to a8b9fbfCompareAugust 5, 2024 19:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow instrumenting calls to libraries

2 participants

@zermelo-wisen@dividedmind