From e09b93a6fc13f4aa141604f71836dacd6c538d19 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 12 Jun 2024 15:16:17 +0200 Subject: [PATCH 01/29] build: Add `tsconfig.json` to test directories to fix VSCode (#12475) VSCode does not pick up the `tsconfig.test.json` files, and thus does not apply the config properly to the test files. By adding a tsconfig.json into the test/ directory, this is picked up correctly by VSCode as well. --- packages/astro/test/tsconfig.json | 3 +++ packages/aws-serverless/test/tsconfig.json | 3 +++ packages/browser-utils/test/tsconfig.json | 3 +++ packages/browser/test/tsconfig.json | 3 +++ packages/bun/test/tsconfig.json | 3 +++ packages/core/test/tsconfig.json | 3 +++ packages/deno/test/tsconfig.json | 3 +++ packages/gatsby/test/tsconfig.json | 3 +++ packages/google-cloud-serverless/test/tsconfig.json | 3 +++ packages/node/test/tsconfig.json | 3 +++ packages/opentelemetry/test/tsconfig.json | 3 +++ packages/profiling-node/test/tsconfig.json | 3 +++ packages/react/test/tsconfig.json | 3 +++ packages/remix/test/tsconfig.json | 3 +++ packages/replay-canvas/test/tsconfig.json | 3 +++ packages/replay-internal/test/tsconfig.json | 3 +++ packages/replay-worker/test/tsconfig.json | 3 +++ packages/solid/test/tsconfig.json | 3 +++ packages/svelte/test/tsconfig.json | 3 +++ packages/sveltekit/test/tsconfig.json | 3 +++ packages/utils/test/tsconfig.json | 3 +++ packages/vercel-edge/test/tsconfig.json | 3 +++ packages/vue/test/tsconfig.json | 3 +++ 23 files changed, 69 insertions(+) create mode 100644 packages/astro/test/tsconfig.json create mode 100644 packages/aws-serverless/test/tsconfig.json create mode 100644 packages/browser-utils/test/tsconfig.json create mode 100644 packages/browser/test/tsconfig.json create mode 100644 packages/bun/test/tsconfig.json create mode 100644 packages/core/test/tsconfig.json create mode 100644 packages/deno/test/tsconfig.json create mode 100644 packages/gatsby/test/tsconfig.json create mode 100644 packages/google-cloud-serverless/test/tsconfig.json create mode 100644 packages/node/test/tsconfig.json create mode 100644 packages/opentelemetry/test/tsconfig.json create mode 100644 packages/profiling-node/test/tsconfig.json create mode 100644 packages/react/test/tsconfig.json create mode 100644 packages/remix/test/tsconfig.json create mode 100644 packages/replay-canvas/test/tsconfig.json create mode 100644 packages/replay-internal/test/tsconfig.json create mode 100644 packages/replay-worker/test/tsconfig.json create mode 100644 packages/solid/test/tsconfig.json create mode 100644 packages/svelte/test/tsconfig.json create mode 100644 packages/sveltekit/test/tsconfig.json create mode 100644 packages/utils/test/tsconfig.json create mode 100644 packages/vercel-edge/test/tsconfig.json create mode 100644 packages/vue/test/tsconfig.json diff --git a/packages/astro/test/tsconfig.json b/packages/astro/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/astro/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/aws-serverless/test/tsconfig.json b/packages/aws-serverless/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/aws-serverless/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/browser-utils/test/tsconfig.json b/packages/browser-utils/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/browser-utils/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/browser/test/tsconfig.json b/packages/browser/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/browser/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/bun/test/tsconfig.json b/packages/bun/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/bun/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/core/test/tsconfig.json b/packages/core/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/core/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/deno/test/tsconfig.json b/packages/deno/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/deno/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/gatsby/test/tsconfig.json b/packages/gatsby/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/gatsby/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/google-cloud-serverless/test/tsconfig.json b/packages/google-cloud-serverless/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/google-cloud-serverless/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/node/test/tsconfig.json b/packages/node/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/node/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/opentelemetry/test/tsconfig.json b/packages/opentelemetry/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/opentelemetry/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/profiling-node/test/tsconfig.json b/packages/profiling-node/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/profiling-node/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/react/test/tsconfig.json b/packages/react/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/react/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/remix/test/tsconfig.json b/packages/remix/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/remix/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/replay-canvas/test/tsconfig.json b/packages/replay-canvas/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/replay-canvas/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/replay-internal/test/tsconfig.json b/packages/replay-internal/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/replay-internal/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/replay-worker/test/tsconfig.json b/packages/replay-worker/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/replay-worker/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/solid/test/tsconfig.json b/packages/solid/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/solid/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/svelte/test/tsconfig.json b/packages/svelte/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/svelte/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/sveltekit/test/tsconfig.json b/packages/sveltekit/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/sveltekit/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/utils/test/tsconfig.json b/packages/utils/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/utils/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/vercel-edge/test/tsconfig.json b/packages/vercel-edge/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/vercel-edge/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} diff --git a/packages/vue/test/tsconfig.json b/packages/vue/test/tsconfig.json new file mode 100644 index 000000000000..38ca0b13bcdd --- /dev/null +++ b/packages/vue/test/tsconfig.json @@ -0,0 +1,3 @@ +{ + "extends": "../tsconfig.test.json" +} From b2bdf1a40f1550b2b623889cd54064c31b533a54 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 12 Jun 2024 09:54:16 -0400 Subject: [PATCH 02/29] feat(profiling) add global profile context while profiler is running (#12394) Attach profile context to events while a profiler is active and assign thread id and name to the trace context. This will be required by the profiling API so that we can figure out the links between spans and profiles. In the future (span streaming), tid and thread name will be assigned to each span --- packages/profiling-node/src/integration.ts | 73 +++++++++++++++++- .../profiling-node/src/spanProfileUtils.ts | 1 - packages/profiling-node/src/utils.ts | 10 +-- .../test/spanProfileUtils.test.ts | 45 ++++++++++- .../test/spanProfileUtils.worker.test.ts | 75 +++++++++++++++++++ 5 files changed, 190 insertions(+), 14 deletions(-) create mode 100644 packages/profiling-node/test/spanProfileUtils.worker.test.ts diff --git a/packages/profiling-node/src/integration.ts b/packages/profiling-node/src/integration.ts index 6cba86fe4f8d..574c0250e76a 100644 --- a/packages/profiling-node/src/integration.ts +++ b/packages/profiling-node/src/integration.ts @@ -1,17 +1,20 @@ import { defineIntegration, getCurrentScope, getIsolationScope, getRootSpan, spanToJSON } from '@sentry/core'; import type { NodeClient } from '@sentry/node'; -import type { Integration, IntegrationFn, Profile, ProfileChunk, Span } from '@sentry/types'; +import type { Event, Integration, IntegrationFn, Profile, ProfileChunk, Span } from '@sentry/types'; import { LRUMap, logger, timestampInSeconds, uuid4 } from '@sentry/utils'; +import { getGlobalScope } from '../../core/src/currentScopes'; import { CpuProfilerBindings } from './cpu_profiler'; import { DEBUG_BUILD } from './debug-build'; import { NODE_MAJOR, NODE_VERSION } from './nodeVersion'; import { MAX_PROFILE_DURATION_MS, maybeProfileSpan, stopSpanProfile } from './spanProfileUtils'; -import type { RawThreadCpuProfile } from './types'; +import type { RawChunkCpuProfile, RawThreadCpuProfile } from './types'; import { ProfileFormat } from './types'; +import { PROFILER_THREAD_NAME } from './utils'; import { + PROFILER_THREAD_ID_STRING, addProfilesToEnvelope, createProfilingChunkEvent, createProfilingEvent, @@ -211,7 +214,9 @@ class ContinuousProfiler { logger.log(`[Profiling] Failed to collect profile for: ${this._chunkData?.id}, the chunk_id is missing.`); return; } - const profile = CpuProfilerBindings.stopProfiling(this._chunkData.id, ProfileFormat.CHUNK); + + const profile = this._stopChunkProfiling(this._chunkData); + if (!profile || !this._chunkData.startTimestampMS) { DEBUG_BUILD && logger.log(`[Profiling] _chunkiledStartTraceID to collect profile for: ${this._chunkData.id}`); return; @@ -274,12 +279,22 @@ class ContinuousProfiler { }); } + /** + * Stops the profile and clears chunk instrumentation from global scope + * @returns void + */ + private _stopChunkProfiling(chunk: ChunkData): RawChunkCpuProfile | null { + this._teardownSpanChunkInstrumentation(); + return CpuProfilerBindings.stopProfiling(chunk.id, ProfileFormat.CHUNK); + } + /** * Starts the profiler and registers the flush timer for a given chunk. * @param chunk */ private _startChunkProfiling(chunk: ChunkData): void { - CpuProfilerBindings.startProfiling(chunk.id!); + this._setupSpanChunkInstrumentation(); + CpuProfilerBindings.startProfiling(chunk.id); DEBUG_BUILD && logger.log(`[Profiling] starting profiling chunk: ${chunk.id}`); chunk.timer = global.setTimeout(() => { @@ -293,6 +308,32 @@ class ContinuousProfiler { chunk.timer.unref(); } + /** + * Attaches profiling information to spans that were started + * during a profiling session. + */ + private _setupSpanChunkInstrumentation(): void { + if (!this._client) { + DEBUG_BUILD && + logger.log('[Profiling] Failed to collect profile, sentry client was never attached to the profiler.'); + return; + } + + getGlobalScope().setContext('profile', { + profiler_id: this._profilerId, + }); + + this._client.on('beforeSendEvent', e => this._assignThreadIdContext(e)); + } + + /** + * Clear profiling information from global context when a profile is not running. + */ + private _teardownSpanChunkInstrumentation(): void { + const globalScope = getGlobalScope(); + globalScope.setContext('profile', {}); + } + /** * Initializes new profile chunk metadata */ @@ -305,6 +346,30 @@ class ContinuousProfiler { }; } + /** + * Assigns thread_id and thread name context to a profiled event. + */ + private _assignThreadIdContext(event: Event): any { + if (!event?.['contexts']?.['profile']) { + return; + } + + if (!event.contexts) { + return; + } + + // @ts-expect-error the trace fallback value is wrong, though it should never happen + // and in case it does, we dont want to override whatever was passed initially. + event.contexts['trace'] = { + ...(event.contexts?.['trace'] ?? {}), + data: { + ...(event.contexts?.['trace']?.['data'] ?? {}), + ['thread.id']: PROFILER_THREAD_ID_STRING, + ['thread.name']: PROFILER_THREAD_NAME, + }, + }; + } + /** * Resets the current chunk state. */ diff --git a/packages/profiling-node/src/spanProfileUtils.ts b/packages/profiling-node/src/spanProfileUtils.ts index 1b347a61b741..ee6d29b8bb93 100644 --- a/packages/profiling-node/src/spanProfileUtils.ts +++ b/packages/profiling-node/src/spanProfileUtils.ts @@ -115,7 +115,6 @@ export function stopSpanProfile(span: Span, profile_id: string | undefined): Raw } const profile = CpuProfilerBindings.stopProfiling(profile_id, 0); - DEBUG_BUILD && logger.log(`[Profiling] stopped profiling of transaction: ${spanToJSON(span).description}`); // In case of an overlapping span, stopProfiling may return null and silently ignore the overlapping profile. diff --git a/packages/profiling-node/src/utils.ts b/packages/profiling-node/src/utils.ts index 107debae5c8b..5cb7824387b7 100644 --- a/packages/profiling-node/src/utils.ts +++ b/packages/profiling-node/src/utils.ts @@ -28,8 +28,8 @@ import type { RawChunkCpuProfile, RawThreadCpuProfile } from './types'; // We require the file because if we import it, it will be included in the bundle. // I guess tsc does not check file contents when it's imported. -const THREAD_ID_STRING = String(threadId); -const THREAD_NAME = isMainThread ? 'main' : 'worker'; +export const PROFILER_THREAD_ID_STRING = String(threadId); +export const PROFILER_THREAD_NAME = isMainThread ? 'main' : 'worker'; const FORMAT_VERSION = '1'; const CONTINUOUS_FORMAT_VERSION = '2'; @@ -75,8 +75,8 @@ export function enrichWithThreadInformation( frames: profile.frames, stacks: profile.stacks, thread_metadata: { - [THREAD_ID_STRING]: { - name: THREAD_NAME, + [PROFILER_THREAD_ID_STRING]: { + name: PROFILER_THREAD_NAME, }, }, } as ThreadCpuProfile | ContinuousThreadCpuProfile; @@ -172,7 +172,7 @@ function createProfilePayload( name: transaction, id: event_id, trace_id: trace_id || '', - active_thread_id: THREAD_ID_STRING, + active_thread_id: PROFILER_THREAD_ID_STRING, }, }; diff --git a/packages/profiling-node/test/spanProfileUtils.test.ts b/packages/profiling-node/test/spanProfileUtils.test.ts index 9cc2ae58a972..f2ca1b932e12 100644 --- a/packages/profiling-node/test/spanProfileUtils.test.ts +++ b/packages/profiling-node/test/spanProfileUtils.test.ts @@ -7,8 +7,6 @@ import { GLOBAL_OBJ, createEnvelope, logger } from '@sentry/utils'; import { CpuProfilerBindings } from '../src/cpu_profiler'; import { type ProfilingIntegration, _nodeProfilingIntegration } from '../src/integration'; -jest.setTimeout(10000); - function makeClientWithHooks(): [Sentry.NodeClient, Transport] { const integration = _nodeProfilingIntegration(); const client = new Sentry.NodeClient({ @@ -322,7 +320,6 @@ describe('automated span instrumentation', () => { transaction.end(); expect(stopProfilingSpy).toHaveBeenCalledTimes(1); }); - it('enriches profile with debug_id', async () => { GLOBAL_OBJ._sentryDebugIds = { 'Error\n at filename.js (filename.js:36:15)': 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', @@ -574,6 +571,47 @@ describe('continuous profiling', () => { expect(transportSpy.mock.calls?.[0]?.[0]?.[1]?.[0]?.[0].type).toBe('profile_chunk'); }); + + it('sets global profile context', async () => { + const [client, transport] = makeContinuousProfilingClient(); + Sentry.setCurrentClient(client); + client.init(); + + const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); + + const nonProfiledTransaction = Sentry.startInactiveSpan({ forceTransaction: true, name: 'profile_hub' }); + nonProfiledTransaction.end(); + + expect(transportSpy.mock.calls?.[0]?.[0]?.[1]?.[0]?.[1]).not.toMatchObject({ + contexts: { + profile: {}, + }, + }); + + const integration = client.getIntegrationByName('ProfilingIntegration'); + if (!integration) { + throw new Error('Profiling integration not found'); + } + + integration._profiler.start(); + const profiledTransaction = Sentry.startInactiveSpan({ forceTransaction: true, name: 'profile_hub' }); + profiledTransaction.end(); + integration._profiler.stop(); + + expect(transportSpy.mock.calls?.[1]?.[0]?.[1]?.[0]?.[1]).toMatchObject({ + contexts: { + trace: { + data: expect.objectContaining({ + ['thread.id']: expect.any(String), + ['thread.name']: expect.any(String), + }), + }, + profile: { + profiler_id: expect.any(String), + }, + }, + }); + }); }); describe('span profiling mode', () => { @@ -610,7 +648,6 @@ describe('span profiling mode', () => { Sentry.startInactiveSpan({ forceTransaction: true, name: 'profile_hub' }); expect(startProfilingSpy).toHaveBeenCalled(); - const integration = client.getIntegrationByName('ProfilingIntegration'); if (!integration) { diff --git a/packages/profiling-node/test/spanProfileUtils.worker.test.ts b/packages/profiling-node/test/spanProfileUtils.worker.test.ts new file mode 100644 index 000000000000..a119f80292d5 --- /dev/null +++ b/packages/profiling-node/test/spanProfileUtils.worker.test.ts @@ -0,0 +1,75 @@ +// Mock the modules before the import, so that the value is initialized before the module is loaded +jest.mock('worker_threads', () => { + return { + isMainThread: false, + threadId: 9999, + }; +}); +jest.setTimeout(10000); + +import * as Sentry from '@sentry/node'; +import type { Transport } from '@sentry/types'; +import { type ProfilingIntegration, _nodeProfilingIntegration } from '../src/integration'; + +function makeContinuousProfilingClient(): [Sentry.NodeClient, Transport] { + const integration = _nodeProfilingIntegration(); + const client = new Sentry.NodeClient({ + stackParser: Sentry.defaultStackParser, + tracesSampleRate: 1, + profilesSampleRate: undefined, + debug: true, + environment: 'test-environment', + dsn: 'https://7fa19397baaf433f919fbe02228d5470@o1137848.ingest.sentry.io/6625302', + integrations: [integration], + transport: _opts => + Sentry.makeNodeTransport({ + url: 'https://7fa19397baaf433f919fbe02228d5470@o1137848.ingest.sentry.io/6625302', + recordDroppedEvent: () => { + return undefined; + }, + }), + }); + + return [client, client.getTransport() as Transport]; +} + +it('worker threads context', () => { + const [client, transport] = makeContinuousProfilingClient(); + Sentry.setCurrentClient(client); + client.init(); + + const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); + + const nonProfiledTransaction = Sentry.startInactiveSpan({ forceTransaction: true, name: 'profile_hub' }); + nonProfiledTransaction.end(); + + expect(transportSpy.mock.calls?.[0]?.[0]?.[1]?.[0]?.[1]).not.toMatchObject({ + contexts: { + profile: {}, + }, + }); + + const integration = client.getIntegrationByName('ProfilingIntegration'); + if (!integration) { + throw new Error('Profiling integration not found'); + } + + integration._profiler.start(); + const profiledTransaction = Sentry.startInactiveSpan({ forceTransaction: true, name: 'profile_hub' }); + profiledTransaction.end(); + integration._profiler.stop(); + + expect(transportSpy.mock.calls?.[1]?.[0]?.[1]?.[0]?.[1]).toMatchObject({ + contexts: { + trace: { + data: expect.objectContaining({ + ['thread.id']: '9999', + ['thread.name']: 'worker', + }), + }, + profile: { + profiler_id: expect.any(String), + }, + }, + }); +}); From f032f3f651e80e75d737413026c38d6c1a725922 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 12 Jun 2024 16:52:38 +0200 Subject: [PATCH 03/29] fix(node): Ensure status is correct for http server span errors (#12477) I noticed that we were not correctly setting a status message based on http status for errored http.server span. The problem was in our logic, where we already had the status set to `{ code: 2, message: undefined }`, and whenever the code was not 0 (unset), we'd never look at the attributes to infer the proper status message. Now, if we have code: 2 but not message, we try to infer the message from the attributes, if possible. --- .../node-express/tests/transactions.test.ts | 82 ++++++++++++++++++- packages/opentelemetry/src/utils/mapStatus.ts | 36 ++++++-- .../test/utils/mapStatus.test.ts | 13 +++ 3 files changed, 121 insertions(+), 10 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts index 8e7fb9fce515..03262edcf6b5 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts @@ -12,7 +12,6 @@ test('Sends an API route transaction', async ({ baseURL }) => { await fetch(`${baseURL}/test-transaction`); const transactionEvent = await pageloadTransactionEventPromise; - const transactionEventId = transactionEvent.event_id; expect(transactionEvent.contexts?.trace).toEqual({ data: { @@ -119,3 +118,84 @@ test('Sends an API route transaction', async ({ baseURL }) => { trace_id: expect.any(String), }); }); + +test('Sends an API route transaction for an errored route', async ({ baseURL }) => { + const transactionEventPromise = waitForTransaction('node-express', transactionEvent => { + return ( + transactionEvent.contexts?.trace?.op === 'http.server' && + transactionEvent.transaction === 'GET /test-exception/:id' && + transactionEvent.request?.url === 'http://localhost:3030/test-exception/777' + ); + }); + + await fetch(`${baseURL}/test-exception/777`); + + const transactionEvent = await transactionEventPromise; + + expect(transactionEvent.contexts?.trace?.op).toEqual('http.server'); + expect(transactionEvent.transaction).toEqual('GET /test-exception/:id'); + expect(transactionEvent.contexts?.trace?.status).toEqual('internal_error'); + expect(transactionEvent.contexts?.trace?.data?.['http.status_code']).toEqual(500); + + const spans = transactionEvent.spans || []; + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', + 'http.route': '/', + 'express.name': 'query', + 'express.type': 'middleware', + 'otel.kind': 'INTERNAL', + }, + description: 'query', + op: 'middleware.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', + 'http.route': '/', + 'express.name': 'expressInit', + 'express.type': 'middleware', + 'otel.kind': 'INTERNAL', + }, + description: 'expressInit', + op: 'middleware.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'request_handler.express', + 'http.route': '/test-exception/:id', + 'express.name': '/test-exception/:id', + 'express.type': 'request_handler', + 'otel.kind': 'INTERNAL', + }, + description: '/test-exception/:id', + op: 'request_handler.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); +}); diff --git a/packages/opentelemetry/src/utils/mapStatus.ts b/packages/opentelemetry/src/utils/mapStatus.ts index 1f65fcb985bb..8df486437d6a 100644 --- a/packages/opentelemetry/src/utils/mapStatus.ts +++ b/packages/opentelemetry/src/utils/mapStatus.ts @@ -1,7 +1,7 @@ import { SpanStatusCode } from '@opentelemetry/api'; import { SEMATTRS_HTTP_STATUS_CODE, SEMATTRS_RPC_GRPC_STATUS_CODE } from '@opentelemetry/semantic-conventions'; import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, getSpanStatusFromHttpCode } from '@sentry/core'; -import type { SpanStatus } from '@sentry/types'; +import type { SpanAttributes, SpanStatus } from '@sentry/types'; import type { AbstractSpan } from '../types'; import { spanHasAttributes, spanHasStatus } from './spanTypes'; @@ -43,7 +43,14 @@ export function mapStatus(span: AbstractSpan): SpanStatus { return { code: SPAN_STATUS_OK }; // If the span is already marked as erroneous we return that exact status } else if (status.code === SpanStatusCode.ERROR) { - if (typeof status.message === 'undefined' || isStatusErrorMessageValid(status.message)) { + if (typeof status.message === 'undefined') { + const inferredStatus = inferStatusFromAttributes(attributes); + if (inferredStatus) { + return inferredStatus; + } + } + + if (status.message && isStatusErrorMessageValid(status.message)) { return { code: SPAN_STATUS_ERROR, message: status.message }; } else { return { code: SPAN_STATUS_ERROR, message: 'unknown_error' }; @@ -51,6 +58,22 @@ export function mapStatus(span: AbstractSpan): SpanStatus { } } + // If the span status is UNSET, we try to infer it from HTTP or GRPC status codes. + const inferredStatus = inferStatusFromAttributes(attributes); + + if (inferredStatus) { + return inferredStatus; + } + + // We default to setting the spans status to ok. + if (status && status.code === SpanStatusCode.UNSET) { + return { code: SPAN_STATUS_OK }; + } else { + return { code: SPAN_STATUS_ERROR, message: 'unknown_error' }; + } +} + +function inferStatusFromAttributes(attributes: SpanAttributes): SpanStatus | undefined { // If the span status is UNSET, we try to infer it from HTTP or GRPC status codes. const httpCodeAttribute = attributes[SEMATTRS_HTTP_STATUS_CODE]; @@ -63,7 +86,7 @@ export function mapStatus(span: AbstractSpan): SpanStatus { ? parseInt(httpCodeAttribute) : undefined; - if (numberHttpCode) { + if (typeof numberHttpCode === 'number') { return getSpanStatusFromHttpCode(numberHttpCode); } @@ -71,10 +94,5 @@ export function mapStatus(span: AbstractSpan): SpanStatus { return { code: SPAN_STATUS_ERROR, message: canonicalGrpcErrorCodesMap[grpcCodeAttribute] || 'unknown_error' }; } - // We default to setting the spans status to ok. - if (status && status.code === SpanStatusCode.UNSET) { - return { code: SPAN_STATUS_OK }; - } else { - return { code: SPAN_STATUS_ERROR, message: 'unknown_error' }; - } + return undefined; } diff --git a/packages/opentelemetry/test/utils/mapStatus.test.ts b/packages/opentelemetry/test/utils/mapStatus.test.ts index 8dcf55a9267b..1734586ee7a2 100644 --- a/packages/opentelemetry/test/utils/mapStatus.test.ts +++ b/packages/opentelemetry/test/utils/mapStatus.test.ts @@ -91,6 +91,19 @@ describe('mapStatus', () => { expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'invalid_argument' }); }); + it('returns error status when span already has error status without message', () => { + const span = createSpan(); + span.setStatus({ code: 2 }); // ERROR + expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'unknown_error' }); + }); + + it('infers error status form attributes when span already has error status without message', () => { + const span = createSpan(); + span.setAttribute(SEMATTRS_HTTP_STATUS_CODE, 500); + span.setStatus({ code: 2 }); // ERROR + expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + }); + it('returns unknown error status when code is unknown', () => { const span = createSpan(); span.setStatus({ code: -1 as 0 }); From 60dad3441faa9286079d418bb11751f523319e1f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 13 Jun 2024 13:28:26 +0200 Subject: [PATCH 04/29] ci: Add workflow to automatically add external contributors to CHANGELOG.md (#12428) This adds a new GHA job that, if a PR is created by an external contributor, it will open a PR against that branch that adds the contributor to the changelog, so we do not forget about this. It will open a PR like this: https://github.com/getsentry/sentry-javascript/pull/12435 which we have to manually merge/review, so nothing "bad" can happen. --- .github/workflows/build.yml | 38 ++++++++++ .../.eslintrc.cjs | 14 ++++ .../external-contributor-gh-action/action.yml | 9 +++ .../external-contributor-gh-action/index.mjs | 72 +++++++++++++++++++ .../package.json | 22 ++++++ package.json | 1 + 6 files changed, 156 insertions(+) create mode 100644 dev-packages/external-contributor-gh-action/.eslintrc.cjs create mode 100644 dev-packages/external-contributor-gh-action/action.yml create mode 100644 dev-packages/external-contributor-gh-action/index.mjs create mode 100644 dev-packages/external-contributor-gh-action/package.json diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bd039420fc3d..53b8f9e32e52 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -224,6 +224,44 @@ jobs: message: | ⚠️ This PR is opened against **master**. You probably want to open it against **develop**. + job_external_contributor: + name: External Contributors + needs: job_install_deps + runs-on: ubuntu-20.04 + if: | + github.event_name == 'pull_request' + && (github.action == 'opened' || github.action == 'reopened') + && github.event.pull_request.author_association != 'COLLABORATOR' + && github.event.pull_request.author_association != 'MEMBER' + && github.event.pull_request.author_association != 'OWNER' + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.head_ref }} + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version-file: 'package.json' + - name: Check dependency cache + uses: actions/cache/restore@v4 + with: + path: ${{ env.CACHED_DEPENDENCY_PATHS }} + key: ${{ needs.job_install_deps.outputs.dependency_cache_key }} + fail-on-cache-miss: true + + - name: Add external contributor to CHANGELOG.md + uses: ./dev-packages/external-contributor-gh-action + with: + name: ${{ github.event.pull_request.user.login }} + - name: Create PR with changes + uses: peter-evans/create-pull-request@v6 + with: + commit-message: "ref: Add external contributor to CHANGELOG.md" + title: "ref: Add external contributor to CHANGELOG.md" + branch: 'external-contributor/patch-${{ github.event.pull_request.user.login }}' + delete-branch: true + body: This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. + job_build: name: Build needs: [job_get_metadata, job_install_deps] diff --git a/dev-packages/external-contributor-gh-action/.eslintrc.cjs b/dev-packages/external-contributor-gh-action/.eslintrc.cjs new file mode 100644 index 000000000000..8c67e0037908 --- /dev/null +++ b/dev-packages/external-contributor-gh-action/.eslintrc.cjs @@ -0,0 +1,14 @@ +module.exports = { + extends: ['../../.eslintrc.js'], + parserOptions: { + sourceType: 'module', + ecmaVersion: 'latest', + }, + + overrides: [ + { + files: ['*.mjs'], + extends: ['@sentry-internal/sdk/src/base'], + }, + ], +}; diff --git a/dev-packages/external-contributor-gh-action/action.yml b/dev-packages/external-contributor-gh-action/action.yml new file mode 100644 index 000000000000..8c6fb9c04944 --- /dev/null +++ b/dev-packages/external-contributor-gh-action/action.yml @@ -0,0 +1,9 @@ +name: 'external-contributor-gh-action' +description: 'Add external contributors to CHANGELOG.md' +inputs: + name: + required: true + description: 'The name of the external contributor' +runs: + using: 'node20' + main: 'index.mjs' diff --git a/dev-packages/external-contributor-gh-action/index.mjs b/dev-packages/external-contributor-gh-action/index.mjs new file mode 100644 index 000000000000..7eff418e9205 --- /dev/null +++ b/dev-packages/external-contributor-gh-action/index.mjs @@ -0,0 +1,72 @@ +import { promises as fs } from 'node:fs'; +import path from 'node:path'; +import * as core from '@actions/core'; + +const UNRELEASED_HEADING = `## Unreleased + +- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +`; + +const contributorMessageRegex = /Work in this release was contributed by (.+)\. Thank you for your contribution!/; + +async function run() { + const { getInput } = core; + + const name = getInput('name'); + + if (!name) { + return; + } + + const ghUserName = name.startsWith('@') ? name : `@${name}`; + + const cwd = process.cwd(); + const changelogFilePath = path.resolve(cwd, 'CHANGELOG.md'); + + const changelogStr = await fs.readFile(changelogFilePath, 'utf8'); + + // Find the unreleased section + const start = changelogStr.indexOf(UNRELEASED_HEADING) + UNRELEASED_HEADING.length; + const end = changelogStr.slice(start).indexOf('## '); + + const inBetween = changelogStr.slice(start, start + end); + + const existing = contributorMessageRegex.exec(inBetween); + + // If the contributor message already exists, add the new contributor to the list + if (existing) { + const users = existing[1].split(/(?:,? and )|(?:, )/); + if (!users.includes(ghUserName)) { + users.push(ghUserName); + } + + const formatter = new Intl.ListFormat('en', { + style: 'long', + type: 'conjunction', + }); + + const newContributors = formatter.format(users); + const newChangelog = changelogStr.replace( + contributorMessageRegex, + `Work in this release was contributed by ${newContributors}. Thank you for your contribution!`, + ); + + fs.writeFile(changelogFilePath, newChangelog); + + // eslint-disable-next-line no-console + console.log('Added contributor to list of existing contributors.'); + return; + } + + // If the contributor message does not exist, add it + const newChangelog = changelogStr.replace( + UNRELEASED_HEADING, + `${UNRELEASED_HEADING}\nWork in this release was contributed by ${ghUserName}. Thank you for your contribution!\n`, + ); + fs.writeFile(changelogFilePath, newChangelog); + + // eslint-disable-next-line no-console + console.log('Added contributor message.'); +} + +run(); diff --git a/dev-packages/external-contributor-gh-action/package.json b/dev-packages/external-contributor-gh-action/package.json new file mode 100644 index 000000000000..6b69533b7ba2 --- /dev/null +++ b/dev-packages/external-contributor-gh-action/package.json @@ -0,0 +1,22 @@ +{ + "name": "@sentry-internal/external-contributor-gh-action", + "description": "An internal Github Action to add external contributors to the CHANGELOG.md file.", + "version": "8.8.0", + "license": "MIT", + "engines": { + "node": ">=18" + }, + "private": true, + "main": "index.mjs", + "type": "module", + "scripts": { + "lint": "eslint . --format stylish", + "fix": "eslint . --format stylish --fix" + }, + "dependencies": { + "@actions/core": "1.10.1" + }, + "volta": { + "extends": "../../package.json" + } +} diff --git a/package.json b/package.json index bd4246a45862..9f5afeb9047d 100644 --- a/package.json +++ b/package.json @@ -85,6 +85,7 @@ "dev-packages/overhead-metrics", "dev-packages/test-utils", "dev-packages/size-limit-gh-action", + "dev-packages/external-contributor-gh-action", "dev-packages/rollup-utils" ], "devDependencies": { From d94695d24f5ce20a46536d8424957eb2c73689ac Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:00:28 -0400 Subject: [PATCH 05/29] feat(feedback): Screenshots don't resize after cropping (#12481) Screenshots will no longer become larger to fit the screenshot area after being cropped, instead it would remain the same size. The image quality of the screenshot goes down if the image is cropped too much. This could help make small crops a bit more readable. Before: Select a speeder After: Pasted Graphic Relates to https://github.com/getsentry/sentry-javascript/issues/12329 --- packages/feedback/src/modal/components/Dialog.css.ts | 3 +-- .../src/screenshot/components/ScreenshotEditor.tsx | 8 +++++--- .../src/screenshot/components/ScreenshotInput.css.ts | 6 ++++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/feedback/src/modal/components/Dialog.css.ts b/packages/feedback/src/modal/components/Dialog.css.ts index ddcd90184657..76ac68f0d6d1 100644 --- a/packages/feedback/src/modal/components/Dialog.css.ts +++ b/packages/feedback/src/modal/components/Dialog.css.ts @@ -100,13 +100,12 @@ const FORM = ` } .form__right { - width: var(--form-width, 272px); + min-width: var(--form-width, 272px); display: flex; overflow: auto; flex-direction: column; justify-content: space-between; gap: 20px; - flex: 1 0 auto; } @media (max-width: 600px) { diff --git a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx index da4217ca5d46..08bbdb780f45 100644 --- a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx +++ b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx @@ -94,8 +94,6 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: if (cropButton) { cropButton.style.width = `${imageDimensions.width}px`; cropButton.style.height = `${imageDimensions.height}px`; - cropButton.style.left = `${imageDimensions.x}px`; - cropButton.style.top = `${imageDimensions.y}px`; } setCroppingRect({ startX: 0, startY: 0, endX: imageDimensions.width, endY: imageDimensions.height }); @@ -212,6 +210,8 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: ctx.clearRect(0, 0, imageBuffer.width, imageBuffer.height); imageBuffer.width = cutoutCanvas.width; imageBuffer.height = cutoutCanvas.height; + imageBuffer.style.width = `${cutoutCanvas.width}px`; + imageBuffer.style.height = `${cutoutCanvas.height}px`; ctx.drawImage(cutoutCanvas, 0, 0); resizeCropper(); } @@ -229,6 +229,8 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: } imageBuffer.width = imageSource.videoWidth; imageBuffer.height = imageSource.videoHeight; + imageBuffer.style.width = '100%'; + imageBuffer.style.height = '100%'; context.drawImage(imageSource, 0, 0); }, [imageBuffer], @@ -249,7 +251,7 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: