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

ESM Mode breaks when using openai package #12237

Closed
3 tasks done
danilofuchs opened this issue May 27, 2024 · 6 comments · Fixed by #12388
Closed
3 tasks done

ESM Mode breaks when using openai package #12237

danilofuchs opened this issue May 27, 2024 · 6 comments · Fixed by #12388
Assignees
Labels
Feature: Performance Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@danilofuchs
Copy link

danilofuchs commented May 27, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

8.4.0

"openai": "^4.33.1"

Framework Version

Node 8.4.0

Link to Sentry event

No response

SDK Setup

// instrumentation.ts
Sentry.init({
  dsn: "",

  // Set tracesSampleRate to 1.0 to capture 100%
  // of transactions for performance monitoring.
  // We recommend adjusting this value in production
  tracesSampleRate: process.env.NODE_ENV === "prod" ? 0.2 : 1,
  profilesSampleRate: 1, // Relative to tracesSampleRate

  environment: process.env.NODE_ENV,
  enabled: process.env.NODE_ENV === "prod",
});

Steps to Reproduce

https://github.com/danilofuchs/sentry-esm-openai-repro

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

Expected Result

Runs with Sentry instrumentation.

Perhaps related to #12059 and #12154 (import-in-the-middle bugs)

Actual Result

> node --import ./build/instrumentation.js build/server.js

file:///home/project/node_modules/openai/resources/beta/assistants/files.mjs:51
    Files.AssistantFilesPage = FilesAPI.AssistantFilesPage;
                                        ^

ReferenceError: Cannot access 'AssistantFilesPage' before initialization
    at file:///project/node_modules/openai/resources/beta/assistants/files.mjs:51:41
    at file:///home/project/node_modules/openai/resources/beta/assistants/files.mjs:52:3
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

Node.js v20.12.2

files.mjs:51:41:

export class AssistantFilesPage extends CursorPage {
}
(function (Files) {
    Files.AssistantFilesPage = FilesAPI.AssistantFilesPage;
})(Files || (Files = {}));
//# sourceMappingURL=files.mjs.map

Obs:
Setting globalThis._sentryEsmLoaderHookRegistered = true; before Sentry.init fixes the crash but disables auto instrumentation

@github-actions github-actions bot added the Package: browser Issues related to the Sentry Browser SDK label May 27, 2024
danilofuchs added a commit to danilofuchs/sentry-esm-openai-repro that referenced this issue May 27, 2024
@AbhiPrasad AbhiPrasad added Feature: Performance Package: node Issues related to the Sentry Node SDK and removed Package: browser Issues related to the Sentry Browser SDK labels May 27, 2024
@AbhiPrasad
Copy link
Member

I have a feeling that all these wildcard imports/exports does not play well

https://github.com/openai/openai-node/blob/d7d5610d91573740d7e3c27e4afde5650ee6e349/src/resources/beta/assistants.ts#L6-L11

@timfish
Copy link
Collaborator

timfish commented May 27, 2024

@danilofuchs thank you so much for the minimal reproduction!

I don't get the same error but it's similar enough:

ReferenceError: Cannot access 'BatchesPage' before initialization
    at file:///Users/tim/Documents/Repositories/sentry-esm-openai-repro/node_modules/openai/resources/batches.mjs:35:38
    at file:///Users/tim/Documents/Repositories/sentry-esm-openai-repro/node_modules/openai/resources/batches.mjs:36:3
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

My best guess at the moment is that this is a bug in import-in-the-middle caused by a circular reference within the file. For example, batches.mjs imports itself via import * as BatchesAPI from "./batches.mjs":

import { APIResource } from "../resource.mjs";
import { isRequestOptions } from "../core.mjs";
import * as BatchesAPI from "./batches.mjs";
import { CursorPage } from "../pagination.mjs";
export class Batches extends APIResource {
    /** snip implementation  */
}
export class BatchesPage extends CursorPage {
}
(function (Batches) {
    Batches.BatchesPage = BatchesAPI.BatchesPage;
})(Batches || (Batches = {}));

If I remove every circular import * reference (there are maybe 10-15 in the lib), it no longer errors:

    Batches.BatchesPage = BatchesPage;

I'll see if I can fix this in import-in-the-middle!

@timfish
Copy link
Collaborator

timfish commented May 27, 2024

Here is the import-in-the-middle issue:
nodejs/import-in-the-middle#82

@AbhiPrasad AbhiPrasad added this to the v8 Instrumentation Bugs milestone May 27, 2024
@timfish
Copy link
Collaborator

timfish commented May 27, 2024

I opened another PR for import-in-the-middle that should fix this.

There is patch available here that combines all the outstanding open PRs.

@danilofuchs
Copy link
Author

@timfish the latest patch fixes it for me!

@mydea
Copy link
Member

mydea commented Jun 7, 2024

Hello, we've just released v8.8.0, which should hopefully resolve this ESM problem. Let us know if you updated and are still running into problems!

billyvg pushed a commit that referenced this issue Jun 10, 2024
resolves #12242
(although there are still some follow ups)

https://github.com/open-telemetry/opentelemetry-js/releases/tag/v1.25.0

I think this lockfile looks correct, but lmk if this feels off.

resolves #12011
resolves #12059
resolves #12154
resolves #12237
resolves nodejs/import-in-the-middle#77 cc
@mohd-akram
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Performance Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants