Skip to content

feat(core): Ensure startSpan() can handle spans that require parent #10386

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 3 commits into from
Jan 30, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
// disable pageload transaction
integrations: [Sentry.BrowserTracing({ tracingOrigins: ['http://example.com'], startTransactionOnPageLoad: false })],
tracesSampleRate: 1,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeUrlRegex, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest(
'there should be no span created for fetch requests with no active span',
async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestPath({ testDir: __dirname });

let requestCount = 0;
page.on('request', request => {
expect(envelopeUrlRegex.test(request.url())).toBe(false);
requestCount++;
});

await page.goto(url);

// Here are the requests that should exist:
// 1. HTML page
// 2. Init JS bundle
// 3. Subject JS bundle
// 4 [OPTIONAl] CDN JS bundle
// and then 3 fetch requests
if (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle_')) {
expect(requestCount).toBe(7);
} else {
expect(requestCount).toBe(6);
}
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
// disable pageload transaction
integrations: [Sentry.BrowserTracing({ tracingOrigins: ['http://example.com'], startTransactionOnPageLoad: false })],
tracesSampleRate: 1,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const xhr_1 = new XMLHttpRequest();
xhr_1.open('GET', 'http://example.com/0');
xhr_1.send();

const xhr_2 = new XMLHttpRequest();
xhr_2.open('GET', 'http://example.com/1');
xhr_2.send();

const xhr_3 = new XMLHttpRequest();
xhr_3.open('GET', 'http://example.com/2');
xhr_3.send();
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeUrlRegex, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest(
'there should be no span created for xhr requests with no active span',
async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestPath({ testDir: __dirname });

let requestCount = 0;
page.on('request', request => {
expect(envelopeUrlRegex.test(request.url())).toBe(false);
requestCount++;
});

await page.goto(url);

// Here are the requests that should exist:
// 1. HTML page
// 2. Init JS bundle
// 3. Subject JS bundle
// 4 [OPTIONAl] CDN JS bundle
// and then 3 fetch requests
if (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle_')) {
expect(requestCount).toBe(7);
} else {
expect(requestCount).toBe(6);
}
},
);
2 changes: 1 addition & 1 deletion dev-packages/browser-integration-tests/utils/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { Page, Request } from '@playwright/test';
import type { EnvelopeItemType, Event, EventEnvelopeHeaders } from '@sentry/types';

const envelopeUrlRegex = /\.sentry\.io\/api\/\d+\/envelope\//;
export const envelopeUrlRegex = /\.sentry\.io\/api\/\d+\/envelope\//;

Check failure

Code scanning / CodeQL

Missing regular expression anchor

When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it.

export const envelopeParser = (request: Request | null): unknown[] => {
// https://develop.sentry.dev/sdk/envelopes/
Expand Down
14 changes: 12 additions & 2 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();

const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx);
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);

// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);

Expand Down Expand Up @@ -128,7 +130,9 @@ export function startSpanManual<T>(
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();

const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx);
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);

// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);

Expand Down Expand Up @@ -174,6 +178,12 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
context.scope.getSpan()
: getActiveSpan();

const shouldSkipSpan = context.onlyIfParent && !parentSpan;

if (shouldSkipSpan) {
return undefined;
}

if (parentSpan) {
// eslint-disable-next-line deprecation/deprecation
return parentSpan.startChild(ctx);
Expand Down
62 changes: 62 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,28 @@ describe('startSpan', () => {
});
});
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

expect(span).toBeUndefined();
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

return span;
});

expect(span).toBeDefined();
});
});
});

describe('startSpanManual', () => {
Expand Down Expand Up @@ -415,6 +437,28 @@ describe('startSpanManual', () => {
});
});
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

expect(span).toBeUndefined();
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

return span;
});

expect(span).toBeDefined();
});
});
});

describe('startInactiveSpan', () => {
Expand Down Expand Up @@ -479,6 +523,24 @@ describe('startInactiveSpan', () => {
span?.end();
});
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });

expect(span).toBeUndefined();
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });

return span;
});

expect(span).toBeDefined();
});
});
});

describe('continueTrace', () => {
Expand Down
20 changes: 17 additions & 3 deletions packages/opentelemetry/src/trace.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { Span, Tracer } from '@opentelemetry/api';
import { context } from '@opentelemetry/api';
import { SpanStatusCode, trace } from '@opentelemetry/api';
import { suppressTracing } from '@opentelemetry/core';
import { SDK_VERSION, handleCallbackErrors } from '@sentry/core';
import type { Client } from '@sentry/types';

Expand All @@ -22,7 +24,11 @@ export function startSpan<T>(spanContext: OpenTelemetrySpanContext, callback: (s

const { name } = spanContext;

return tracer.startActiveSpan(name, spanContext, span => {
const activeCtx = context.active();
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;

return tracer.startActiveSpan(name, spanContext, ctx, span => {
_applySentryAttributesToSpan(span, spanContext);

return handleCallbackErrors(
Expand All @@ -49,7 +55,11 @@ export function startSpanManual<T>(spanContext: OpenTelemetrySpanContext, callba

const { name } = spanContext;

return tracer.startActiveSpan(name, spanContext, span => {
const activeCtx = context.active();
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;

return tracer.startActiveSpan(name, spanContext, ctx, span => {
_applySentryAttributesToSpan(span, spanContext);

return handleCallbackErrors(
Expand Down Expand Up @@ -81,7 +91,11 @@ export function startInactiveSpan(spanContext: OpenTelemetrySpanContext): Span {

const { name } = spanContext;

const span = tracer.startSpan(name, spanContext);
const activeCtx = context.active();
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;

const span = tracer.startSpan(name, spanContext, ctx);

_applySentryAttributesToSpan(span, spanContext);

Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export interface OpenTelemetrySpanContext {
origin?: SpanOrigin;
source?: TransactionSource;
scope?: Scope;
onlyIfParent?: boolean;

// Base SpanOptions we support
attributes?: Attributes;
Expand Down
63 changes: 63 additions & 0 deletions packages/opentelemetry/test/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Span } from '@opentelemetry/api';
import { SpanKind } from '@opentelemetry/api';
import { TraceFlags, context, trace } from '@opentelemetry/api';
import type { ReadableSpan } from '@opentelemetry/sdk-trace-base';
import { Span as SpanClass } from '@opentelemetry/sdk-trace-base';
import type { PropagationContext } from '@sentry/types';

import { getClient } from '../src/custom/hub';
Expand Down Expand Up @@ -260,6 +261,28 @@ describe('trace', () => {
},
);
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

expect(span).not.toBeInstanceOf(SpanClass);
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

return span;
});

expect(span).toBeInstanceOf(SpanClass);
});
});
});

describe('startInactiveSpan', () => {
Expand Down Expand Up @@ -349,6 +372,24 @@ describe('trace', () => {
});
expect(getSpanKind(span)).toEqual(SpanKind.CLIENT);
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });

expect(span).not.toBeInstanceOf(SpanClass);
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });

return span;
});

expect(span).toBeInstanceOf(SpanClass);
});
});
});

describe('startSpanManual', () => {
Expand Down Expand Up @@ -419,6 +460,28 @@ describe('trace', () => {
);
});
});

describe('onlyIfParent', () => {
it('does not create a span if there is no parent', () => {
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

expect(span).not.toBeInstanceOf(SpanClass);
});

it('creates a span if there is a parent', () => {
const span = startSpan({ name: 'parent span' }, () => {
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
return span;
});

return span;
});

expect(span).toBeInstanceOf(SpanClass);
});
});
});

describe('trace (tracing disabled)', () => {
Expand Down
Loading