Skip to content
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

Merged
merged 5 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ module.exports = [
path: 'packages/vue/build/esm/index.js',
import: createImport('init', 'browserTracingIntegration'),
gzip: true,
limit: '38 KB',
limit: '38.5 KB',
},
// Svelte SDK (ESM)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,41 @@
import { expect } from '@playwright/test';

import {
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/browser';
import { sentryTest } from '../../../../utils/fixtures';
import {
envelopeRequestParser,
shouldSkipTracingTest,
waitForTransactionRequestOnUrl,
} from '../../../../utils/helpers';

sentryTest('should send a transaction in an envelope', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestPath({ testDir: __dirname });
const req = await waitForTransactionRequestOnUrl(page, url);
const transaction = envelopeRequestParser(req);

expect(transaction.transaction).toBe('parent_span');
expect(transaction.spans).toBeDefined();
});
sentryTest(
Copy link
Member Author

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"

'sends a transaction in an envelope with manual origin and custom source',
async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestPath({ testDir: __dirname });
const req = await waitForTransactionRequestOnUrl(page, url);
const transaction = envelopeRequestParser(req);

const attributes = transaction.contexts?.trace?.data;
expect(attributes).toEqual({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
});

expect(transaction.transaction_info?.source).toBe('custom');

expect(transaction.transaction).toBe('parent_span');
expect(transaction.spans).toBeDefined();
},
);

sentryTest('should report finished spans as children of the root transaction', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
Expand Down
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 }) => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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();
}
Expand All @@ -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');
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ export class SentrySpan implements Span {
*/
public updateName(name: string): this {
this._name = name;
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: Wondering if we would want to mention this in the jsdoc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, let me add a note

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Span interface, I need to upadet the JSDoc of the interface which we should only do once the behaviour is adjusted everywhere.

return this;
}

Expand Down
15 changes: 14 additions & 1 deletion packages/core/test/lib/tracing/sentrySpan.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { timestampInSeconds } from '@sentry/utils';
import { setCurrentClient } from '../../../src';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, setCurrentClient } from '../../../src';
import { SentrySpan } from '../../../src/tracing/sentrySpan';
import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus';
import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, spanToJSON } from '../../../src/utils/spanUtils';
Expand All @@ -20,6 +20,19 @@ describe('SentrySpan', () => {

expect(spanToJSON(span).description).toEqual('new name');
});

it('sets the source to custom when calling updateName', () => {
const span = new SentrySpan({
name: 'original name',
attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' },
});

span.updateName('new name');

const spanJson = spanToJSON(span);
expect(spanJson.description).toEqual('new name');
expect(spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toEqual('custom');
});
});

describe('setters', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export function appRouterInstrumentNavigation(client: Client): void {
WINDOW.addEventListener('popstate', () => {
if (currentNavigationSpan && currentNavigationSpan.isRecording()) {
currentNavigationSpan.updateName(WINDOW.location.pathname);
currentNavigationSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
} else {
currentNavigationSpan = startBrowserTracingNavigationSpan(client, {
name: WINDOW.location.pathname,
Expand Down Expand Up @@ -105,9 +106,11 @@ export function appRouterInstrumentNavigation(client: Client): void {

if (routerFunctionName === 'push') {
span?.updateName(transactionNameifyRouterArgument(argArray[0]));
span?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
span?.setAttribute('navigation.type', 'router.push');
} else if (routerFunctionName === 'replace') {
span?.updateName(transactionNameifyRouterArgument(argArray[0]));
span?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
span?.setAttribute('navigation.type', 'router.replace');
} else if (routerFunctionName === 'back') {
span?.setAttribute('navigation.type', 'router.back');
Expand Down
1 change: 1 addition & 0 deletions packages/solid/src/solidrouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ function withSentryRouterRoot(Root: Component<RouteSectionProps>): Component<Rou
// everything else was already instrumented correctly in `useBeforeLeave`
if (op === 'navigation' && description === '-1') {
rootSpan.updateName(name);
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
}
}
});
Expand Down
Loading