From 3483a62bdb3d140fbd147093a52a14425f019f0e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 22 Jun 2023 14:15:29 +0200 Subject: [PATCH 1/5] fix(tracing): Instrument Prisma client in constructor of integration --- .../src/node/integrations/prisma.ts | 57 +++++++++---------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/packages/tracing-internal/src/node/integrations/prisma.ts b/packages/tracing-internal/src/node/integrations/prisma.ts index 20a84aa36aaf..0601d8e22b8d 100644 --- a/packages/tracing-internal/src/node/integrations/prisma.ts +++ b/packages/tracing-internal/src/node/integrations/prisma.ts @@ -1,7 +1,6 @@ -import type { Hub } from '@sentry/core'; -import { trace } from '@sentry/core'; -import type { EventProcessor, Integration } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import { getCurrentHub, trace } from '@sentry/core'; +import type { Integration } from '@sentry/types'; +import { addNonEnumerableProperty, logger } from '@sentry/utils'; import { shouldDisableAutoInstrumentation } from './utils/node-utils'; @@ -36,6 +35,7 @@ type PrismaMiddleware = ( ) => Promise; interface PrismaClient { + _sentryInstrumented?: boolean; $use: (cb: PrismaMiddleware) => void; } @@ -55,17 +55,31 @@ export class Prisma implements Integration { */ public name: string = Prisma.id; - /** - * Prisma ORM Client Instance - */ - private readonly _client?: PrismaClient; - /** * @inheritDoc */ public constructor(options: { client?: unknown } = {}) { - if (isValidPrismaClient(options.client)) { - this._client = options.client; + // We instrument the PrismaClient inside the constructor and not inside `setupOnce` because in some cases of server-side + // bundling (Next.js) multiple Prisma clients can be instantiated, even though users don't intend to. When instrumenting + // in setupOnce we can only ever instrument one client. + // https://github.com/getsentry/sentry-javascript/issues/7216#issuecomment-1602375012 + // In the future we might explore providing a dedicated PrismaClient middleware instead of this hack. + if (isValidPrismaClient(options.client) && !options.client._sentryInstrumented) { + addNonEnumerableProperty(options.client as any, '_sentryInstrumented', true); + + if (shouldDisableAutoInstrumentation(getCurrentHub)) { + __DEBUG_BUILD__ && logger.log('Prisma Integration is skipped because of instrumenter configuration.'); + return; + } + + options.client.$use((params, next: (params: PrismaMiddlewareParams) => Promise) => { + const action = params.action; + const model = params.model; + return trace( + { name: model ? `${model} ${action}` : action, op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, + () => next(params), + ); + }); } else { __DEBUG_BUILD__ && logger.warn( @@ -77,24 +91,7 @@ export class Prisma implements Integration { /** * @inheritDoc */ - public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { - if (!this._client) { - __DEBUG_BUILD__ && logger.error('PrismaIntegration is missing a Prisma Client Instance'); - return; - } - - if (shouldDisableAutoInstrumentation(getCurrentHub)) { - __DEBUG_BUILD__ && logger.log('Prisma Integration is skipped because of instrumenter configuration.'); - return; - } - - this._client.$use((params, next: (params: PrismaMiddlewareParams) => Promise) => { - const action = params.action; - const model = params.model; - return trace( - { name: model ? `${model} ${action}` : action, op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, - () => next(params), - ); - }); + public setupOnce(): void { + // Noop - here for backwards compatibility } } From 31879228cbc5635faf9438358de5a0e17b5dd6c6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 22 Jun 2023 14:42:21 +0200 Subject: [PATCH 2/5] fix unit tests --- .../test/integrations/node/prisma.test.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/packages/tracing/test/integrations/node/prisma.test.ts b/packages/tracing/test/integrations/node/prisma.test.ts index 3096401ec43a..953947f7df5d 100644 --- a/packages/tracing/test/integrations/node/prisma.test.ts +++ b/packages/tracing/test/integrations/node/prisma.test.ts @@ -1,10 +1,7 @@ /* eslint-disable deprecation/deprecation */ -/* eslint-disable @typescript-eslint/unbound-method */ -import { Hub, Scope } from '@sentry/core'; import { logger } from '@sentry/utils'; import { Integrations } from '../../../src'; -import { getTestClient } from '../../testutils'; const mockTrace = jest.fn(); @@ -41,10 +38,7 @@ describe('setupOnce', function () { const Client: PrismaClient = new PrismaClient(); beforeAll(() => { - new Integrations.Prisma({ client: Client }).setupOnce( - () => undefined, - () => new Hub(undefined, new Scope()), - ); + new Integrations.Prisma({ client: Client }); }); beforeEach(() => { @@ -65,14 +59,7 @@ describe('setupOnce', function () { it("doesn't attach when using otel instrumenter", () => { const loggerLogSpy = jest.spyOn(logger, 'log'); - const client = getTestClient({ instrumenter: 'otel' }); - const hub = new Hub(client); - - const integration = new Integrations.Prisma({ client: Client }); - integration.setupOnce( - () => {}, - () => hub, - ); + new Integrations.Prisma({ client: Client }); expect(loggerLogSpy).toBeCalledWith('Prisma Integration is skipped because of instrumenter configuration.'); }); From 42c507bbcc7ffa69933a5072ebc2156b46eb8242 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 22 Jun 2023 14:42:46 +0200 Subject: [PATCH 3/5] add tracing internal to shared packages in changed logic in ci --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3bc95b171d94..b8e3fcafcb34 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -89,6 +89,7 @@ jobs: - 'scripts/**' - 'packages/core/**' - 'packages/tracing/**' + - 'packages/tracing-internal/**' - 'packages/utils/**' - 'packages/types/**' - 'packages/integrations/**' From 32e4b3edf89860c7b4df3562a3bd45e03c574390 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 22 Jun 2023 15:41:09 +0200 Subject: [PATCH 4/5] Fix tests --- .../src/node/integrations/prisma.ts | 9 +++--- .../test/integrations/node/prisma.test.ts | 29 ++++++++++++------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/tracing-internal/src/node/integrations/prisma.ts b/packages/tracing-internal/src/node/integrations/prisma.ts index 0601d8e22b8d..ab6fd1f1ae5b 100644 --- a/packages/tracing-internal/src/node/integrations/prisma.ts +++ b/packages/tracing-internal/src/node/integrations/prisma.ts @@ -67,12 +67,11 @@ export class Prisma implements Integration { if (isValidPrismaClient(options.client) && !options.client._sentryInstrumented) { addNonEnumerableProperty(options.client as any, '_sentryInstrumented', true); - if (shouldDisableAutoInstrumentation(getCurrentHub)) { - __DEBUG_BUILD__ && logger.log('Prisma Integration is skipped because of instrumenter configuration.'); - return; - } - options.client.$use((params, next: (params: PrismaMiddlewareParams) => Promise) => { + if (shouldDisableAutoInstrumentation(getCurrentHub)) { + return next(params); + } + const action = params.action; const model = params.model; return trace( diff --git a/packages/tracing/test/integrations/node/prisma.test.ts b/packages/tracing/test/integrations/node/prisma.test.ts index 953947f7df5d..b3bd30c49f75 100644 --- a/packages/tracing/test/integrations/node/prisma.test.ts +++ b/packages/tracing/test/integrations/node/prisma.test.ts @@ -1,7 +1,10 @@ /* eslint-disable deprecation/deprecation */ +import * as sentryCore from '@sentry/core'; +import { Hub } from '@sentry/core'; import { logger } from '@sentry/utils'; import { Integrations } from '../../../src'; +import { getTestClient } from '../../testutils'; const mockTrace = jest.fn(); @@ -35,18 +38,15 @@ class PrismaClient { } describe('setupOnce', function () { - const Client: PrismaClient = new PrismaClient(); - - beforeAll(() => { - new Integrations.Prisma({ client: Client }); - }); - beforeEach(() => { mockTrace.mockClear(); + mockTrace.mockReset(); }); it('should add middleware with $use method correctly', done => { - void Client.user.create()?.then(() => { + const prismaClient = new PrismaClient(); + new Integrations.Prisma({ client: prismaClient }); + void prismaClient.user.create()?.then(() => { expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenLastCalledWith( { name: 'user create', op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, @@ -56,11 +56,18 @@ describe('setupOnce', function () { }); }); - it("doesn't attach when using otel instrumenter", () => { - const loggerLogSpy = jest.spyOn(logger, 'log'); + it("doesn't trace when using otel instrumenter", done => { + const prismaClient = new PrismaClient(); + new Integrations.Prisma({ client: prismaClient }); - new Integrations.Prisma({ client: Client }); + const client = getTestClient({ instrumenter: 'otel' }); + const hub = new Hub(client); - expect(loggerLogSpy).toBeCalledWith('Prisma Integration is skipped because of instrumenter configuration.'); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + void prismaClient.user.create()?.then(() => { + expect(mockTrace).not.toHaveBeenCalled(); + done(); + }); }); }); From bf42b2f10aed651e17c50f759f8c8ad5482c0c14 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 23 Jun 2023 09:04:48 +0200 Subject: [PATCH 5/5] lint --- packages/tracing/test/integrations/node/prisma.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/tracing/test/integrations/node/prisma.test.ts b/packages/tracing/test/integrations/node/prisma.test.ts index b3bd30c49f75..61c0e5fb07f6 100644 --- a/packages/tracing/test/integrations/node/prisma.test.ts +++ b/packages/tracing/test/integrations/node/prisma.test.ts @@ -1,7 +1,6 @@ /* eslint-disable deprecation/deprecation */ import * as sentryCore from '@sentry/core'; import { Hub } from '@sentry/core'; -import { logger } from '@sentry/utils'; import { Integrations } from '../../../src'; import { getTestClient } from '../../testutils';