-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add option to explicitly end pageload span via reportPageLoaded()
#17697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5e5b6bc
feat(browser): Explicitly end pageload span via `Sentry.reportPageLoa…
Lms24 9ac071e
add tests, adjust end time trimming
Lms24 64ef836
jsdoc and fix cleanup of _pageloadSpan
Lms24 c1762f1
lint
Lms24 145086e
rename option, streamlie reportPageLoaded
Lms24 bd61a09
extract to its own file
Lms24 f69665e
fix exports
Lms24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
15 changes: 15 additions & 0 deletions
15
...tegration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/init.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window._testBaseTimestamp = performance.timeOrigin / 1000; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
integrations: [Sentry.browserTracingIntegration({ enableReportPageLoaded: true })], | ||
tracesSampleRate: 1, | ||
debug: true, | ||
}); | ||
|
||
setTimeout(() => { | ||
Sentry.reportPageLoaded(); | ||
}, 2500); |
42 changes: 42 additions & 0 deletions
42
...tegration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { expect } from '@playwright/test'; | ||
import { | ||
SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, | ||
} from '@sentry/browser'; | ||
import { sentryTest } from '../../../../../utils/fixtures'; | ||
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers'; | ||
|
||
sentryTest( | ||
'waits for Sentry.reportPageLoaded() to be called when `enableReportPageLoaded` is true', | ||
async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload'); | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
await page.goto(url); | ||
|
||
const eventData = envelopeRequestParser(await pageloadEventPromise); | ||
|
||
const traceContextData = eventData.contexts?.trace?.data; | ||
const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!; | ||
|
||
expect(traceContextData).toMatchObject({ | ||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', | ||
['sentry.idle_span_finish_reason']: 'reportPageLoaded', | ||
}); | ||
|
||
// We wait for 2.5 seconds before calling Sentry.reportPageLoaded() | ||
// the margins are to account for timing weirdness in CI to avoid flakes | ||
expect(spanDurationSeconds).toBeGreaterThan(2); | ||
expect(spanDurationSeconds).toBeLessThan(3); | ||
}, | ||
); |
13 changes: 13 additions & 0 deletions
13
...tion-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/init.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window._testBaseTimestamp = performance.timeOrigin / 1000; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
integrations: [Sentry.browserTracingIntegration({ enableReportPageLoaded: true, finalTimeout: 3000 })], | ||
tracesSampleRate: 1, | ||
debug: true, | ||
}); | ||
|
||
// not calling Sentry.reportPageLoaded() on purpose! |
42 changes: 42 additions & 0 deletions
42
...tion-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { expect } from '@playwright/test'; | ||
import { | ||
SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, | ||
} from '@sentry/browser'; | ||
import { sentryTest } from '../../../../../utils/fixtures'; | ||
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers'; | ||
|
||
sentryTest( | ||
'final timeout cancels the pageload span even if `enableReportPageLoaded` is true', | ||
async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload'); | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
await page.goto(url); | ||
|
||
const eventData = envelopeRequestParser(await pageloadEventPromise); | ||
|
||
const traceContextData = eventData.contexts?.trace?.data; | ||
const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!; | ||
|
||
expect(traceContextData).toMatchObject({ | ||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', | ||
['sentry.idle_span_finish_reason']: 'finalTimeout', | ||
}); | ||
|
||
// We wait for 3 seconds before calling Sentry.reportPageLoaded() | ||
// the margins are to account for timing weirdness in CI to avoid flakes | ||
expect(spanDurationSeconds).toBeGreaterThan(2.5); | ||
expect(spanDurationSeconds).toBeLessThan(3.5); | ||
}, | ||
); |
19 changes: 19 additions & 0 deletions
19
...ration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/init.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window._testBaseTimestamp = performance.timeOrigin / 1000; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
integrations: [Sentry.browserTracingIntegration({ enableReportPageLoaded: true, instrumentNavigation: false })], | ||
tracesSampleRate: 1, | ||
debug: true, | ||
}); | ||
|
||
setTimeout(() => { | ||
Sentry.startBrowserTracingNavigationSpan(Sentry.getClient(), { name: 'custom_navigation' }); | ||
}, 1000); | ||
|
||
setTimeout(() => { | ||
Sentry.reportPageLoaded(); | ||
}, 2500); |
40 changes: 40 additions & 0 deletions
40
...ration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import { expect } from '@playwright/test'; | ||
import { | ||
SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, | ||
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, | ||
} from '@sentry/browser'; | ||
import { sentryTest } from '../../../../../utils/fixtures'; | ||
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers'; | ||
|
||
sentryTest( | ||
'starting a navigation span cancels the pageload span even if `enableReportPageLoaded` is true', | ||
async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload'); | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
await page.goto(url); | ||
|
||
const eventData = envelopeRequestParser(await pageloadEventPromise); | ||
|
||
const traceContextData = eventData.contexts?.trace?.data; | ||
const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!; | ||
|
||
expect(traceContextData).toMatchObject({ | ||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', | ||
['sentry.idle_span_finish_reason']: 'cancelled', | ||
}); | ||
|
||
// ending span after 1s but adding a margin of 0.5s to account for timing weirdness in CI to avoid flakes | ||
expect(spanDurationSeconds).toBeLessThan(1.5); | ||
}, | ||
); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go with the client hooks approach instead of exposing a method on
browserTracingIntegration
because in contrast to all other integrations, we don't hide the specific implementation ofbrowserTracingIntegration
behinddefineIntegration
. So adding a method would indeed be public API (we also can't introducedefineIntegration
now as that would be a breaking change).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this is a nice enough API for this anyhow, I like it!