SDK fails in ESM mode in combination with openai

[Xhale1]

trafficstars

Is there an existing issue for this?

• [X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• [X] I have reviewed the documentation https://docs.sentry.io/
• [X] 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 https://github.com/getsentry/sentry-javascript/issues/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:

• https://github.com/NatoBoram/bug-report-sentry/pull/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: https://github.com/DataDog/import-in-the-middle/pull/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!

[NatoBoram]

Confirmed as fixed in Sentry v8.13.0

[timfish]

There has been some remaining issues reported with openai due to the way it's "shims" work internally.

For example, someone has reported issues with this code:

import OpenAI from "openai";

const openai = new OpenAI({
  apiKey: "fake-api-key",
});

async function doWork() {
  const response = await openai.chat.completions.create({
    model: "gpt-3.5-turbo",
    messages: [
      {
        role: "user",
        content: "Hello, how are you?",
      },
    ],
  });

  console.log(response);
}

doWork().catch((err) => {
  console.error(err);
});

[danilofuchs]

Just tried upgrading again to Sentry 8, version 8.17.0, and started received this error with completion api calls

[timfish]

The next SDK release will have a way to work around this issue.

[timfish]

With v8.18.0 of the SDK just released, there's a new config option which can disable import-in-the-middle for specific libraries:

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

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

[Xhale1]

Hi @timfish, the new method for excluding openai does not appear to work for us. Instead of throwing an error, openai calls seem to freeze any further execution (I might be using the wrong terminology here).

Example:

import OpenAI from "openai";

export const openAI = new OpenAI({
  apiKey: "...",
});

const response = await openAI.chat.completions.create({
      model: "gpt-3.5-turbo",
      messages: [
        {
          role: "system",
          content: "You are a helpful assistant.",
        },
        {
          role: "user",
          content: "What's the weather like?",
        },
      ],
    });

console.log("Hello there")

In this example, with ESM, registerEsmLoaderHooks: { exclude: ['openai'] }, and Sentry v8.18.0, "Hello there" is never printed and no errors are outputted to the console.

[timfish]

Over the weekend I submitted another PR for import-in-the-middle which expands exclude so that we can use regular expressions:

• https://github.com/nodejs/import-in-the-middle/pull/148

In the long run I think we've got a better solution which should completely remove the need for manually excluding problematic libraries like this. This PR allows us to automatically add only the libraries that Sentry are instrumenting to the include list:

• https://github.com/nodejs/import-in-the-middle/pull/146

[timfish]

If you update your dependencies to ensure that you're using [email protected] you can now pass a regular expressions for exclude. The Sentry types will be updated in the next release to reflect this.

This allows all paths with openai in them to be excluded:

registerEsmLoaderHooks: { exclude: [/openai/] },

[Ivan-juni]

any updates here? @timfish still getting this error with the newest sdk version when using late initialization --import @sentry/node/preload

[andreiborza]

hey @Ivan-juni we are still waiting on https://github.com/getsentry/sentry-javascript/pull/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:

https://github.com/getsentry/sentry-javascript/blob/a6fda4c1cbc3cea44b1c0386ebeb08c4d43b6a24/packages/node/src/types.ts#L94-L97

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 https://github.com/getsentry/sentry-javascript/pull/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: https://github.com/getsentry/sentry-javascript/blob/bcf571d9954094be76a99edbb12c23eff7f7b5dc/packages/node/src/types.ts#L20

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

[timfish]

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