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.

Sign up for GitHub

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

Jump to bottom

fix(tracing): Ensure you can pass null as parentSpan in startSpan* #12928

Merged
merged 1 commit into from
Jul 16, 2024
Merged

fix(tracing): Ensure you can pass null as parentSpan in startSpan* #12928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions packages/core/src/tracing/trace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export function startInactiveSpan(options: StartSpanOptions): Span {
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
const wrapper = options.scope
? (callback: () => Span) => withScope(options.scope, callback)
: customParentSpan
: customParentSpan !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

hmm I'm not sure I understand the PR description correctly but to me it looks like we're changing 2 things in this PR:

  1. make options.parentSpan accept null. This sounds fine to me, given we're aligning our public API better
  2. call withActiveSpan(null,...) in case parentSpan is null. Is this necessary? Shouldn't the behaviour be identical to simply invoking the callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it is important to call withActiveSpan in this case - if you pass null, the meaning is start this span as a root span without a parent, while undefined means the default behavior (the parent is the current active span). Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

See https://docs.sentry.io/platforms/javascript/tracing/instrumentation/custom-instrumentation/#withactivespan

Copy link
Member

Choose a reason for hiding this comment

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

missed this, thx for explaining!

? (callback: () => Span) => withActiveSpan(customParentSpan, callback)
: (callback: () => Span) => callback();

Expand Down Expand Up @@ -445,8 +445,8 @@ function getParentSpan(scope: Scope): SentrySpan | undefined {
return span;
}

function getActiveSpanWrapper<T>(parentSpan?: Span): (callback: () => T) => T {
return parentSpan
function getActiveSpanWrapper<T>(parentSpan: Span | undefined | null): (callback: () => T) => T {
return parentSpan !== undefined
? (callback: () => T) => {
return withActiveSpan(parentSpan, callback);
}
Expand Down
25 changes: 25 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,14 @@ describe('startSpan', () => {
expect(getActiveSpan()).toBe(undefined);
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'GET users/[id]' }, () => {
startSpan({ name: 'GET users/[id]', parentSpan: null }, span => {
expect(spanToJSON(span).parent_span_id).toBe(undefined);
});
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
client = new TestClient(options);
Expand Down Expand Up @@ -693,6 +701,15 @@ describe('startSpanManual', () => {
expect(getActiveSpan()).toBe(undefined);
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'GET users/[id]' }, () => {
startSpanManual({ name: 'child', parentSpan: null }, span => {
expect(spanToJSON(span).parent_span_id).toBe(undefined);
span.end();
});
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
client = new TestClient(options);
Expand Down Expand Up @@ -1014,6 +1031,14 @@ describe('startInactiveSpan', () => {
expect(getActiveSpan()).toBeUndefined();
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'outer' }, () => {
const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan: null });
expect(spanToJSON(span).parent_span_id).toBe(undefined);
span.end();
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
client = new TestClient(options);
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry/src/trace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,8 @@ export function continueTrace<T>(options: Parameters<typeof baseContinueTrace>[0
});
}

function getActiveSpanWrapper<T>(parentSpan?: Span | SentrySpan): (callback: () => T) => T {
return parentSpan
function getActiveSpanWrapper<T>(parentSpan: Span | SentrySpan | undefined | null): (callback: () => T) => T {
return parentSpan !== undefined
? (callback: () => T) => {
// We cast this, because the OTEL Span has a few more methods than our Span interface
// TODO: Add these missing methods to the Span interface
Expand Down
32 changes: 32 additions & 0 deletions packages/opentelemetry/test/trace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,16 @@ describe('trace', () => {
expect(getActiveSpan()).toBe(undefined);
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'GET users/[id' }, () => {
startSpan({ name: 'child', parentSpan: null }, span => {
// Due to the way we propagate the scope in OTEL,
// the parent_span_id is not actually undefined here, but comes from the propagation context
expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId);
});
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const client = getClient()!;
const transactionEvents: Event[] = [];
Expand Down Expand Up @@ -577,6 +587,17 @@ describe('trace', () => {
expect(getActiveSpan()).toBe(undefined);
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'outer' }, () => {
const span = startInactiveSpan({ name: 'test span', parentSpan: null });

// Due to the way we propagate the scope in OTEL,
// the parent_span_id is not actually undefined here, but comes from the propagation context
expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId);
span.end();
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const client = getClient()!;
const transactionEvents: Event[] = [];
Expand Down Expand Up @@ -856,6 +877,17 @@ describe('trace', () => {
expect(getActiveSpan()).toBe(undefined);
});

it('allows to pass parentSpan=null', () => {
startSpan({ name: 'outer' }, () => {
startSpanManual({ name: 'GET users/[id]', parentSpan: null }, span => {
// Due to the way we propagate the scope in OTEL,
// the parent_span_id is not actually undefined here, but comes from the propagation context
expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId);
span.end();
});
});
});

it('allows to force a transaction with forceTransaction=true', async () => {
const client = getClient()!;
const transactionEvents: Event[] = [];
Expand Down
3 changes: 2 additions & 1 deletion packages/types/src/startSpanOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ export interface StartSpanOptions {
/**
* If provided, make the new span a child of this span.
* If this is not provided, the new span will be a child of the currently active span.
* If this is set to `null`, the new span will have no parent span.
*/
parentSpan?: Span;
parentSpan?: Span | null;

/**
* If set to true, this span will be forced to be treated as a transaction in the Sentry UI, if possible and applicable.
Expand Down
Loading