Skip to content

ref(tracing): Only add user_id to DSC if sendDefaultPii is true #5344

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 4 commits into from
Jul 4, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
add and adjust unit and integration tests
revert unintended yarn script deletion
  • Loading branch information
Lms24 committed Jul 1, 2022
commit 841e236361707dda0e4d2bf2c2252484e549cb3a
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import * as Sentry from '@sentry/browser';
import { Integrations } from '@sentry/tracing';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
environment: 'production',
tracesSampleRate: 1,
debug: true,
});

Sentry.configureScope(scope => {
scope.setUser({ id: 'user123', segment: 'segmentB' });
scope.setTransactionName('testTransactionDSC');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { expect } from '@playwright/test';
import { EventEnvelopeHeaders } from '@sentry/types';

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',
async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

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

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),
public_key: 'public',
});
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Sentry.init({
integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
environment: 'production',
tracesSampleRate: 1,
sendDefaultPii: true,
debug: true,
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import * as Sentry from '@sentry/node';
import * as Tracing from '@sentry/tracing';
import cors from 'cors';
import express from 'express';
import http from 'http';

const app = express();

export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
environment: 'prod',
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
tracesSampleRate: 1.0,
});

Sentry.setUser({ id: 'user123', segment: 'SegmentA' });

app.use(Sentry.Handlers.requestHandler());
app.use(Sentry.Handlers.tracingHandler());

app.use(cors());

app.get('/test/express', (_req, res) => {
const transaction = Sentry.getCurrentHub().getScope()?.getTransaction();
if (transaction) {
transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd';
}
const headers = http.get('http://somewhere.not.sentry/').getHeaders();

// Responding with the headers outgoing request headers back to the assertions.
res.send({ test_data: headers });
});

app.use(Sentry.Handlers.errorHandler());

export default app;
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,31 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
import { TestAPIResponse } from '../server';

test('should attach a `baggage` header to an outgoing request.', async () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);

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

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringMatching('sentry-environment=prod,sentry-release=1.0'),
baggage:
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,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 () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);

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

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.not.stringContaining('sentry-user_id'),
},
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import * as Sentry from '@sentry/node';
import * as Tracing from '@sentry/tracing';
import cors from 'cors';
import express from 'express';
import http from 'http';

const app = express();

export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
environment: 'prod',
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
tracesSampleRate: 1.0,
sendDefaultPii: true,
});

Sentry.setUser({ id: 'user123', segment: 'SegmentA' });

app.use(Sentry.Handlers.requestHandler());
app.use(Sentry.Handlers.tracingHandler());

app.use(cors());

app.get('/test/express', (_req, res) => {
const transaction = Sentry.getCurrentHub().getScope()?.getTransaction();
if (transaction) {
transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd';
}
const headers = http.get('http://somewhere.not.sentry/').getHeaders();

// Responding with the headers outgoing request headers back to the assertions.
res.send({ test_data: headers });
});

app.use(Sentry.Handlers.errorHandler());

export default app;
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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 () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);

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

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining('sentry-user_id=user123'),
},
});
});
20 changes: 19 additions & 1 deletion packages/node/test/integrations/http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ import { getDefaultNodeClientOptions } from '../helper/node-client-options';
const NODE_VERSION = parseSemver(process.versions.node);

describe('tracing', () => {
function createTransactionOnScope() {
function createTransactionOnScope(sendDefaultPii: boolean = true) {
const options = getDefaultNodeClientOptions({
dsn: 'https://[email protected]/12312012',
tracesSampleRate: 1.0,
integrations: [new HttpIntegration({ tracing: true })],
release: '1.0.0',
environment: 'production',
sendDefaultPii,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) We could refactor this a bit so that createTransactionOnScope takes node client options as an argument which override a default (default being what's there now).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea; that seems like a more sustainable solution

Copy link
Member Author

Choose a reason for hiding this comment

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

const hub = new Hub(new NodeClient(options));
addExtensionMethods();
Expand Down Expand Up @@ -133,6 +134,23 @@ describe('tracing', () => {
);
});

it('does not add the user_id to the baggage header if sendDefaultPii is set to false', async () => {
nock('http://dogs.are.great').get('/').reply(200);

createTransactionOnScope(false);
Copy link
Contributor

@lforst lforst Jul 1, 2022

Choose a reason for hiding this comment

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

Just seeing this in tests is so glorius. Sentry.startTransaction() should really just instantly put a transaction on the scope... I hope we get to that some day.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I 100% agree


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,' +
'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
);
});

it("doesn't attach the sentry-trace header to outgoing sentry requests", () => {
nock('http://squirrelchasers.ingest.sentry.io').get('/api/12312012/store/').reply(200);

Expand Down
5 changes: 3 additions & 2 deletions packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,12 @@ describe('Span', () => {
hub,
);

const hubSpy = jest.spyOn(hub.getClient()!, 'getOptions');
const getOptionsSpy = jest.spyOn(hub.getClient()!, 'getOptions');

const baggage = transaction.getBaggage();

expect(hubSpy).toHaveBeenCalledTimes(1);
// this is called twice because hub.shouldSendDefaultPii also calls getOptions()
expect(getOptionsSpy).toHaveBeenCalledTimes(2);
expect(baggage && isSentryBaggageEmpty(baggage)).toBe(false);
expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
release: '1.0.1',
Expand Down