Skip to content

fix(core): Normalize trace context #5171

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 8 commits into from
Jun 3, 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
21 changes: 19 additions & 2 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
return null;
}

const normalized = {
const normalized: Event = {
...event,
...(event.breadcrumbs && {
breadcrumbs: event.breadcrumbs.map(b => ({
Expand All @@ -496,6 +496,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
extra: normalize(event.extra, depth, maxBreadth),
}),
};

// event.contexts.trace stores information about a Transaction. Similarly,
// event.spans[] stores information about child Spans. Given that a
// Transaction is conceptually a Span, normalization should apply to both
Expand All @@ -504,8 +505,24 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
// so this block overwrites the normalized event to add back the original
// Transaction information prior to normalization.
if (event.contexts && event.contexts.trace) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
normalized.contexts = {};
normalized.contexts.trace = event.contexts.trace;

// event.contexts.trace.data may contain circular/dangerous data so we need to normalize it
if (event.contexts.trace.data) {
normalized.contexts.trace.data = normalize(event.contexts.trace.data, depth, maxBreadth);
}
}

// event.spans[].data may contain circular/dangerous data so we need to normalize it
if (event.spans) {
Copy link
Member

Choose a reason for hiding this comment

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

should we gate this with a tracing debug flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't do that. The tracing flag currently only guards code that sets up automatic instrumentation, i.e. side effect code that makes use of tracing. This allows tracing to be treeshaken if it's not in use. It can still be used, even if the flag is set to false. If we guard this here and users call startTransaction and pass in circular data, they're not gonna have a good time. Good consideration though.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

normalized.spans = event.spans.map(span => {
// We cannot use the spread operator here because `toJSON` on `span` is non-enumerable
if (span.data) {
span.data = normalize(span.data, depth, maxBreadth);
}
return span;
});
}

normalized.sdkProcessingMetadata = { ...normalized.sdkProcessingMetadata, baseClientNormalized: true };
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const chicken = {};
const egg = { contains: chicken };
chicken.lays = egg;

const circularObject = chicken;

const transaction = Sentry.startTransaction({ name: 'circular_object_test_transaction', data: circularObject });
const span = transaction.startChild({ op: 'circular_object_test_span', data: circularObject });

span.finish();
transaction.finish();
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { expect } from '@playwright/test';
import { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should be able to handle circular data', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.type).toBe('transaction');
expect(eventData.transaction).toBe('circular_object_test_transaction');

expect(eventData.contexts).toMatchObject({
trace: {
data: { lays: { contains: '[Circular ~]' } },
},
});

expect(eventData?.spans?.[0]).toMatchObject({
data: { lays: { contains: '[Circular ~]' } },
op: 'circular_object_test_span',
});

await new Promise(resolve => setTimeout(resolve, 2000));
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ window.Sentry = Sentry;
Sentry.init({
dsn: 'https://[email protected]/1337',
tracesSampleRate: 1.0,
normalizeDepth: 10,
});