From 6fa2ccbd64bc6b5ca1fe254bad1f7c66e5d52943 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 25 Jun 2024 13:20:18 +0200 Subject: [PATCH 1/5] feat(node): add context info for missing intrumentation --- packages/node/src/utils/ensureIsWrapped.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/node/src/utils/ensureIsWrapped.ts b/packages/node/src/utils/ensureIsWrapped.ts index 8cf85b39d545..6cd0d1d324be 100644 --- a/packages/node/src/utils/ensureIsWrapped.ts +++ b/packages/node/src/utils/ensureIsWrapped.ts @@ -1,5 +1,5 @@ import { isWrapped } from '@opentelemetry/core'; -import { hasTracingEnabled, isEnabled } from '@sentry/core'; +import { getCurrentScope, hasTracingEnabled, isEnabled } from '@sentry/core'; import { consoleSandbox } from '@sentry/utils'; import { isCjs } from './commonjs'; @@ -24,5 +24,10 @@ export function ensureIsWrapped( ); } }); + + getCurrentScope().setContext('Instrumentation', { + isMissing: true, + framework: name, + }); } } From 49b5cbddd18ac216dd99d734140faac4a6fedf5e Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 25 Jun 2024 14:53:41 +0200 Subject: [PATCH 2/5] feat: rename package attribute and add isCjs to context --- packages/node/src/utils/ensureIsWrapped.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/node/src/utils/ensureIsWrapped.ts b/packages/node/src/utils/ensureIsWrapped.ts index 6cd0d1d324be..47ae6d5ea199 100644 --- a/packages/node/src/utils/ensureIsWrapped.ts +++ b/packages/node/src/utils/ensureIsWrapped.ts @@ -1,5 +1,5 @@ import { isWrapped } from '@opentelemetry/core'; -import { getCurrentScope, hasTracingEnabled, isEnabled } from '@sentry/core'; +import { getGlobalScope, hasTracingEnabled, isEnabled } from '@sentry/core'; import { consoleSandbox } from '@sentry/utils'; import { isCjs } from './commonjs'; @@ -25,9 +25,10 @@ export function ensureIsWrapped( } }); - getCurrentScope().setContext('Instrumentation', { + getGlobalScope().setContext('Instrumentation', { isMissing: true, - framework: name, + package: name, + isCjs: isCjs(), }); } } From a26d2e97ae4e57b1d8b5872ce68ec01a269e5e8c Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 25 Jun 2024 14:54:22 +0200 Subject: [PATCH 3/5] test: Add e2e test for missing instrumentation --- .../.gitignore | 1 + .../.npmrc | 2 + .../package.json | 32 +++++++++++++++ .../playwright.config.mjs | 7 ++++ .../src/app.ts | 39 +++++++++++++++++++ .../start-event-proxy.mjs | 6 +++ .../tests/instrumentation.test.ts | 21 ++++++++++ .../tsconfig.json | 10 +++++ 8 files changed, 118 insertions(+) create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.gitignore create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.npmrc create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/package.json create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/playwright.config.mjs create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/src/app.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/start-event-proxy.mjs create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tsconfig.json diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.gitignore b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.gitignore new file mode 100644 index 000000000000..1521c8b7652b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.gitignore @@ -0,0 +1 @@ +dist diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.npmrc b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.npmrc new file mode 100644 index 000000000000..070f80f05092 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/.npmrc @@ -0,0 +1,2 @@ +@sentry:registry=http://127.0.0.1:4873 +@sentry-internal:registry=http://127.0.0.1:4873 diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/package.json b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/package.json new file mode 100644 index 000000000000..3df947bb58c5 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/package.json @@ -0,0 +1,32 @@ +{ + "name": "node-express-incorrect-instrumentation", + "version": "1.0.0", + "private": true, + "scripts": { + "build": "tsc", + "start": "node dist/app.js", + "test": "playwright test", + "clean": "npx rimraf node_modules pnpm-lock.yaml", + "test:build": "pnpm install && pnpm build", + "test:assert": "pnpm test" + }, + "dependencies": { + "@sentry/core": "latest || *", + "@sentry/node": "latest || *", + "@sentry/types": "latest || *", + "@trpc/server": "10.45.2", + "@trpc/client": "10.45.2", + "@types/express": "4.17.17", + "@types/node": "18.15.1", + "express": "4.19.2", + "typescript": "4.9.5", + "zod": "~3.22.4" + }, + "devDependencies": { + "@playwright/test": "^1.44.1", + "@sentry-internal/test-utils": "link:../../../test-utils" + }, + "volta": { + "extends": "../../package.json" + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/playwright.config.mjs new file mode 100644 index 000000000000..31f2b913b58b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/playwright.config.mjs @@ -0,0 +1,7 @@ +import { getPlaywrightConfig } from '@sentry-internal/test-utils'; + +const config = getPlaywrightConfig({ + startCommand: `pnpm start`, +}); + +export default config; diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/src/app.ts b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/src/app.ts new file mode 100644 index 000000000000..2ab5d1ace5a0 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/src/app.ts @@ -0,0 +1,39 @@ +declare global { + namespace globalThis { + var transactionIds: string[]; + } +} + +import express from 'express'; + +const app = express(); +const port = 3030; + +// import and init sentry last for missing instrumentation +import * as Sentry from '@sentry/node'; +Sentry.init({ + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + includeLocalVariables: true, + debug: !!process.env.DEBUG, + tunnel: `http://localhost:3031/`, // proxy server + tracesSampleRate: 1, +}); + +app.get('/test-exception/:id', function (req, _res) { + throw new Error(`This is an exception with id ${req.params.id}`); +}); + +Sentry.setupExpressErrorHandler(app); + +// @ts-ignore +app.use(function onError(err, req, res, next) { + // The error id is attached to `res.sentry` to be returned + // and optionally displayed to the user for support. + res.statusCode = 500; + res.end(res.sentry + '\n'); +}); + +app.listen(port, () => { + console.log(`Example app listening on port ${port}`); +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/start-event-proxy.mjs b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/start-event-proxy.mjs new file mode 100644 index 000000000000..3276781a442a --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/start-event-proxy.mjs @@ -0,0 +1,6 @@ +import { startEventProxyServer } from '@sentry-internal/test-utils'; + +startEventProxyServer({ + port: 3031, + proxyServerName: 'node-express', +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts new file mode 100644 index 000000000000..94829a9c735d --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts @@ -0,0 +1,21 @@ +import { expect, test } from '@playwright/test'; +import { waitForError } from '@sentry-internal/test-utils'; + +test('Sends correct context when instrumentation was set up incorrectly', async ({ baseURL }) => { + const errorEventPromise = waitForError('node-express', event => { + return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123'; + }); + + await fetch(`${baseURL}/test-exception/123`); + + const errorEvent = await errorEventPromise; + + expect(errorEvent.exception?.values).toHaveLength(1); + expect(errorEvent.exception?.values?.[0]?.value).toBe('This is an exception with id 123'); + + expect(errorEvent.contexts?.Instrumentation).toEqual({ + isMissing: true, + package: 'express', + isCjs: true, + }); +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tsconfig.json b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tsconfig.json new file mode 100644 index 000000000000..8cb64e989ed9 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tsconfig.json @@ -0,0 +1,10 @@ +{ + "compilerOptions": { + "types": ["node"], + "esModuleInterop": true, + "lib": ["es2018"], + "strict": true, + "outDir": "dist" + }, + "include": ["src/**/*.ts"] +} From eae9855cd30e6e75005829de8159909832185469 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 25 Jun 2024 17:41:05 +0200 Subject: [PATCH 4/5] feat: update context for missing instrumentation --- .../tests/instrumentation.test.ts | 5 ++--- packages/node/src/integrations/context.ts | 7 +++++++ packages/node/src/utils/ensureIsWrapped.ts | 7 ++----- packages/types/src/context.ts | 5 +++++ packages/types/src/index.ts | 1 + 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts index 94829a9c735d..f562cb3a5837 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/tests/instrumentation.test.ts @@ -13,9 +13,8 @@ test('Sends correct context when instrumentation was set up incorrectly', async expect(errorEvent.exception?.values).toHaveLength(1); expect(errorEvent.exception?.values?.[0]?.value).toBe('This is an exception with id 123'); - expect(errorEvent.contexts?.Instrumentation).toEqual({ - isMissing: true, + expect(errorEvent.contexts?.missing_instrumentation).toEqual({ package: 'express', - isCjs: true, + 'javascript.is_cjs': true, }); }); diff --git a/packages/node/src/integrations/context.ts b/packages/node/src/integrations/context.ts index e8ab2c7f84f1..29a6a14da216 100644 --- a/packages/node/src/integrations/context.ts +++ b/packages/node/src/integrations/context.ts @@ -13,8 +13,10 @@ import type { DeviceContext, Event, IntegrationFn, + MissingInstrumentationContext, OsContext, } from '@sentry/types'; +import { isCjs } from '../utils/commonjs'; export const readFileAsync = promisify(readFile); export const readDirAsync = promisify(readdir); @@ -476,3 +478,8 @@ function getCloudResourceContext(): CloudResourceContext | undefined { return undefined; } } + +export const createMissingInstrumentationContext = (pkg: string): MissingInstrumentationContext => ({ + package: pkg, + 'javascript.is_cjs': isCjs(), +}); diff --git a/packages/node/src/utils/ensureIsWrapped.ts b/packages/node/src/utils/ensureIsWrapped.ts index 47ae6d5ea199..2362c735d8ea 100644 --- a/packages/node/src/utils/ensureIsWrapped.ts +++ b/packages/node/src/utils/ensureIsWrapped.ts @@ -1,6 +1,7 @@ import { isWrapped } from '@opentelemetry/core'; import { getGlobalScope, hasTracingEnabled, isEnabled } from '@sentry/core'; import { consoleSandbox } from '@sentry/utils'; +import { createMissingInstrumentationContext } from '../integrations/context'; import { isCjs } from './commonjs'; /** @@ -25,10 +26,6 @@ export function ensureIsWrapped( } }); - getGlobalScope().setContext('Instrumentation', { - isMissing: true, - package: name, - isCjs: isCjs(), - }); + getGlobalScope().setContext('missing_instrumentation', createMissingInstrumentationContext(name)); } } diff --git a/packages/types/src/context.ts b/packages/types/src/context.ts index 0344dd179787..10fc61420e25 100644 --- a/packages/types/src/context.ts +++ b/packages/types/src/context.ts @@ -119,3 +119,8 @@ export interface CloudResourceContext extends Record { export interface ProfileContext extends Record { profile_id: string; } + +export interface MissingInstrumentationContext extends Record { + package: string; + ['javascript.is_cjs']?: boolean; +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index a7cf18056eb6..8f7fdce74c33 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -18,6 +18,7 @@ export type { CultureContext, TraceContext, CloudResourceContext, + MissingInstrumentationContext, } from './context'; export type { DataCategory } from './datacategory'; export type { DsnComponents, DsnLike, DsnProtocol } from './dsn'; From 83cda6b04a2ed0b9496357b1696974e91e1fa13c Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 26 Jun 2024 08:50:14 +0200 Subject: [PATCH 5/5] ref: Move context create fn to utils --- packages/node/src/integrations/context.ts | 7 ------- .../node/src/utils/createMissingInstrumentationContext.ts | 7 +++++++ packages/node/src/utils/ensureIsWrapped.ts | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) create mode 100644 packages/node/src/utils/createMissingInstrumentationContext.ts diff --git a/packages/node/src/integrations/context.ts b/packages/node/src/integrations/context.ts index 29a6a14da216..e8ab2c7f84f1 100644 --- a/packages/node/src/integrations/context.ts +++ b/packages/node/src/integrations/context.ts @@ -13,10 +13,8 @@ import type { DeviceContext, Event, IntegrationFn, - MissingInstrumentationContext, OsContext, } from '@sentry/types'; -import { isCjs } from '../utils/commonjs'; export const readFileAsync = promisify(readFile); export const readDirAsync = promisify(readdir); @@ -478,8 +476,3 @@ function getCloudResourceContext(): CloudResourceContext | undefined { return undefined; } } - -export const createMissingInstrumentationContext = (pkg: string): MissingInstrumentationContext => ({ - package: pkg, - 'javascript.is_cjs': isCjs(), -}); diff --git a/packages/node/src/utils/createMissingInstrumentationContext.ts b/packages/node/src/utils/createMissingInstrumentationContext.ts new file mode 100644 index 000000000000..849e49e8b8e2 --- /dev/null +++ b/packages/node/src/utils/createMissingInstrumentationContext.ts @@ -0,0 +1,7 @@ +import type { MissingInstrumentationContext } from '@sentry/types'; +import { isCjs } from './commonjs'; + +export const createMissingInstrumentationContext = (pkg: string): MissingInstrumentationContext => ({ + package: pkg, + 'javascript.is_cjs': isCjs(), +}); diff --git a/packages/node/src/utils/ensureIsWrapped.ts b/packages/node/src/utils/ensureIsWrapped.ts index 2362c735d8ea..05185a293bed 100644 --- a/packages/node/src/utils/ensureIsWrapped.ts +++ b/packages/node/src/utils/ensureIsWrapped.ts @@ -1,8 +1,8 @@ import { isWrapped } from '@opentelemetry/core'; import { getGlobalScope, hasTracingEnabled, isEnabled } from '@sentry/core'; import { consoleSandbox } from '@sentry/utils'; -import { createMissingInstrumentationContext } from '../integrations/context'; import { isCjs } from './commonjs'; +import { createMissingInstrumentationContext } from './createMissingInstrumentationContext'; /** * Checks and warns if a framework isn't wrapped by opentelemetry.