Skip to content

ref: Collect child spans references via non-enumerable on Span object #10715

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
Feb 20, 2024
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
4 changes: 2 additions & 2 deletions packages/core/src/tracing/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru
...customSamplingContext,
});
if (transaction.isRecording()) {
transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
transaction.initSpanRecorder();
}
if (client) {
client.emit('startTransaction', transaction);
Expand Down Expand Up @@ -122,7 +122,7 @@ export function startIdleTransaction(
...customSamplingContext,
});
if (transaction.isRecording()) {
transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
transaction.initSpanRecorder();
}
if (client) {
client.emit('startTransaction', transaction);
Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from '../utils/spanUtils';
import type { SpanStatusType } from './spanstatus';
import { setHttpStatus } from './spanstatus';
import { addChildSpanToSpan } from './trace';

/**
* Keeps track of finished spans for a given transaction
Expand Down Expand Up @@ -401,6 +402,11 @@ export class SentrySpan implements SpanInterface {
childSpan.spanRecorder.add(childSpan);
}

// To allow for interoperability we track the children of a span twice: Once with the span recorder (old) once with
// the `addChildSpanToSpan`. Eventually we will only use `addChildSpanToSpan` and drop the span recorder.
// To ensure interoperability with the `startSpan` API, `addChildSpanToSpan` is also called here.
addChildSpanToSpan(this, childSpan);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addChildSpanToSpan(this, childSpan);
// For now, we capture this here as well in order to ensure backwards compatibility
// eventually this will only be captured in `startSpan` methods
addChildSpanToSpan(this, childSpan);


const rootSpan = getRootSpan(this);
// TODO: still set span.transaction here until we have a more permanent solution
// Probably similarly to the weakmap we hold in node-experimental
Expand Down
49 changes: 49 additions & 0 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
});
}

if (parentSpan) {
addChildSpanToSpan(parentSpan, span);
}

setCapturedScopesOnSpan(span, scope, isolationScope);

return span;
Expand Down Expand Up @@ -307,6 +311,10 @@ function createChildSpanOrTransaction(
});
}

if (parentSpan) {
addChildSpanToSpan(parentSpan, span);
}

setCapturedScopesOnSpan(span, scope, isolationScope);

return span;
Expand All @@ -330,6 +338,47 @@ function normalizeContext(context: StartSpanOptions): TransactionContext {
return context;
}

const CHILD_SPANS_FIELD = '_sentryChildSpans';

type SpanWithPotentialChildren = Span & {
[CHILD_SPANS_FIELD]?: Set<Span>;
};

/**
* Adds an opaque child span reference to a span.
*/
export function addChildSpanToSpan(span: SpanWithPotentialChildren, childSpan: Span): void {
if (span[CHILD_SPANS_FIELD] && span[CHILD_SPANS_FIELD].size < 1000) {
span[CHILD_SPANS_FIELD].add(childSpan);
} else {
span[CHILD_SPANS_FIELD] = new Set([childSpan]);
}
}

/**
* Obtains the entire span tree, meaning a span + all of its descendants for a particular span.
*/
export function getSpanTree(span: SpanWithPotentialChildren): Span[] {
const resultSet = new Set<Span>();

function addSpanChildren(span: SpanWithPotentialChildren): void {
// This exit condition is required to not infinitely loop in case of a circular dependency.
if (resultSet.has(span)) {
return;
} else {
resultSet.add(span);
const childSpans = span[CHILD_SPANS_FIELD] ? Array.from(span[CHILD_SPANS_FIELD]) : [];
for (const childSpan of childSpans) {
addSpanChildren(childSpan);
}
}
}

addSpanChildren(span);

return Array.from(resultSet);
}

const SCOPE_ON_START_SPAN_FIELD = '_sentryScope';
const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope';

Expand Down
9 changes: 3 additions & 6 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE
import { spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
import { SentrySpan, SpanRecorder } from './sentrySpan';
import { getCapturedScopesOnSpan } from './trace';
import { getCapturedScopesOnSpan, getSpanTree } from './trace';

/** JSDoc */
export class Transaction extends SentrySpan implements TransactionInterface {
Expand Down Expand Up @@ -293,11 +293,8 @@ export class Transaction extends SentrySpan implements TransactionInterface {
return undefined;
}

// eslint-disable-next-line deprecation/deprecation
const finishedSpans = this.spanRecorder
? // eslint-disable-next-line deprecation/deprecation
this.spanRecorder.spans.filter(span => span !== this && spanToJSON(span).timestamp)
: [];
// We only want to include finished spans in the event
const finishedSpans = getSpanTree(this).filter(span => span !== this && spanToJSON(span).timestamp);

if (this._trimEnd && finishedSpans.length > 0) {
const endTimes = finishedSpans.map(span => spanToJSON(span).timestamp).filter(Boolean) as number[];
Expand Down
29 changes: 29 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,16 @@ describe('startSpan', () => {
expect.anything(),
);
});

it('sets a child span reference on the parent span', () => {
expect.assertions(1);
startSpan({ name: 'outer' }, (outerSpan: any) => {
startSpan({ name: 'inner' }, innerSpan => {
const childSpans = Array.from(outerSpan._sentryChildSpans);
expect(childSpans).toContain(innerSpan);
});
});
});
});

describe('startSpanManual', () => {
Expand Down Expand Up @@ -545,6 +555,16 @@ describe('startSpanManual', () => {
expect(span).toBeDefined();
});
});

it('sets a child span reference on the parent span', () => {
expect.assertions(1);
startSpan({ name: 'outer' }, (outerSpan: any) => {
startSpanManual({ name: 'inner' }, innerSpan => {
const childSpans = Array.from(outerSpan._sentryChildSpans);
expect(childSpans).toContain(innerSpan);
});
});
});
});

describe('startInactiveSpan', () => {
Expand Down Expand Up @@ -674,6 +694,15 @@ describe('startInactiveSpan', () => {
expect.anything(),
);
});

it('sets a child span reference on the parent span', () => {
expect.assertions(1);
startSpan({ name: 'outer' }, (outerSpan: any) => {
const innerSpan = startInactiveSpan({ name: 'inner' });
const childSpans = Array.from(outerSpan._sentryChildSpans);
expect(childSpans).toContain(innerSpan);
});
});
});

describe('continueTrace', () => {
Expand Down
5 changes: 2 additions & 3 deletions packages/opentelemetry/src/custom/transaction.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Hub } from '@sentry/core';
import { Transaction } from '@sentry/core';
import type { ClientOptions, Hub as HubInterface, TransactionContext } from '@sentry/types';
import type { Hub as HubInterface, TransactionContext } from '@sentry/types';

/**
* This is a fork of core's tracing/hubextensions.ts _startTransaction,
Expand All @@ -9,13 +9,12 @@ import type { ClientOptions, Hub as HubInterface, TransactionContext } from '@se
export function startTransaction(hub: HubInterface, transactionContext: TransactionContext): Transaction {
// eslint-disable-next-line deprecation/deprecation
const client = hub.getClient();
const options: Partial<ClientOptions> = (client && client.getOptions()) || {};

// eslint-disable-next-line deprecation/deprecation
const transaction = new Transaction(transactionContext, hub as Hub);
// Since we do not do sampling here, we assume that this is _always_ sampled
// Any sampling decision happens in OpenTelemetry's sampler
transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
transaction.initSpanRecorder();

if (client) {
client.emit('startTransaction', transaction);
Expand Down
32 changes: 0 additions & 32 deletions packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,22 +272,6 @@ describe('SentrySpan', () => {
expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
});

test('maxSpans correctly limits number of spans', () => {
const options = getDefaultBrowserClientOptions({
_experiments: { maxSpans: 3 },
tracesSampleRate: 1,
});
const _hub = new Hub(new BrowserClient(options));
const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
const transaction = _hub.startTransaction({ name: 'test' });
for (let i = 0; i < 10; i++) {
const child = transaction.startChild();
child.end();
}
transaction.end();
expect(spy.mock.calls[0][0].spans).toHaveLength(3);
});

test('no span recorder created if transaction.sampled is false', () => {
const options = getDefaultBrowserClientOptions({
tracesSampleRate: 1,
Expand Down Expand Up @@ -421,22 +405,6 @@ describe('SentrySpan', () => {
expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
});

test('maxSpans correctly limits number of spans', () => {
const options = getDefaultBrowserClientOptions({
_experiments: { maxSpans: 3 },
tracesSampleRate: 1,
});
const _hub = new Hub(new BrowserClient(options));
const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
const transaction = _hub.startTransaction({ name: 'test' });
for (let i = 0; i < 10; i++) {
const child = transaction.startChild();
child.end();
}
transaction.end();
expect(spy.mock.calls[0][0].spans).toHaveLength(3);
});

test('no span recorder created if transaction.sampled is false', () => {
const options = getDefaultBrowserClientOptions({
tracesSampleRate: 1,
Expand Down