Skip to content

Commit e97a639

Browse files
authored
ref(tracing): Include transaction in DSC if transaction source is not an unparameterized URL (#5392)
This patch re-introduces the `transaction` field in the Dynamic Sampling Context (DSC). However, its presence is now determined by the [transaction source](https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations) which was introduced in #5367. As of this we we add the `transaction` field back, if the source indicates that the transaction name is not an unparameterized URL (meaning, the source is set and it is not `url`). Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>`sendDefaultPii` tests that we previously skipped to now cover the transaction<=>transaction source dependence. Did some cleanup of commented out old code and explanations that no longer apply. Remove he `'unknown'` field from the `TransactionSource` type because it is only used by Relay and SDKs shouldn't set it.
1 parent 439b9d3 commit e97a639

File tree

16 files changed

+71
-51
lines changed

16 files changed

+71
-51
lines changed

packages/core/test/lib/envelope.test.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,7 @@ describe('createEventEnvelope', () => {
4444
{
4545
environment: 'prod',
4646
release: '1.0.0',
47-
// transaction: 'TX',
48-
// user_id: 'bob',
47+
transaction: 'TX',
4948
user_segment: 'segmentA',
5049
sample_rate: '0.95',
5150
public_key: 'pubKey123',
@@ -59,8 +58,7 @@ describe('createEventEnvelope', () => {
5958
{
6059
environment: 'prod',
6160
release: '1.0.0',
62-
// transaction: 'TX',
63-
// user_id: 'bob',
61+
transaction: 'TX',
6462
user_segment: 'segmentA',
6563
sample_rate: '0.95',
6664
public_key: 'pubKey123',

packages/integration-tests/suites/tracing/envelope-header-no-pii/init.js renamed to packages/integration-tests/suites/tracing/envelope-header-transaction-name/init.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,5 @@ Sentry.init({
1414
Sentry.configureScope(scope => {
1515
scope.setUser({ id: 'user123', segment: 'segmentB' });
1616
scope.setTransactionName('testTransactionDSC');
17+
scope.getTransaction().setMetadata({ source: 'custom' });
1718
});

packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts renamed to packages/integration-tests/suites/tracing/envelope-header-transaction-name/test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { sentryTest } from '../../../utils/fixtures';
55
import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
66

77
sentryTest(
8-
'should not send user_id and transaction in DSC data in trace envelope header (for now)',
8+
'should only include transaction name if source is better than an unparameterized URL',
99
async ({ getLocalTestPath, page }) => {
1010
const url = await getLocalTestPath({ testDir: __dirname });
1111

@@ -16,6 +16,7 @@ sentryTest(
1616
environment: 'production',
1717
user_segment: 'segmentB',
1818
sample_rate: '1',
19+
transaction: expect.stringContaining('/index.html'),
1920
trace_id: expect.any(String),
2021
public_key: 'public',
2122
});

packages/integration-tests/suites/tracing/envelope-header/init.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ Sentry.init({
88
integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
99
environment: 'production',
1010
tracesSampleRate: 1,
11-
// TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
12-
// sendDefaultPii: true,
1311
debug: true,
1412
});
1513

packages/integration-tests/suites/tracing/envelope-header/test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,13 @@ sentryTest(
1111

1212
const envHeader = await getFirstSentryEnvelopeRequest<EventEnvelopeHeaders>(page, url, envelopeHeaderRequestParser);
1313

14+
// In this test, we don't expect trace.transaction to be present because without a custom routing instrumentation
15+
// we for now don't have parameterization. This might change in the future but for now the only way of having
16+
// transaction in DSC with the default BrowserTracing integration is when the transaction name is set manually.
17+
// This scenario is covered in another integration test (envelope-header-transaction-name).
1418
expect(envHeader.trace).toBeDefined();
1519
expect(envHeader.trace).toEqual({
1620
environment: 'production',
17-
// transaction: expect.stringContaining('index.html'),
18-
// user_id: 'user123',
1921
user_segment: 'segmentB',
2022
sample_rate: '1',
2123
trace_id: expect.any(String),

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,8 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
7878
expect(response).toMatchObject({
7979
test_data: {
8080
host: 'somewhere.not.sentry',
81+
// TraceId changes, hence we only expect that the string contains the traceid key
8182
baggage: expect.stringContaining(
82-
// Commented out as long as transaction and user_id are not part of DSC
83-
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
84-
// 'sentry-public_key=public,sentry-trace_id=',
8583
'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
8684
),
8785
},
@@ -101,9 +99,6 @@ test('Should populate Sentry and ignore 3rd party content if sentry-trace header
10199
host: 'somewhere.not.sentry',
102100
// TraceId changes, hence we only expect that the string contains the traceid key
103101
baggage: expect.stringContaining(
104-
// Commented out as long as transaction and user_id are not part of DSC
105-
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
106-
// 'sentry-public_key=public,sentry-trace_id=',
107102
'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
108103
),
109104
},

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,13 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
1313
test_data: {
1414
host: 'somewhere.not.sentry',
1515
baggage:
16-
// Commented out as long as transaction and user_id are not part of DSC
17-
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
1816
'sentry-environment=prod,sentry-release=1.0,sentry-user_segment=SegmentA' +
1917
',sentry-public_key=public,sentry-trace_id=86f39e84263a4de99c326acab3bfe3bd,sentry-sample_rate=1',
2018
},
2119
});
2220
});
2321

24-
test('Does not include user_id and transaction name (for now)', async () => {
22+
test('Does not include transaction name if transaction source is not set', async () => {
2523
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
2624

2725
const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;

packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,6 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
3030
test_data: {
3131
host: 'somewhere.not.sentry',
3232
baggage: expect.stringContaining(
33-
// Commented out as long as transaction and user_id are not part of DSC
34-
// 'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' +
35-
// 'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
3633
'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-public_key=public',
3734
),
3835
},

packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/server.ts renamed to packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ app.get('/test/express', (_req, res) => {
2929
const transaction = Sentry.getCurrentHub().getScope()?.getTransaction();
3030
if (transaction) {
3131
transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd';
32+
transaction.setMetadata({ source: 'route' });
3233
}
3334
const headers = http.get('http://somewhere.not.sentry/').getHeaders();
3435

packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/test.ts renamed to packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,7 @@ import * as path from 'path';
33
import { getAPIResponse, runServer } from '../../../../utils/index';
44
import { TestAPIResponse } from '../server';
55

6-
// TODO: Skipping this test because right now we're rethinking the mechanism for including such data
7-
test.skip('Includes user_id in baggage if <optionTBA> is set to true', async () => {
6+
test('Includes transaction in baggage if the transaction name is parameterized', async () => {
87
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
98

109
const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
@@ -13,7 +12,7 @@ test.skip('Includes user_id in baggage if <optionTBA> is set to true', async ()
1312
expect(response).toMatchObject({
1413
test_data: {
1514
host: 'somewhere.not.sentry',
16-
baggage: expect.stringContaining('sentry-user_id=user123'),
15+
baggage: expect.stringContaining('sentry-transaction=GET%20%2Ftest%2Fexpress'),
1716
},
1817
});
1918
});

0 commit comments

Comments
 (0)