From b6e6634268ac18c93dee499c5a9038cd440973ec Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 2 Aug 2024 12:04:32 +0200 Subject: [PATCH 1/7] fix(nuxt): Detect pageload by adding flag in Vue --- .../nuxt-3/playwright.config.ts | 3 ++ .../nuxt-3/tests/performance.client.test.ts | 31 +++++++++++++++++++ .../nuxt-3/tests/performance.server.test.ts | 2 +- packages/vue/src/router.ts | 14 ++++++--- 4 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.client.test.ts diff --git a/dev-packages/e2e-tests/test-applications/nuxt-3/playwright.config.ts b/dev-packages/e2e-tests/test-applications/nuxt-3/playwright.config.ts index f270a5ad9b48..d1094993131d 100644 --- a/dev-packages/e2e-tests/test-applications/nuxt-3/playwright.config.ts +++ b/dev-packages/e2e-tests/test-applications/nuxt-3/playwright.config.ts @@ -8,6 +8,9 @@ const nuxtConfigOptions: ConfigOptions = { }, }; +/* Make sure to import from '@nuxt/test-utils/playwright' in the tests + * Like this: import { expect, test } from '@nuxt/test-utils/playwright' */ + const config = getPlaywrightConfig({ startCommand: `pnpm preview`, use: { ...nuxtConfigOptions }, diff --git a/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.client.test.ts b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.client.test.ts new file mode 100644 index 000000000000..66c8c9dfce2d --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.client.test.ts @@ -0,0 +1,31 @@ +import { expect, test } from '@nuxt/test-utils/playwright'; +import { waitForTransaction } from '@sentry-internal/test-utils'; + +test('sends a pageload root span with a parameterized URL', async ({ page }) => { + const transactionPromise = waitForTransaction('nuxt-3', async transactionEvent => { + return transactionEvent.transaction === '/test-param/:param()'; + }); + + await page.goto(`/test-param/1234`); + + const rootSpan = await transactionPromise; + + expect(rootSpan).toMatchObject({ + contexts: { + trace: { + data: { + 'sentry.source': 'route', + 'sentry.origin': 'auto.pageload.vue', + 'sentry.op': 'pageload', + 'params.param': '1234', + }, + op: 'pageload', + origin: 'auto.pageload.vue', + }, + }, + transaction: '/test-param/:param()', + transaction_info: { + source: 'route', + }, + }); +}); diff --git a/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts index 5b78e235e564..b8a8007af3af 100644 --- a/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts @@ -1,4 +1,4 @@ -import { expect, test } from '@playwright/test'; +import { expect, test } from '@nuxt/test-utils/playwright'; import { waitForTransaction } from '@sentry-internal/test-utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index e54c71eb550f..956dcb421764 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -50,18 +50,22 @@ export function instrumentVueRouter( }, startNavigationSpanFn: (context: StartSpanOptions) => void, ): void { + let IS_FIRST_PAGE_LOAD = true; + router.onError(error => captureException(error, { mechanism: { handled: false } })); router.beforeEach((to, from, next) => { // According to docs we could use `from === VueRouter.START_LOCATION` but I couldnt get it working for Vue 2 // https://router.vuejs.org/api/#router-start-location // https://next.router.vuejs.org/api/#start-location + // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (like it was the case before). + // Therefore, a flag was added to track the page-load: IS_FIRST_PAGE_LOAD + + const isPageLoadNavigation = IS_FIRST_PAGE_LOAD; - // from.name: - // - Vue 2: null - // - Vue 3: undefined - // hence only '==' instead of '===', because `undefined == null` evaluates to `true` - const isPageLoadNavigation = from.name == null && from.matched.length === 0; + if (IS_FIRST_PAGE_LOAD) { + IS_FIRST_PAGE_LOAD = false; + } const attributes: SpanAttributes = { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.vue', From e31e6c92e529d6ef78b0d95ef9322bd00a7142f4 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 2 Aug 2024 13:33:01 +0200 Subject: [PATCH 2/7] make vue unit tests running again --- packages/vue/src/router.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index 956dcb421764..092ef7c34104 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -61,7 +61,12 @@ export function instrumentVueRouter( // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (like it was the case before). // Therefore, a flag was added to track the page-load: IS_FIRST_PAGE_LOAD - const isPageLoadNavigation = IS_FIRST_PAGE_LOAD; + // from.name: + // - Vue 2: null + // - Vue 3: undefined + // - Nuxt: same as to.name (always a value) + // hence only '==' instead of '===', because `undefined == null` evaluates to `true` + const isPageLoadNavigation = (from.name == null && from.matched.length === 0) || (from.name && IS_FIRST_PAGE_LOAD); if (IS_FIRST_PAGE_LOAD) { IS_FIRST_PAGE_LOAD = false; From 1877074bcd93bc19cfe8053d4cfba609e8c6fa7b Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 2 Aug 2024 13:36:38 +0200 Subject: [PATCH 3/7] fix comment --- packages/vue/src/router.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index 092ef7c34104..b973afe9ab4a 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -58,7 +58,7 @@ export function instrumentVueRouter( // According to docs we could use `from === VueRouter.START_LOCATION` but I couldnt get it working for Vue 2 // https://router.vuejs.org/api/#router-start-location // https://next.router.vuejs.org/api/#start-location - // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (like it was the case before). + // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (this is never 0). // Therefore, a flag was added to track the page-load: IS_FIRST_PAGE_LOAD // from.name: From dce66f5a54b67af4f96fd37f6a6e6a6b0c188750 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 2 Aug 2024 14:59:03 +0200 Subject: [PATCH 4/7] review comments --- packages/vue/src/router.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index b973afe9ab4a..9b7f6376eb00 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -50,7 +50,7 @@ export function instrumentVueRouter( }, startNavigationSpanFn: (context: StartSpanOptions) => void, ): void { - let IS_FIRST_PAGE_LOAD = true; + let isFirstPageLoad = true; router.onError(error => captureException(error, { mechanism: { handled: false } })); @@ -64,12 +64,13 @@ export function instrumentVueRouter( // from.name: // - Vue 2: null // - Vue 3: undefined - // - Nuxt: same as to.name (always a value) + // - Nuxt: undefined // hence only '==' instead of '===', because `undefined == null` evaluates to `true` - const isPageLoadNavigation = (from.name == null && from.matched.length === 0) || (from.name && IS_FIRST_PAGE_LOAD); + const isPageLoadNavigation = + (from.name == null && from.matched.length === 0) || (from.name === undefined && isFirstPageLoad); - if (IS_FIRST_PAGE_LOAD) { - IS_FIRST_PAGE_LOAD = false; + if (isFirstPageLoad) { + isFirstPageLoad = false; } const attributes: SpanAttributes = { From 8fb974b4d6366b9318f5c8dbb87b43f8462fd16a Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 2 Aug 2024 16:13:16 +0200 Subject: [PATCH 5/7] fix vue unit tests --- packages/vue/test/router.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts index dc69d7ae0fd9..1da5097b11e0 100644 --- a/packages/vue/test/router.test.ts +++ b/packages/vue/test/router.test.ts @@ -114,6 +114,7 @@ describe('instrumentVueRouter()', () => { const from = testRoutes[fromKey]!; const to = testRoutes[toKey]!; + beforeEachCallback(to, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload beforeEachCallback(to, from, mockNext); expect(mockStartSpan).toHaveBeenCalledTimes(1); @@ -127,7 +128,7 @@ describe('instrumentVueRouter()', () => { op: 'navigation', }); - expect(mockNext).toHaveBeenCalledTimes(1); + expect(mockNext).toHaveBeenCalledTimes(2); }, ); @@ -192,6 +193,7 @@ describe('instrumentVueRouter()', () => { const from = testRoutes.normalRoute1!; const to = testRoutes.namedRoute!; + beforeEachCallback(to, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload beforeEachCallback(to, from, mockNext); // first startTx call happens when the instrumentation is initialized (for pageloads) @@ -219,6 +221,7 @@ describe('instrumentVueRouter()', () => { const from = testRoutes.normalRoute1!; const to = testRoutes.namedRoute!; + beforeEachCallback(to, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload beforeEachCallback(to, from, mockNext); // first startTx call happens when the instrumentation is initialized (for pageloads) @@ -373,6 +376,7 @@ describe('instrumentVueRouter()', () => { expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1); const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0]![0]!; + beforeEachCallback(testRoutes['normalRoute1']!, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload beforeEachCallback(testRoutes['normalRoute2']!, testRoutes['normalRoute1']!, mockNext); expect(mockStartSpan).toHaveBeenCalledTimes(expectedCallsAmount); @@ -391,6 +395,7 @@ describe('instrumentVueRouter()', () => { const from = testRoutes.normalRoute1!; const to = testRoutes.namedRoute!; + beforeEachCallback(to, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload beforeEachCallback(to, from, undefined); // first startTx call happens when the instrumentation is initialized (for pageloads) From af14d4fb9d0263897dcc1a03340e186f13adbbf5 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Mon, 5 Aug 2024 10:03:18 +0200 Subject: [PATCH 6/7] change variable name in comment --- packages/vue/src/router.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index 9b7f6376eb00..6427eefe158e 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -60,6 +60,7 @@ export function instrumentVueRouter( // https://next.router.vuejs.org/api/#start-location // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (this is never 0). // Therefore, a flag was added to track the page-load: IS_FIRST_PAGE_LOAD + // Therefore, a flag was added to track the page-load: isFirstPageLoad // from.name: // - Vue 2: null From 90ef6e13cbe5aecf4705ba342c386219dab04982 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Mon, 5 Aug 2024 10:57:27 +0200 Subject: [PATCH 7/7] fix tests; change to logger --- .../nuxt-3/tests/performance.server.test.ts | 2 +- packages/nuxt/src/server/sdk.ts | 6 ++++-- packages/vue/src/router.ts | 3 +-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts index b8a8007af3af..5b78e235e564 100644 --- a/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/nuxt-3/tests/performance.server.test.ts @@ -1,4 +1,4 @@ -import { expect, test } from '@nuxt/test-utils/playwright'; +import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; diff --git a/packages/nuxt/src/server/sdk.ts b/packages/nuxt/src/server/sdk.ts index f14cc23ab8cd..deadea3c54df 100644 --- a/packages/nuxt/src/server/sdk.ts +++ b/packages/nuxt/src/server/sdk.ts @@ -1,6 +1,8 @@ import { applySdkMetadata, getGlobalScope } from '@sentry/core'; import { init as initNode } from '@sentry/node'; import type { Client, EventProcessor } from '@sentry/types'; +import { logger } from '@sentry/utils'; +import { DEBUG_BUILD } from '../common/debug-build'; import type { SentryNuxtOptions } from '../common/types'; /** @@ -26,8 +28,8 @@ export function init(options: SentryNuxtOptions): Client | undefined { // todo: the buildAssetDir could be changed in the nuxt config - change this to a more generic solution if (event.transaction?.match(/^GET \/_nuxt\//)) { options.debug && - // eslint-disable-next-line no-console - console.log('[Sentry] NuxtLowQualityTransactionsFilter filtered transaction: ', event.transaction); + DEBUG_BUILD && + logger.log('NuxtLowQualityTransactionsFilter filtered transaction: ', event.transaction); return null; } diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index 6427eefe158e..8e8bf32ac172 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -55,11 +55,10 @@ export function instrumentVueRouter( router.onError(error => captureException(error, { mechanism: { handled: false } })); router.beforeEach((to, from, next) => { - // According to docs we could use `from === VueRouter.START_LOCATION` but I couldnt get it working for Vue 2 + // According to docs we could use `from === VueRouter.START_LOCATION` but I couldn't get it working for Vue 2 // https://router.vuejs.org/api/#router-start-location // https://next.router.vuejs.org/api/#start-location // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (this is never 0). - // Therefore, a flag was added to track the page-load: IS_FIRST_PAGE_LOAD // Therefore, a flag was added to track the page-load: isFirstPageLoad // from.name: