Skip to content

Commit

Permalink
fix: Normalize Transaction and Span consistently
Browse files Browse the repository at this point in the history
This is such that setting data on a Transaction behaves the same as
setting data on a Span.
  • Loading branch information
rhcarvalho committed Jun 8, 2020
1 parent fc53353 commit c03883a
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 2 deletions.
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

0 comments on commit c03883a

Please sign in to comment.