SDK fails in ESM mode in combination with openai

[Xhale1]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.8.0

Framework Version

Node 20.14.0

Link to Sentry event

https://trainwell.sentry.io/issues/5463070600/?project=6067364&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=14d&stream_index=1

SDK Setup

Sentry.init({
  dsn: "[REDACTED]",
});

Steps to Reproduce

1. Add openai package
2. Instrument using instrumentation.js file
3. Run with node --import ./dist/instrumentation.js dist/index.js

The addition of the following code is what triggers the issue:

import OpenAI from "openai";

const openAI = new OpenAI({
  apiKey: "[REDACTED]",
});

Expected Result

App builds with sentry instrumentation and no errors.

Actual Result

I receive the following error:

TypeError: API.Completions is not a constructor
    at new OpenAI (file:///[REDACTED]/node_modules/.pnpm/[email protected]/node_modules/openai/index.mjs:46:28)
    at file:///[REDACTED]
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5)

This might be related to #12237 however it appears to be a unique issue unrelated to initialization.

[Lms24]

Hey @Xhale1 thanks for reporting!

Just to rule something out: Which version of openai are you using? It seems there were related issues in an older version. Thanks!

[hudovisk]

Same issue here, using openai: 4.47.3

"openai": "^4.47.3",

[Xhale1]

Thanks for checking in! Just confirmed that my issue exists with version 4.49.1 which appears to be the latest offered by openai.

Let me know if a reproduction repo would help

[Lms24]

A reproduction would be greatly appreciated, thanks :)

[Xhale1]

This is my first time making an issue reproduction repo, let me know if it works for you: https://github.com/Xhale1/sentry-openai

[Lms24]

Hmm yeah, I can reproduce this myself, thanks for the excellent repro app!

I already tried switching the import style to namespace or named import but it doesn't matter. Looks like for some reason API.Completions within the OpenAI package is undefined rather than a class. My bet is that import-in-the-middle plays a role here. Also I wonder if IITM doesn't handle namespace imports within node_modules correctly. Not sure though.

Update: Narrowed it down to the import * as API from "./resources/index.mjs"
statement in /[email protected]/node_modules/openai/index.mjs. For some reason, this doesn't include Completions.

@timfish when you have a minute, would you mind taking a look?

[timfish]

It looks like openai/resources/index.mjs exports two different Completions classes, one for /completions and one for /chat/completions.

For import-in-the-middle we deduced that for ESM, duplicate named exports were not exported at all but that doesn't appear to be the case here.

[NatoBoram]

I made a minimal reproduction here:

• 🧪 TypeError: API.Completions is not a constructor NatoBoram/bug-report-sentry#9

It shows that all you have to do to make your app crash is new OpenAI({ apiKey: OPENAI_API_KEY }) when both Sentry and OpenAI are installed and Sentry is preloaded with --import @sentry/node/preload.

[timfish]

I've opened a PR for import-in-the-middle to fix this:
nodejs/import-in-the-middle#103

[timfish]

This should hopefully have been fixed by the numerous PRs recently merged at import-in-the-middle.

While we wait for this to be released, there is a patch available that combines all the fixes. If anyone can confirm this patch fixes this issue that would be super helpful!

[danilofuchs]

@timfish from my local testing, the patch seems to address the issue!

[Xhale1]

Confirming that that patch also appears to be working for us, thank you!

[andreiborza]

hey @Ivan-juni we are still waiting on #13139 to be fixed for this.

[Ivan-juni]

thanks @andreiborza
would be super helpful, can't use sentry just because of this error

[timfish]

The workaround for this issue for now is to exclude openai from being instrumented via the registerEsmLoaderHooks init option:

import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: '__DSN__',
  registerEsmLoaderHooks: { exclude: [/openai/] },
})

If you're using @sentry/node/preload you'll need to create your own preload file to pass this option:
preload.mjs

import * as Sentry from '@sentry/node';

Sentry.preloadOpenTelemetry({
  registerEsmLoaderHooks: { exclude: [/openai/] },
})

node --import ./preload.mjs ./my-app.mjs

[nwalters512]

I landed here via this hint:

sentry-javascript/packages/node/src/types.ts

Lines 94 to 97 in a6fda4c

	* Whether to register ESM loader hooks to automatically instrument libraries.
	* This is necessary to auto instrument libraries that are loaded via ESM imports, but it can cause issues
	* with certain libraries. If you run into problems running your app with this enabled,
	* please raise an issue in https://github.com/getsentry/sentry-javascript.

I'm not sure if Sentry or openai changed, but this is the latest error message:

TypeError: getDefaultAgent is not a function
    at OpenAI.buildRequest (file:///path/to/node_modules/openai/core.mjs:208:66)
    at OpenAI.makeRequest (file:///path/to/node_modules/openai/core.mjs:279:44)

I can confirm that registerEsmLoaderHooks: { exclude: [/openai/] } works as a temporary fix. I'll share yet again how frustrating Sentry v8 has been. Dropping an error-reporting library into my application shouldn't cause seemingly-random errors like this!

[lforst]

@timfish What is the status here again? IIRC we couldn't come up with a solution for the export pattern that openai uses in their package right? Or did I miss something.

Other than that, when #13139 is merged, people should be able to set registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true } to escape any random other packages from being patched.

I'll share yet again how frustrating Sentry v8 has been. Dropping an error-reporting library into my application shouldn't cause seemingly-random errors like this!

@nwalters512 Fully acknowledged! I hope you believe me when I say we are doing everything in our power to improve this situation. We took partial ownership of IITM in the Node.js org and we are regularly contributing upstream to OTEL. This is something not only improving the Sentry user experience but improves the entire ecosystem. These are not easy problems to solve (no excuse but to give some insight)! I hope you understand.

[timfish]

IIRC we couldn't come up with a solution for the export pattern that openai uses in their package right? Or did I miss something.

I seem to remember it being caused by this issue which is a fundamental problem with import-in-the-middle.

[timfish]

v8.29.0 of the Node SDK adds the new onlyIncludeInstrumentedModules option to registerEsmLoaderHooks.

import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: '__PUBLIC_DSN__',
  registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true },
});

When set to true, import-in-the-middle will only wrap ESM modules that are specifically instrumented by OpenTelemetry plugins. This is useful to avoid issues where import-in-the-middle is not compatible with some of your dependencies.

This feature will only work if you Sentry.init() the SDK before the instrumented modules are loaded. This can be achieved via the Node --register and --import CLI flags or by loading your app code via async import() after calling Sentry.init().

Please let me know if this helps solve the import-in-the-middle incompatibility issues you are experiencing!

[torickjdavis]

@timfish, it seems that the option added in v8.29.0 is onlyIncludeInstrumentedModules:

sentry-javascript/packages/node/src/types.ts

Line 20 in bcf571d

onlyIncludeInstrumentedModules?: boolean;

For others: there's also relevant issue for the OpenAI package with some additional discussion: openai/openai-node#903. And a PR for import-in-the-middle to try and add tests for this case: nodejs/import-in-the-middle#113

[timfish]

Oh wow, thanks @torickjdavis, that means the release notes are wrong too. I'll correct these!

[Xhale1]

Haha, I was a bit confused by the release notes too, thanks for the quick fix! (and thanks for your work here as always!)

Off topic so feel free to disregard: how confident do you and the team feel in the Sentry + ESM stability now that this new option is released? Seems #12806 might be pretty much squared away?

[torickjdavis]

🚀 Awesome! I really should look, but haven't yet for Sentry's documentation process, but relevant documentation for onlyIncludeInstrumentedModules should probably be added as another option for skipping instrumentation across the different guides.

[timfish]

how confident do you and the team feel in the Sentry + ESM stability now that this new option is released?

This new option negates all of the fundamental (and not-fixable) issues we've found with import-in-the-middle which have been the major pain points. If you use this option and ensure you init the SDK (and therefore the otel instrumentations) before you app code is loaded, import-in-the-middle should no longer cause problems.

There is a proposal for new loader hooks so this will likely continue to be a moving target.

relevant documentation for onlyIncludeInstrumentedModules should probably be added as another option

We plan to add this to the documentation, we're just trying it out in apps ourselves and waiting for confirmation from more users that it is in fact solving the issues it was suppoed to.

Sentry want it to be as simple and painless as possible to start using the SDKs so we are still left with things we want to improve here. This fix is currently opt-in which is far from ideal. It's not clear from the original error above that this new option will help here!

[Xhale1]

Hello! I'm sorry for not closing the loop here earlier.

Using the onlyIncludeInstrumentedModules option is working great and has solved the original issue! I'm also happy to report my team has fully migrated to v8 thanks to this option 🎉

I'll leave this issue open on the oft chance @timfish wants to use it to track openai compatibility without this option, but feel free to close whenever you see fit :)

Thank you everyone for your dedication to this issue and to the larger open source community :)