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: Override import-in-the-middle code to only instrument specified modules #12167

Closed
wants to merge 1 commit into from

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented May 22, 2024

This is a temporary workaround for import-in-the-middle incompatibilities with many libraries (#12059, #12154).

By default, import-in-the-middle analyses and proxies every import but it currently can't handle some cases.

This PR modifies the code in import-in-the-middle/hook.mjs via another loader hook. This modified code only resolves via import-in-the-middle if the module matches a supplied list. By default, this list contains all the built-in auto-instrumentations for @sentry/node and users can add to this list via options._experiments.modulesToInstrument:

Sentry.init({
  dsn: '__DSN__',
  _experiments: {
    modulesToInstrument: ['some-other-module'],
  }
});

);
});
}
registerEsmModuleHooks([
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, will this prevent other instrumentation from working, if people add their own OTEL instrumentation?

'pg',
'@nestjs/core',
'prisma',
...(options._experiments?.modulesToInstrument || []),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mydea I added this so that users can include other modules

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another more complex approach might be to catch the file-not-found errors and revert to the default resolve/load.

Copy link
Member

Choose a reason for hiding this comment

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

catch the file-not-found errors and revert to the default resolve/load.

I assume this is pretty expensive right?

Copy link
Collaborator Author

@timfish timfish May 22, 2024

Choose a reason for hiding this comment

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

Oh, it looks like it's not possible to fix this way. The errors are coming from the load hook while it's fetching files to collect exports.

Looking through the code, import-in-the-middle simply doesn't support re-exporting exports from external modules (ie. export * from 'some-external-module'). It doesn't appear to support resolving anything that isn't relative to the current file and it needs significant work to fix this. For example, to fix this for mono-repositories too, it will need to traverse up though multiple the node_modules directories searching for the module 🙁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although having written that... I'm going to try a couple more things...

Copy link
Collaborator Author

@timfish timfish May 22, 2024

Choose a reason for hiding this comment

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

Ok, so it's possible to call resolve from within load to find modules that cannot be found relative to the current file. I suspect this is how it can be fixed in import-in-the-middle so I'll look into doing a PR there.

I could create a hacky PR similar the this PR but using what I've learnt above but it doesn't fix all the issues and it's almost too hacky to consider actually publishing with the SDK. Once I've got an import-in-the-middle PR up, this might change.

The issue with the @discord packages at least is that every package has a version export which all get combined via export * from and results in:
image

This comment was marked as outdated.

@AbhiPrasad
Copy link
Member

I'd like us to try identifying and wrapping the the file-not-found errors first. If that really doesn't work I'm fine with settling with an allow-list, but this feels super fragile to me.

@timfish
Copy link
Collaborator Author

timfish commented May 24, 2024

I'm going to close this for now since this is likely too fragile to ship!

@timfish timfish closed this May 24, 2024
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.

3 participants