-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(core): Set sentry.source
attribute to custom
when calling span.updateName
on SentrySpan
#14251
Changes from all commits
eb55f17
cf17780
0f12ced
2232458
ab779bb
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,10 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window._testBaseTimestamp = performance.timeOrigin / 1000; | ||
|
||
Sentry.init({ | ||
dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
integrations: [Sentry.browserTracingIntegration()], | ||
tracesSampleRate: 1, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const activeSpan = Sentry.getActiveSpan(); | ||
const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan); | ||
rootSpan?.updateName('new name'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { expect } from '@playwright/test'; | ||
import type { Event } from '@sentry/types'; | ||
|
||
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 { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; | ||
|
||
sentryTest('sets the source to custom when updating the transaction name', async ({ getLocalTestPath, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const url = await getLocalTestPath({ testDir: __dirname }); | ||
|
||
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url); | ||
|
||
const traceContextData = eventData.contexts?.trace?.data; | ||
|
||
expect(traceContextData).toMatchObject({ | ||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, | ||
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', | ||
}); | ||
|
||
expect(eventData.transaction).toBe('new name'); | ||
|
||
expect(eventData.contexts?.trace?.op).toBe('pageload'); | ||
expect(eventData.spans?.length).toBeGreaterThan(0); | ||
expect(eventData.transaction_info?.source).toEqual('custom'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,16 @@ | ||
import { expect } from '@playwright/test'; | ||
import type { Event } from '@sentry/types'; | ||
|
||
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 { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; | ||
|
||
sentryTest('should create a pageload transaction', async ({ getLocalTestPath, page }) => { | ||
sentryTest('creates a pageload transaction with url as source', async ({ getLocalTestPath, page }) => { | ||
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. no behaviour change here, just to explicitly check that our default browserTracingIntegration starts spans with source "url" |
||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
@@ -16,8 +22,17 @@ sentryTest('should create a pageload transaction', async ({ getLocalTestPath, pa | |
|
||
const { start_timestamp: startTimestamp } = eventData; | ||
|
||
const traceContextData = eventData.contexts?.trace?.data; | ||
|
||
expect(startTimestamp).toBeCloseTo(timeOrigin, 1); | ||
|
||
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', | ||
}); | ||
|
||
expect(eventData.contexts?.trace?.op).toBe('pageload'); | ||
expect(eventData.spans?.length).toBeGreaterThan(0); | ||
expect(eventData.transaction_info?.source).toEqual('url'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,6 +193,7 @@ export class SentrySpan implements Span { | |
*/ | ||
public updateName(name: string): this { | ||
this._name = name; | ||
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom'); | ||
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. l: Wondering if we would want to mention this in the jsdoc? 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. Good point, let me add a note 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. On second thought, I'll add this but only in #14280 (which is the PR to fix this and a couple of depending things in Node). Since we always pass the |
||
return this; | ||
} | ||
|
||
|
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.
adjusted this test to check that we by default start a span with source "custom"