Skip to content

ref(tracing): Remove transaction name and user_id from DSC #5363

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 7 commits into from
Jul 5, 2022
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
8 changes: 4 additions & 4 deletions packages/core/test/lib/envelope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ describe('createEventEnvelope', () => {
{
environment: 'prod',
release: '1.0.0',
transaction: 'TX',
user_id: 'bob',
// transaction: 'TX',
// user_id: 'bob',
user_segment: 'segmentA',
sample_rate: '0.95',
public_key: 'pubKey123',
Expand All @@ -59,8 +59,8 @@ describe('createEventEnvelope', () => {
{
environment: 'prod',
release: '1.0.0',
transaction: 'TX',
user_id: 'bob',
// transaction: 'TX',
// user_id: 'bob',
user_segment: 'segmentA',
sample_rate: '0.95',
public_key: 'pubKey123',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { sentryTest } from '../../../utils/fixtures';
import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';

sentryTest(
'should not send user_id in DSC data in trace envelope header if sendDefaultPii option is not set',
'should not send user_id and transaction in DSC data in trace envelope header (for now)',
async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

Expand All @@ -14,7 +14,6 @@ sentryTest(
expect(envHeader.trace).toBeDefined();
expect(envHeader.trace).toEqual({
environment: 'production',
transaction: expect.stringContaining('index.html'),
user_segment: 'segmentB',
sample_rate: '1',
trace_id: expect.any(String),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ Sentry.init({
integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
environment: 'production',
tracesSampleRate: 1,
sendDefaultPii: true,
// TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
// sendDefaultPii: true,
debug: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ sentryTest(
expect(envHeader.trace).toBeDefined();
expect(envHeader.trace).toEqual({
environment: 'production',
transaction: expect.stringContaining('index.html'),
user_id: 'user123',
// transaction: expect.stringContaining('index.html'),
// user_id: 'user123',
user_segment: 'segmentB',
sample_rate: '1',
trace_id: expect.any(String),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,10 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining(
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
'sentry-public_key=public,sentry-trace_id=',
// Commented out as long as transaction and user_id are not part of DSC
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
// 'sentry-public_key=public,sentry-trace_id=',
'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
Copy link
Member

Choose a reason for hiding this comment

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

Tiny tiny nit: If we're commenting stuff out in the other tests rather than modifying said stuff, I'd leave the old value here, commented out (in addition to the new value, of course).

(Same in the other spots where this applies.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 5249b77

),
},
});
Expand All @@ -99,8 +101,10 @@ test('Should populate Sentry and ignore 3rd party content if sentry-trace header
host: 'somewhere.not.sentry',
// TraceId changes, hence we only expect that the string contains the traceid key
baggage: expect.stringContaining(
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
'sentry-public_key=public,sentry-trace_id=',
// Commented out as long as transaction and user_id are not part of DSC
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
// 'sentry-public_key=public,sentry-trace_id=',
'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
),
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,26 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
test_data: {
host: 'somewhere.not.sentry',
baggage:
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
// Commented out as long as transaction and user_id are not part of DSC
// 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
'sentry-environment=prod,sentry-release=1.0,sentry-user_segment=SegmentA' +
',sentry-public_key=public,sentry-trace_id=86f39e84263a4de99c326acab3bfe3bd,sentry-sample_rate=1',
},
});
});

test('Does not include user_id in baggage if sendDefaultPii is not set', async () => {
test('Does not include user_id and transaction name (for now)', async () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);

const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
const baggageString = response.test_data.baggage;

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.not.stringContaining('sentry-user_id'),
},
});
expect(baggageString).not.toContain('sentry-user_id=');
expect(baggageString).not.toContain('sentry-transaction=');
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining(
'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' +
'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
// Commented out as long as transaction and user_id are not part of DSC
// 'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' +
// 'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-public_key=public',
),
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ Sentry.init({
environment: 'prod',
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
tracesSampleRate: 1.0,
sendDefaultPii: true,
// TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
// sendDefaultPii: true,
});

Sentry.setUser({ id: 'user123', segment: 'SegmentA' });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import * as path from 'path';
import { getAPIResponse, runServer } from '../../../../utils/index';
import { TestAPIResponse } from '../server';

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

const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
Expand Down
22 changes: 15 additions & 7 deletions packages/node/test/integrations/http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ describe('tracing', () => {
integrations: [new HttpIntegration({ tracing: true })],
release: '1.0.0',
environment: 'production',
sendDefaultPii: true,
...customOptions,
});
const hub = new Hub(new NodeClient(options));
Expand Down Expand Up @@ -113,7 +112,9 @@ describe('tracing', () => {
expect(baggageHeader).toBeDefined();
expect(typeof baggageHeader).toEqual('string');
expect(baggageHeader).toEqual(
'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_id=uid123,' +
// Commented out as long as transaction and user_id are not part of DSC
// 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_id=uid123,' +
'sentry-environment=production,sentry-release=1.0.0,' +
'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
);
Expand All @@ -130,24 +131,31 @@ describe('tracing', () => {
expect(baggageHeader).toBeDefined();
expect(typeof baggageHeader).toEqual('string');
expect(baggageHeader).toEqual(
'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
'sentry-user_id=uid123,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
// Commented out as long as transaction and user_id are not part of DSC
// 'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
// 'sentry-user_id=uid123,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
// 'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
'dog=great,sentry-environment=production,sentry-release=1.0.0,' +
'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
);
});

it('does not add the user_id to the baggage header if sendDefaultPii is set to false', async () => {
// TODO: Skipping this test because right now we're rethinking the mechanism for including such data
it.skip('does not add the user_id to the baggage header if <optionTBA> is set to false', async () => {
nock('http://dogs.are.great').get('/').reply(200);

createTransactionOnScope({ sendDefaultPii: false });
createTransactionOnScope();

const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } });
const baggageHeader = request.getHeader('baggage') as string;

expect(baggageHeader).toBeDefined();
expect(typeof baggageHeader).toEqual('string');
expect(baggageHeader).toEqual(
'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
// Commented out as long as transaction and user_id are not part of DSC
// 'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
'dog=great,sentry-environment=production,sentry-release=1.0.0,' +
'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
);
Expand Down
10 changes: 7 additions & 3 deletions packages/tracing/src/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,19 @@ export class Transaction extends SpanClass implements TransactionInterface {
? rate.toLocaleString('fullwide', { useGrouping: false, maximumFractionDigits: 16 })
: undefined;

// For now we're not sending the transaction name and user_id due to PII concerns
// commenting it out for now because we'll probably need it in the future

const scope = hub.getScope();
const { id: user_id, segment: user_segment } = (scope && scope.getUser()) || {};
const { /* id: user_id, */ segment: user_segment } = (scope && scope.getUser()) || {};

return createBaggage(
dropUndefinedKeys({
environment,
release,
transaction: this.name,
...(hub.shouldSendDefaultPii() && { user_id }),
// transaction: this.name,
// replace `someContidion` with whatever decision we come up with to guard PII in DSC
// ...(someCondition && { user_id }),
user_segment,
public_key,
trace_id: this.traceId,
Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/test/browser/browsertracing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ describe('BrowserTracing', () => {
expect(baggage[0]).toEqual({
release: '1.0.0',
environment: 'production',
transaction: 'blank',
// transaction: 'blank',
public_key: 'pubKey',
trace_id: expect.not.stringMatching('12312012123120121231201212312012'),
});
Expand Down
7 changes: 3 additions & 4 deletions packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,13 +438,12 @@ describe('Span', () => {

const baggage = transaction.getBaggage();

// this is called twice because hub.shouldSendDefaultPii also calls getOptions()
expect(getOptionsSpy).toHaveBeenCalledTimes(2);
expect(getOptionsSpy).toHaveBeenCalledTimes(1);
expect(baggage && isSentryBaggageEmpty(baggage)).toBe(false);
expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
release: '1.0.1',
environment: 'production',
transaction: 'tx',
// transaction: 'tx',
sample_rate: '0.56',
trace_id: expect.any(String),
});
Expand All @@ -468,7 +467,7 @@ describe('Span', () => {
expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
release: '1.0.1',
environment: 'production',
transaction: 'tx',
// transaction: 'tx',
sample_rate: '0.0000000000000145',
trace_id: expect.any(String),
});
Expand Down
4 changes: 2 additions & 2 deletions packages/types/src/envelope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export type DynamicSamplingContext = {
sample_rate?: string;
release?: string;
environment?: string;
transaction?: string;
user_id?: string;
// transaction?: string; // omitted for now
// user_id?: string; // omitted for now
user_segment?: string;
};

Expand Down
13 changes: 6 additions & 7 deletions packages/types/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,17 +153,16 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
tunnel?: string;

/**
* Currently controls if the user id (e.g. set by `Sentry.setUser`) should
* be used for dynamic sampling. Only if this flag is set to `true`, the user id
* will be propagated to outgoing requests in the `baggage` Http header.
* Important: This option is currently unused and will only work in the next major version of the SDK
*
* Note that in the next major version of this SDK, this option will not only
* control dynamic sampling data: As long as this flag is not set to `true`,
* the SDK will not send sensitive data by default.
* Controls if potentially sensitive data should be sent to Sentry by default.
* Note that this only applies to data that the SDK is sending by default
* but not data that was explicitly set (e.g. by calling `Sentry.setUser()`).
* Details about the implementation are TBD.
*
* Defaults to `false`.
*
* @experimental (will be fully introduced in the next major version)
* @ignore
*/
sendDefaultPii?: boolean;

Expand Down