Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(core, node): support loading Express options lazily#20211
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import * as Sentry from '@sentry/node'; | ||
| import { loggingTransport } from '@sentry-internal/node-integration-tests'; | ||
| // First: preload the express instrumentation without calling Sentry.init(). | ||
| // registers OTel module hook, patches the Express module with no config. | ||
| Sentry.preloadOpenTelemetry({ integrations: ['Express'] }); | ||
| // call Sentry.init() with express integration config. | ||
| // instrumentExpress is already registered, so this calls setConfig() on the | ||
| // existing instrumentation to update its options. The lazy getOptions() | ||
| // in patchLayer ensures the updated options are read at request time. | ||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| release: '1.0', | ||
| tracesSampleRate: 1.0, | ||
| transport: loggingTransport, | ||
| // suppress the middleware layer that the cors module generates | ||
| integrations: [Sentry.expressIntegration({ ignoreLayersType: ['middleware'] })], | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import cors from 'cors'; | ||
| import express from 'express'; | ||
| import * as Sentry from '@sentry/node'; | ||
| import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; | ||
| const app = express(); | ||
| // cors() would normally create a 'middleware' type span, but the | ||
| // ignoreLayersType: ['middleware'] option set via Sentry.init() suppresses it. | ||
| app.use(cors()); | ||
| app.get('/test/express', (_req, res) => { | ||
| res.send({ response: 'response 1' }); | ||
| }); | ||
| Sentry.setupExpressErrorHandler(app); | ||
| startExpressServerAndSendPortToRunner(app); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { afterAll, describe, expect } from 'vitest'; | ||
| import { assertSentryTransaction } from '../../../utils/assertions'; | ||
| import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; | ||
| describe('express late init', () => { | ||
| afterAll(() => { | ||
| cleanupChildProcesses(); | ||
| }); | ||
| createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { | ||
| test('applies expressIntegration config set via Sentry.init() called after instrumentExpress()', async () => { | ||
| const runner = createRunner() | ||
| .expect({ | ||
| transaction: transaction => { | ||
| assertSentryTransaction(transaction, { | ||
| transaction: 'GET /test/express', | ||
| contexts: { | ||
| trace: { | ||
| op: 'http.server', | ||
| status: 'ok', | ||
| }, | ||
| }, | ||
| }); | ||
| // request_handler span IS present | ||
| // confirms the express patch was applied. | ||
| expect(transaction.spans).toContainEqual( | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| 'express.type': 'request_handler', | ||
| }), | ||
| }), | ||
| ); | ||
| // Middleware spans NOT present, ignoreLayersType: ['middleware'] | ||
| // configured via the Sentry.init() AFTER instrumentExpress(). | ||
| expect(transaction.spans).not.toContainEqual( | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| 'express.type': 'middleware', | ||
| }), | ||
| }), | ||
| ); | ||
| }, | ||
| }) | ||
| .start(); | ||
| runner.makeRequest('get', '/test/express'); | ||
| await runner.completed(); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -136,7 +136,12 @@ export type ExpressRouter = { | ||
| export type IgnoreMatcher = string | RegExp | ((name: string) => boolean); | ||
| export type ExpressIntegrationOptions = { | ||
| express: ExpressModuleExport; //Express | ||
isaacs marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /** | ||
| * @deprecated Pass the express module as the first argument, and an | ||
| * options getter as the second argument to patchExpressModule. | ||
| */ | ||
| express?: ExpressModuleExport; | ||
| /** Ignore specific based on their name */ | ||
| ignoreLayers?: IgnoreMatcher[]; | ||
| /** Ignore specific layers based on their type */ | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public API function signature changed for exported function
Low Severity
Flagging per the review rules file:
patchExpressModuleis publicly exported from@sentry/coreand its function signature changed from a single-argument form to a two-argument overloaded form. Additionally, the publicly exportedExpressIntegrationOptionstype changed theexpressfield from required to optional. While backward compatibility is maintained via a deprecated overload and runtime deprecation warning, consumers who stored options in a variable typed asExpressIntegrationOptionsand passed it topatchExpressModulewill encounter a TypeScript error, since the deprecated overload requiresExpressIntegrationOptions & { express: ExpressModuleExport }which the now-optionalexpressfield no longer satisfies.Additional Locations (1)
packages/core/src/integrations/express/types.ts#L137-L143Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0be7618. Configure here.