From 06c9d35031a777d764c2699f48bb3af6fa9afe41 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 12:03:08 +0200 Subject: [PATCH 01/12] test(browser): Fix flaky test --- .../pageloadWithChildSpanTimeout/test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts index 5987061c741f..ca342a5533c2 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts @@ -21,6 +21,7 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn const eventData = envelopeRequestParser(req); expect(eventData.contexts?.trace?.op).toBe('pageload'); + expect(eventData.contexts?.trace?.data?.['sentry.idle_span_discarded_spans']).toBeUndefined(); expect(eventData.spans?.length).toBeGreaterThanOrEqual(1); const testSpan = eventData.spans?.find(span => span.description === 'pageload-child-span'); expect(testSpan).toBeDefined(); From 921ab6ad1b47b2dac1d1dfa31269acb5ea170c17 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 12:17:24 +0200 Subject: [PATCH 02/12] WIP debug this... --- .../pageloadWithChildSpanTimeout/init.js | 1 + .../pageloadWithChildSpanTimeout/test.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js index 98e297d13625..a78618158398 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js @@ -12,6 +12,7 @@ Sentry.init({ ], defaultIntegrations: false, tracesSampleRate: 1, + debug: true, }); const activeSpan = Sentry.getActiveSpan(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts index ca342a5533c2..a08bf90d09f3 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts @@ -15,6 +15,8 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn sentryTest.skip(); } + page.on('console', msg => console.log(msg.text())); + const url = await getLocalTestUrl({ testDir: __dirname }); const req = await waitForTransactionRequestOnUrl(page, url); From 29fc11e306a77c4c8f0ba47dcc5a2ae6aac658f6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 12:33:31 +0200 Subject: [PATCH 03/12] WIP run flaky detector in bundle mode... --- .github/workflows/flaky-test-detector.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/flaky-test-detector.yml b/.github/workflows/flaky-test-detector.yml index 44edf51fd45d..7be4a126ff2b 100644 --- a/.github/workflows/flaky-test-detector.yml +++ b/.github/workflows/flaky-test-detector.yml @@ -85,6 +85,7 @@ jobs: env: CHANGED_TEST_PATHS: ${{ steps.changed.outputs.browser_integration_files }} TEST_RUN_COUNT: 'AUTO' + PW_BUNDLE: bundle_tracing_replay_feedback - name: Artifacts upload uses: actions/upload-artifact@v4 From 5253d189331735aa6827667023cbab0e90fca83d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 13:03:52 +0200 Subject: [PATCH 04/12] more debug --- packages/core/src/tracing/idleSpan.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 8f3ecb7e7d35..a548f1228777 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -268,7 +268,17 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } const childSpanJSON = spanToJSON(childSpan); - const { timestamp: childEndTimestamp = 0, start_timestamp: childStartTimestamp = 0 } = childSpanJSON; + const { timestamp: childEndTimestamp, start_timestamp: childStartTimestamp } = childSpanJSON; + + if (!childStartTimestamp || !childEndTimestamp) { + DEBUG_BUILD && + logger.log( + '[Tracing] Discarding span since it has no start or end timestamp', + JSON.stringify(childSpan, undefined, 2), + ); + + return; + } const spanStartedBeforeIdleSpanEnd = childStartTimestamp <= endTimestamp; @@ -279,7 +289,11 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti if (DEBUG_BUILD) { const stringifiedSpan = JSON.stringify(childSpan, undefined, 2); if (!spanStartedBeforeIdleSpanEnd) { - logger.log('[Tracing] Discarding span since it happened after idle span was finished', stringifiedSpan); + logger.log( + '[Tracing] Discarding span since it happened after idle span was finished', + stringifiedSpan, + endTimestamp, + ); } else if (!spanEndedBeforeFinalTimeout) { logger.log('[Tracing] Discarding span since it finished after idle span final timeout', stringifiedSpan); } From 8c2e4d05c78767b0e0a5a58e7ac58139ef3abe01 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 13:05:08 +0200 Subject: [PATCH 05/12] maybe fix it?? --- packages/core/src/tracing/idleSpan.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index a548f1228777..d1ad85ae261c 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -130,7 +130,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti // If we have no spans, we just end, nothing else to do here if (!spans.length) { onIdleSpanEnded(spanEndTimestamp); - return Reflect.apply(target, thisArg, args); + return Reflect.apply(target, thisArg, [spanEndTimestamp]); } const childEndTimestamps = spans From dcb905f4edd7b41778ddf66f76627594c5dd35a5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 14:30:13 +0200 Subject: [PATCH 06/12] Revert "WIP run flaky detector in bundle mode..." This reverts commit 61dc634088b51fb3823ff7e5a0fffe29ff39c1e3. --- .github/workflows/flaky-test-detector.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/flaky-test-detector.yml b/.github/workflows/flaky-test-detector.yml index 7be4a126ff2b..44edf51fd45d 100644 --- a/.github/workflows/flaky-test-detector.yml +++ b/.github/workflows/flaky-test-detector.yml @@ -85,7 +85,6 @@ jobs: env: CHANGED_TEST_PATHS: ${{ steps.changed.outputs.browser_integration_files }} TEST_RUN_COUNT: 'AUTO' - PW_BUNDLE: bundle_tracing_replay_feedback - name: Artifacts upload uses: actions/upload-artifact@v4 From df9e5f5c3ef748aa7d8bdbc25229714c2332d745 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 14:30:18 +0200 Subject: [PATCH 07/12] Revert "more debug" This reverts commit 7a2f1587857ba9ca2a3eec6e47cb95dfee2728b3. --- packages/core/src/tracing/idleSpan.ts | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index d1ad85ae261c..8db67413d8e4 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -268,17 +268,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } const childSpanJSON = spanToJSON(childSpan); - const { timestamp: childEndTimestamp, start_timestamp: childStartTimestamp } = childSpanJSON; - - if (!childStartTimestamp || !childEndTimestamp) { - DEBUG_BUILD && - logger.log( - '[Tracing] Discarding span since it has no start or end timestamp', - JSON.stringify(childSpan, undefined, 2), - ); - - return; - } + const { timestamp: childEndTimestamp = 0, start_timestamp: childStartTimestamp = 0 } = childSpanJSON; const spanStartedBeforeIdleSpanEnd = childStartTimestamp <= endTimestamp; @@ -289,11 +279,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti if (DEBUG_BUILD) { const stringifiedSpan = JSON.stringify(childSpan, undefined, 2); if (!spanStartedBeforeIdleSpanEnd) { - logger.log( - '[Tracing] Discarding span since it happened after idle span was finished', - stringifiedSpan, - endTimestamp, - ); + logger.log('[Tracing] Discarding span since it happened after idle span was finished', stringifiedSpan); } else if (!spanEndedBeforeFinalTimeout) { logger.log('[Tracing] Discarding span since it finished after idle span final timeout', stringifiedSpan); } From 2db21888a44ab2ff2dfb342eb2e3f01f666a35b9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 14:30:24 +0200 Subject: [PATCH 08/12] Revert "WIP debug this..." This reverts commit 5df26884c63994524e51fe4cdc51fef0021e23b2. --- .../pageloadWithChildSpanTimeout/init.js | 1 - .../pageloadWithChildSpanTimeout/test.ts | 2 -- 2 files changed, 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js index a78618158398..98e297d13625 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js @@ -12,7 +12,6 @@ Sentry.init({ ], defaultIntegrations: false, tracesSampleRate: 1, - debug: true, }); const activeSpan = Sentry.getActiveSpan(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts index a08bf90d09f3..ca342a5533c2 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts @@ -15,8 +15,6 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn sentryTest.skip(); } - page.on('console', msg => console.log(msg.text())); - const url = await getLocalTestUrl({ testDir: __dirname }); const req = await waitForTransactionRequestOnUrl(page, url); From e9b0824f807f7aa1bf3e6f85b343d3cd5cba7ee7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 May 2024 14:38:28 +0200 Subject: [PATCH 09/12] make more robust --- packages/core/src/tracing/idleSpan.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 8db67413d8e4..67093076f443 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -121,7 +121,9 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti beforeSpanEnd(span); } - const timestamp = args[0] || timestampInSeconds(); + // Just ensuring that this keeps working, even if we ever have more arguments here + const [definedEndTimestamp, ...rest] = args; + const timestamp = definedEndTimestamp || timestampInSeconds(); const spanEndTimestamp = spanTimeInputToSeconds(timestamp); // Ensure we end with the last span timestamp, if possible @@ -130,7 +132,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti // If we have no spans, we just end, nothing else to do here if (!spans.length) { onIdleSpanEnded(spanEndTimestamp); - return Reflect.apply(target, thisArg, [spanEndTimestamp]); + return Reflect.apply(target, thisArg, [spanEndTimestamp, ...rest]); } const childEndTimestamps = spans @@ -152,7 +154,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti ); onIdleSpanEnded(endTimestamp); - return Reflect.apply(target, thisArg, [endTimestamp]); + return Reflect.apply(target, thisArg, [endTimestamp, ...rest]); }, }); From 8cb3053944689d3568cb7d6a97db8e5da32cbefa Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Jun 2024 10:53:08 +0200 Subject: [PATCH 10/12] WIP debug flaky test? --- .../pageloadWithChildSpanTimeout/init.js | 1 + .../pageloadWithChildSpanTimeout/test.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js index 98e297d13625..d08efe5fc303 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js @@ -12,6 +12,7 @@ Sentry.init({ ], defaultIntegrations: false, tracesSampleRate: 1, + debug: true }); const activeSpan = Sentry.getActiveSpan(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts index ca342a5533c2..a08bf90d09f3 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts @@ -15,6 +15,8 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn sentryTest.skip(); } + page.on('console', msg => console.log(msg.text())); + const url = await getLocalTestUrl({ testDir: __dirname }); const req = await waitForTransactionRequestOnUrl(page, url); From 4fea67ea9cbd613df10fe332f87745e4cf27d215 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Jun 2024 11:52:57 +0200 Subject: [PATCH 11/12] fix it ??? --- .../pageloadWithChildSpanTimeout/init.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js index d08efe5fc303..be0f1e8f9abc 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js @@ -12,14 +12,13 @@ Sentry.init({ ], defaultIntegrations: false, tracesSampleRate: 1, - debug: true + debug: true, }); const activeSpan = Sentry.getActiveSpan(); -if (activeSpan) { - Sentry.startInactiveSpan({ name: 'pageload-child-span' }); -} else { - setTimeout(() => { - Sentry.startInactiveSpan({ name: 'pageload-child-span' }); - }, 200); -} +Sentry.startInactiveSpan({ + name: 'pageload-child-span', + onlyIfParent: true, + // Set this to ensure we do not discard this span due to timeout + startTime: activeSpan && Sentry.spanToJSON(activeSpan).start_timestamp, +}); From 6c12cd8ed482e20ecf35daafc8f703a66823ce5e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Jun 2024 15:17:25 +0200 Subject: [PATCH 12/12] cleanup --- .../pageloadWithChildSpanTimeout/init.js | 1 - .../pageloadWithChildSpanTimeout/test.ts | 2 -- 2 files changed, 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js index be0f1e8f9abc..229372e215c7 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js @@ -12,7 +12,6 @@ Sentry.init({ ], defaultIntegrations: false, tracesSampleRate: 1, - debug: true, }); const activeSpan = Sentry.getActiveSpan(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts index a08bf90d09f3..ca342a5533c2 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts @@ -15,8 +15,6 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn sentryTest.skip(); } - page.on('console', msg => console.log(msg.text())); - const url = await getLocalTestUrl({ testDir: __dirname }); const req = await waitForTransactionRequestOnUrl(page, url);