Skip to content

ref(core): Remove beforeSendSpan null return warning #24129

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

Draft
msonnb wants to merge 1 commit into
base: develop
Choose a base branch
from
+11 −71
Draft

ref(core): Remove beforeSendSpan null return warning #24129

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
17 changes: 1 addition & 16 deletions packages/core/src/tracing/spans/beforeSendSpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { DEBUG_BUILD } from '../../debug-build';
import type { BeforeSendStaticSpanCallback, BeforeSendStreamedSpanCallback } from '../../types/options';
import type { SpanJSON, StreamedSpanJSON } from '../../types/span';
import { addNonEnumerableProperty } from '../../utils/object';
import { consoleSandbox } from '../../utils/debug-logger';
import { safeCallback } from '../../utils/safeCallback';

/**
Expand Down Expand Up @@ -57,7 +56,6 @@ export function isStaticBeforeSendSpanCallback(callback: unknown): callback is B
return !!callback && typeof callback === 'function' && '_static' in callback && !!callback._static;
}

let hasShownSpanDropWarning = false;
/**
* Apply a user-provided beforeSendSpan callback to a span JSON.
*/
Expand All @@ -72,18 +70,5 @@ export function applyBeforeSendSpanCallback<T extends StreamedSpanJSON | SpanJSO
() => beforeSendSpan(span),
() => span,
);
if (modifiedSpan) {
return modifiedSpan;
}

if (!hasShownSpanDropWarning) {
consoleSandbox(() => {
// eslint-disable-next-line no-console
console.warn(
'[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.',
);
});
hasShownSpanDropWarning = true;
}
return span;
return modifiedSpan || span;
}
45 changes: 0 additions & 45 deletions packages/core/test/lib/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,51 +1727,6 @@ describe('Client', () => {
expect(loggerLogSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.');
});

test('does not discard span and warn when returning null from `beforeSendSpan', () => {
const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined);

// @ts-expect-error - intentionally violating the type signature here
const beforeSendSpan = withStaticSpan(vi.fn(() => null));

const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan });
const client = new TestClient(options);

const transaction: Event = {
transaction: '/dogs/are/great',
type: 'transaction',
spans: [
{
description: 'first span',
span_id: '9e15bf99fbe4bc80',
start_timestamp: 1591603196.637835,
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
data: {},
status: 'ok',
},
{
description: 'second span',
span_id: 'aa554c1f506b0783',
start_timestamp: 1591603196.637835,
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
data: {},
status: 'ok',
},
],
};
client.captureEvent(transaction);

expect(beforeSendSpan).toHaveBeenCalledTimes(3);
const capturedEvent = TestClient.instance!.event!;
expect(capturedEvent.spans).toHaveLength(2);
expect(client['_outcomes']).toEqual({});

expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
expect(consoleWarnSpy).toHaveBeenCalledWith(
'[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.',
);
consoleWarnSpy.mockRestore();
});

test("doesn't throw if the `beforeSendSpan` callback throws", () => {
const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error').mockImplementation(() => undefined);
const error = new Error('beforeSendSpan is broken');
Expand Down
20 changes: 10 additions & 10 deletions packages/core/test/lib/tracing/spans/captureSpan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,7 @@ describe('captureSpan', () => {
expect(beforeSendSpan).not.toHaveBeenCalled();
});

it('logs a warning if the beforeSendSpan callback returns null', () => {
const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined);
it('keeps the span if the beforeSendSpan callback returns null', () => {
const beforeSendSpan = vi.fn(() => null as unknown as StreamedSpanJSON);

const client = new TestClient(
Expand All @@ -522,16 +521,17 @@ describe('captureSpan', () => {
}),
);

const span = startInactiveSpan({ name: 'my-span', attributes: { 'sentry.op': 'http.client' } });
span.end();

captureSpan(span, client);
const span = withScope(scope => {
scope.setClient(client);
const span = startInactiveSpan({ name: 'my-span', attributes: { 'sentry.op': 'http.client' } });
span.end();
return span;
});

expect(consoleWarnSpy).toHaveBeenCalledWith(
'[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.',
);
const serialized = captureSpan(span, client);

consoleWarnSpy.mockRestore();
expect(serialized.span_id).toBe(span.spanContext().spanId);
expect(serialized.name).toBe('my-span');
});

it('keeps the span and logs an error if the beforeSendSpan callback throws', () => {
Expand Down
Loading