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: Normalize Transaction and Span consistently #2655

Merged
merged 2 commits into from Jun 8, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/core/src/baseclient.ts
Expand Up @@ -300,7 +300,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
}

// tslint:disable:no-unsafe-any
return {
const normalized = {
...event,
...(event.breadcrumbs && {
breadcrumbs: event.breadcrumbs.map(b => ({
Expand All @@ -320,6 +320,17 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
extra: normalize(event.extra, depth),
}),
};
// 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
// Transactions and Spans consistently.
// For now the decision is to skip normalization of Transactions and Spans,
// so this block overwrites the normalized event to add back the original
// Transaction information prior to normalization.
if (event.contexts && event.contexts.trace) {
normalized.contexts.trace = event.contexts.trace;
}
return normalized;
}

/**
Expand Down
50 changes: 49 additions & 1 deletion packages/core/test/lib/base.test.ts
@@ -1,5 +1,5 @@
import { Hub, Scope } from '@sentry/hub';
import { Event, Severity } from '@sentry/types';
import { Event, Severity, Span } from '@sentry/types';
import { SentryError } from '@sentry/utils';

import { TestBackend } from '../mocks/backend';
Expand Down Expand Up @@ -570,6 +570,54 @@ describe('BaseClient', () => {
});
});

test('normalization applies to Transaction and Span consistently', () => {
expect.assertions(1);
const client = new TestClient({ dsn: PUBLIC_DSN });
const transaction: Event = {
contexts: {
trace: {
data: { _sentry_web_vitals: { LCP: { value: 99.9 } } },
op: 'pageload',
span_id: 'a3df84a60c2e4e76',
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
},
},
event_id: '972f45b826a248bba98e990878a177e1',
spans: [
({
data: { _sentry_extra_metrics: { M1: { value: 1 }, M2: { value: 2 } } },
description: 'first-paint',
endTimestamp: 1591603196.637835,
op: 'paint',
parentSpanId: 'a3df84a60c2e4e76',
spanId: '9e15bf99fbe4bc80',
startTimestamp: 1591603196.637835,
traceId: '86f39e84263a4de99c326acab3bfe3bd',
} as any) as Span, // `as any` to bypass linter https://palantir.github.io/tslint/rules/no-object-literal-type-assertion/
({
description: 'first-contentful-paint',
endTimestamp: 1591603196.637835,
op: 'paint',
parentSpanId: 'a3df84a60c2e4e76',
spanId: 'aa554c1f506b0783',
startTimestamp: 1591603196.637835,
traceId: '86f39e84263a4de99c326acab3bfe3bd',
} as any) as Span,
],
start_timestamp: 1591603196.614865,
timestamp: 1591603196.728485,
transaction: '/',
type: 'transaction',
};
// To be consistent, normalization could apply either to both transactions
// and spans, or to none. So far the decision is to skip normalization for
// both, such that the expected normalizedTransaction is the same as the
// input transaction.
const normalizedTransaction = JSON.parse(JSON.stringify(transaction)); // deep-copy
client.captureEvent(transaction);
expect(TestBackend.instance!.event!).toEqual(normalizedTransaction);
});

test('calls beforeSend and uses original event without any changes', () => {
expect.assertions(1);
const beforeSend = jest.fn(event => event);
Expand Down