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

feat: Allow passing of include or exclude list via module.register() #124

Merged
merged 9 commits into from
Jul 4, 2024

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jul 3, 2024

This PR adds the ability to pass an array of modules or URLs to include or exclude from wrapping via the data property passed as the third argument of module.register().

Since import-in-the-middle has issues that we don't know how to solve, it makes sense to allow a way to work around these issues so it can still be used.

@timfish timfish changed the title feat: Allow passing of include or exclude list via register feat: Allow passing of include or exclude list via module.register() Jul 3, 2024
hook.js Outdated Show resolved Hide resolved
hook.js Outdated Show resolved Hide resolved
hook.js Outdated Show resolved Hide resolved
@timfish timfish requested a review from AbhiPrasad July 4, 2024 22:38
@timfish timfish merged commit 381f48c into nodejs:main Jul 4, 2024
48 checks passed
@timfish timfish deleted the feat/include-exclude branch July 4, 2024 23:10
@vchirikov
Copy link

@timfish previously I patch the lib and use url.startsWith('node:') to include everything from node: only, I think it's very useful to allow pattern/regex entries in include/exclude, what do you think?

@timfish
Copy link
Contributor Author

timfish commented Jul 8, 2024

Hey @vchirikov. Yep, a good idea.

PRs are welcome. Just keep it non-breaking, add some tests and it will likely get merged and released quickly!

AbhiPrasad pushed a commit to getsentry/sentry-javascript that referenced this pull request Jul 15, 2024
Closes #12878

I added this feature to `import-in-the-middle` which was released in
v1.9.0:
- nodejs/import-in-the-middle#124

This PR changes the hook from `@opentelemetry/instrumentation/hook.mjs`
to `import-in-the-middle/hook.mjs` as it was only pasing though anyway
and the otel hook doesn't pass the `initialize` export.
@FabianFrank
Copy link

Really appreciate people trying to work through this for real and make the fix a proper library options. I tried using the exclude option to avoid wrapping the openai package like so:

register("import-in-the-middle/hook.mjs", import.meta.url, {
  parentURL: import.meta.url,
  data: {
    exclude: [
      // openai shim breaks with import in the middle, see https://github.com/openai/openai-node/issues/903
      "openai",
    ],
  },
})

That said, it does not seem to be enough. Only the openai import itself, i.e. the top level export of openai, is skipped by ITIM since the exclude is an exact match. I have to add code like for example

    if (parentURL.includes("/openai/")) {
      console.log("skip openai", parentURL, specifier)
      return result
    }

to resolve in hook.js to prevent wrapping all other files in openai and thus

Does someone actually have a working example where openai breaks due to ITIM and is fixed by setting this option, but all other modules still get intercepted?

@timfish
Copy link
Contributor Author

timfish commented Jul 20, 2024

There has already been a request to allow regular expressions for exclude/include which would help here. I'll probably submit a PR for this over the weekend.

Until then, you could use include and only list the libraries that you're instrumenting.

I think #146 is the long term solution for most.

Sign up for free to 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.

5 participants