-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add option to ignore mark
and measure
spans
#16443
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
|
||
Sentry.init({ | ||
dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
integrations: [ | ||
Sentry.browserTracingIntegration({ | ||
ignoreMeasureSpans: ['measure-ignore', /mark-i/], | ||
idleTimeout: 9000, | ||
}), | ||
], | ||
tracesSampleRate: 1, | ||
}); | ||
|
||
performance.mark('mark-pass'); | ||
performance.mark('mark-ignore'); | ||
performance.measure('measure-pass'); | ||
performance.measure('measure-ignore'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import type { Route } from '@playwright/test'; | ||
import { expect } from '@playwright/test'; | ||
import { sentryTest } from '../../../../utils/fixtures'; | ||
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers'; | ||
|
||
sentryTest( | ||
'should ignore mark and measure spans that match `ignoreMeasureSpans`', | ||
async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
await page.route('**/path/to/script.js', (route: Route) => | ||
route.fulfill({ path: `${__dirname}/assets/script.js` }), | ||
); | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
const transactionRequestPromise = waitForTransactionRequest( | ||
page, | ||
evt => evt.type === 'transaction' && evt.contexts?.trace?.op === 'pageload', | ||
); | ||
|
||
await page.goto(url); | ||
|
||
const transactionEvent = envelopeRequestParser(await transactionRequestPromise); | ||
const markAndMeasureSpans = transactionEvent.spans?.filter(({ op }) => op && ['mark', 'measure'].includes(op)); | ||
|
||
expect(markAndMeasureSpans?.length).toBe(3); | ||
expect(markAndMeasureSpans).toEqual( | ||
expect.arrayContaining([ | ||
expect.objectContaining({ | ||
description: 'mark-pass', | ||
op: 'mark', | ||
}), | ||
expect.objectContaining({ | ||
description: 'measure-pass', | ||
op: 'measure', | ||
}), | ||
expect.objectContaining({ | ||
description: 'sentry-tracing-init', | ||
op: 'mark', | ||
}), | ||
]), | ||
); | ||
}, | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,7 +149,22 @@ export interface BrowserTracingOptions { | |
* | ||
* Default: [] | ||
*/ | ||
ignoreResourceSpans: Array<string>; | ||
ignoreResourceSpans: Array<'resouce.script' | 'resource.css' | 'resource.img' | 'resource.other' | string>; | ||
|
||
/** | ||
* Spans created from | ||
* [`performance.mark(...)`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/mark) | ||
* and | ||
* [`performance.measure(...)`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure) | ||
* calls will not be emitted if their names match strings in this array. | ||
* | ||
* This is useful, if you come across `mark` or `measure` spans in your Sentry traces | ||
* that you want to ignore. For example, sometimes, browser extensions or libraries | ||
* emit these entries on their own, which might not be relevant to your application. | ||
* | ||
* Default: [] - By default, all `mark` and `measure` entries are sent as spans. | ||
*/ | ||
ignoreMeasureSpans: Array<string | RegExp>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this not only applies to the No hard opinion on that though as my proposed name might be too wide in its description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had exactly the same feeling. Let's make a decision fast, as I agree the name doesn't really include mark spans. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this is how we could extend this if people need granular options: ignorePerformanceAPISpans: {
mark:
measure:
...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed the option to |
||
|
||
/** | ||
* Link the currently started trace to a previous trace (e.g. a prior pageload, navigation or | ||
|
@@ -234,6 +249,7 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { | |
enableLongAnimationFrame: true, | ||
enableInp: true, | ||
ignoreResourceSpans: [], | ||
ignoreMeasureSpans: [], | ||
linkPreviousTrace: 'in-memory', | ||
consistentTraceSampling: false, | ||
_experiments: {}, | ||
|
@@ -277,6 +293,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |
shouldCreateSpanForRequest, | ||
enableHTTPTimings, | ||
ignoreResourceSpans, | ||
ignoreMeasureSpans, | ||
instrumentPageLoad, | ||
instrumentNavigation, | ||
linkPreviousTrace, | ||
|
@@ -319,7 +336,11 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |
// This will generally always be defined here, because it is set in `setup()` of the integration | ||
// but technically, it is optional, so we guard here to be extra safe | ||
_collectWebVitals?.(); | ||
addPerformanceEntries(span, { recordClsOnPageloadSpan: !enableStandaloneClsSpans, ignoreResourceSpans }); | ||
addPerformanceEntries(span, { | ||
recordClsOnPageloadSpan: !enableStandaloneClsSpans, | ||
ignoreResourceSpans, | ||
ignoreMeasureSpans, | ||
}); | ||
setActiveIdleSpan(client, undefined); | ||
|
||
// A trace should stay consistent over the entire timespan of one route - even after the pageload/navigation ended. | ||
|
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.
Nice type addition 👍
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.
yeah, realized we only made this addition in
browsermetrics.ts
rather than the user-facing option. Luckily not breaking :D