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

Upgrade @vercel/nft to 0.27.4 to include ESM module.register hooks #2703

Open
s1gr1d opened this issue Sep 4, 2024 · 11 comments
Open

Upgrade @vercel/nft to 0.27.4 to include ESM module.register hooks #2703

s1gr1d opened this issue Sep 4, 2024 · 11 comments

Comments

@s1gr1d
Copy link

s1gr1d commented Sep 4, 2024

Environment

ESM

Reproduction

Link: https://github.com/s1gr1d/nitro-sentry-iitm

This is how import-in-the-middle (IITM) looks like in .output/server/node_modules:

Without resolution (uses @vercel/nft 0.26.x) With @vercel/nft package resolution to 0.27.4
image image

Describe the bug

Previous versions of @vercel/nft do not include ESM module.register hooks so the @opentelemetry/instrumentation/hook.mjs is not included, which leads to errors when starting the server if opentelemetry or packages that depend on it (like Sentry) are used.


@vercel/nft released a new version with the fix, but as it is still a 0 major version, the caret (^) does not work like usual. With 0 major versions, only the patch is automatically updated, but not the minor (npm source).

The current release of nitro depends on ^0.26.5 and the fix is included in 0.27.4 (another minor version).

It would be awesome to release a new nitro version (minor or patch), which includes this @vercel/nft minor version upgrade 🙌

Additional context

Issue Reference:

Logs

No response

@pi0
Copy link
Member

pi0 commented Sep 5, 2024

We have already upgraded to 0.27 range in v2/main branches but since v2 branch has some risky changes that require more ecosystem testing, unfortunately, there is no immediate release planned.

In the meantime, it is possible to use nightly release channel to receive the update.

@s1gr1d
Copy link
Author

s1gr1d commented Sep 5, 2024

Thanks for this information :) Would it be possible to create a patch release (nitro 2.9.8) with just this nft change? The current 0.27 range is fine as well, but the current version 2.9.7 only includes 0.26. I understand that this might be a big request but with this update people would be able to use OTel instrumentation with nitro (OTel uses import-in-the-middle).

@pi0
Copy link
Member

pi0 commented Sep 5, 2024

We can... Can you please help to make a minimal nitro reproduction for the issue with open telemetry dependency? I remember last time there were more issues specially on nodeless (edge) platforms this way at least we can make sure this releases fully solves the problem.

@s1gr1d
Copy link
Author

s1gr1d commented Sep 5, 2024

I updated the issue and created a minimal reproduction with Sentry as Sentry uses module.register to add import-in-the-middle to make it work with OTel: https://github.com/getsentry/sentry-javascript/blob/a8e5f593b3341f90cf9aafaaa1f6eb1e0da7545e/packages/node/src/sdk/initOtel.ts#L68

@andreiborza
Copy link

Hi @pi0, any decisions on this? This also holds up otel instrumentation for vinxi for example.

@pi0
Copy link
Member

pi0 commented Sep 12, 2024

Thanks for reproduction @s1gr1d it helped me to understand the better of usage.

@andreiborza We are working on the v2 branch to be stable (we also already asked both Nuxt and solid/vinxi teams since last week to review the nightly channel) as soon as we are sure of stability will issue a new release, I hope (but don't promise) by early next week at latest for it to happen.

My understanding is that import-in-the-middle is an experimental feature (and it is for Node.js targets only). In the meantime (while I fully understand the inconvenience) you can offer users to opt into the nightly channel or manually add the resolution of vercel/nft.

On a relevant topic, if you are interested I would love to have better support of Sentry for Nitro (both Node.js and Edge) (long-time user!), i will contact you if you are intrested.

@s1gr1d
Copy link
Author

s1gr1d commented Sep 12, 2024

Hy, yeah adding a resolution is the only current workaround. And it is good to hear that another release is coming soon! Currently, the Nuxt and SolidStart SDKs are in alpha/beta and it would be nice for developers to use it without the resolution once the proper release of the SDK comes out.

And we are about to start working on a Nitro instrumentation: getsentry/sentry-javascript#13670
It's great to hear that we can reach out to you!

@pi0
Copy link
Member

pi0 commented Sep 12, 2024

Sounds exciting. I could not find any contact in X. Would you mind to drop me a DM in discord (@pi0) or X (@_pi0_)? 🙏🏼

@cjpearson
Copy link
Contributor

We've been using a manual OpenTelemetry instrumentation for our projects and the idea of having an officially supported version is exciting!

Have you given any thought to supporting Native Instrumentation within the Nitro package itself? (Similar to NextJS) With an external package it can be difficult to inject the necessary code at the right points. For example, in our instrumentation package we have to use a nitro plugin where we get a handle on the h3 app and patch the nitro router to wrap each request in a handle. It works, but as with all code that depends on internals there is the risk of future breakage.

https://opentelemetry.io/docs/concepts/instrumentation/libraries/

@pi0
Copy link
Member

pi0 commented Sep 12, 2024

@cjpearson Actually yes one of the reasons I am thinking of nitro-native support is to have a cross-platform instrument system. I don't prefer the complexity of open telemetry Node.js SDK either.

@cjpearson
Copy link
Contributor

Oh cool. Are you open to PRs in this direction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants