Skip to content

feat(core): Add ignoreSpans option #17078

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ module.exports = [
import: createImport('init'),
ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
gzip: true,
limit: '144 KB',
limit: '146 KB',
},
{
name: '@sentry/node - without tracing',
Expand Down
5 changes: 5 additions & 0 deletions dev-packages/e2e-tests/test-applications/webpack-4/build.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ webpack(
minimize: true,
minimizer: [new TerserPlugin()],
},
performance: {
hints: false,
maxEntrypointSize: 512000,
maxAssetSize: 512000
},
plugins: [new HtmlWebpackPlugin(), new webpack.EnvironmentPlugin(['E2E_TEST_DSN'])],
mode: 'production',
// webpack 4 does not support ES2020 features out of the box, so we need to transpile them
Expand Down
64 changes: 48 additions & 16 deletions packages/core/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import { merge } from './utils/merge';
import { checkOrSetAlreadyCaught, uuid4 } from './utils/misc';
import { parseSampleRate } from './utils/parseSampleRate';
import { prepareEvent } from './utils/prepareEvent';
import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span';
import { getActiveSpan, showSpanDropWarning, spanToTraceContext } from './utils/spanUtils';
import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from './utils/syncpromise';
import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent';
Expand Down Expand Up @@ -1281,36 +1282,67 @@ function processBeforeSend(
event: Event,
hint: EventHint,
): PromiseLike<Event | null> | Event | null {
const { beforeSend, beforeSendTransaction, beforeSendSpan } = options;
const { beforeSend, beforeSendTransaction, beforeSendSpan, ignoreSpans } = options;
let processedEvent = event;

if (isErrorEvent(processedEvent) && beforeSend) {
return beforeSend(processedEvent, hint);
}

if (isTransactionEvent(processedEvent)) {
if (beforeSendSpan) {
// process root span
const processedRootSpanJson = beforeSendSpan(convertTransactionEventToSpanJson(processedEvent));
if (!processedRootSpanJson) {
showSpanDropWarning();
} else {
// update event with processed root span values
processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson));
// Avoid processing if we don't have to
if (beforeSendSpan || ignoreSpans) {
Copy link
Member

Choose a reason for hiding this comment

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

l: This should save a bit of performance overhead in case the passed array is empty

Suggested change
if (beforeSendSpan || ignoreSpans) {
if (beforeSendSpan || ignoreSpans && ignoreSpans.length) {

// 1. Process root span
const rootSpanJson = convertTransactionEventToSpanJson(processedEvent);

// 1.1 If the root span should be ignored, drop the whole transaction
if (shouldIgnoreSpan(rootSpanJson, ignoreSpans)) {
// dropping the whole transaction!
return null;
}

// process child spans
// 1.2 If a `beforeSendSpan` callback is defined, process the root span
if (beforeSendSpan) {
const processedRootSpanJson = beforeSendSpan(rootSpanJson);
if (!processedRootSpanJson) {
showSpanDropWarning();
} else {
// update event with processed root span values
processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson));
}
}

// 2. Process child spans
if (processedEvent.spans) {
const processedSpans: SpanJSON[] = [];
for (const span of processedEvent.spans) {
const processedSpan = beforeSendSpan(span);
if (!processedSpan) {
showSpanDropWarning();
processedSpans.push(span);

const initialSpans = processedEvent.spans;

for (const span of initialSpans) {
// 2.a If the child span should be ignored, reparent it to the root span
if (shouldIgnoreSpan(span, ignoreSpans)) {
reparentChildSpans(initialSpans, span);
continue;
}

// 2.b If a `beforeSendSpan` callback is defined, process the child span
if (beforeSendSpan) {
const processedSpan = beforeSendSpan(span);
if (!processedSpan) {
showSpanDropWarning();
processedSpans.push(span);
} else {
processedSpans.push(processedSpan);
}
} else {
processedSpans.push(processedSpan);
processedSpans.push(span);
}
}

const droppedSpans = processedEvent.spans.length - processedSpans.length;
if (droppedSpans) {
client.recordDroppedEvent('before_send', 'span', droppedSpans);
}
processedEvent.spans = processedSpans;
}
}
Expand Down
13 changes: 11 additions & 2 deletions packages/core/src/envelope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
getSdkMetadataForEnvelopeHeader,
} from './utils/envelope';
import { uuid4 } from './utils/misc';
import { shouldIgnoreSpan } from './utils/should-ignore-span';
import { showSpanDropWarning, spanToJSON } from './utils/spanUtils';

/**
Expand Down Expand Up @@ -122,7 +123,15 @@ export function createSpanEnvelope(spans: [SentrySpan, ...SentrySpan[]], client?
...(!!tunnel && dsn && { dsn: dsnToString(dsn) }),
};

const beforeSendSpan = client?.getOptions().beforeSendSpan;
const { beforeSendSpan, ignoreSpans } = client?.getOptions() || {};

const filteredSpans = ignoreSpans ? spans.filter(span => !shouldIgnoreSpan(spanToJSON(span), ignoreSpans)) : spans;
const droppedSpans = spans.length - filteredSpans.length;

if (droppedSpans) {
client?.recordDroppedEvent('before_send', 'span', droppedSpans);
}

const convertToSpanJSON = beforeSendSpan
? (span: SentrySpan) => {
const spanJson = spanToJSON(span);
Expand All @@ -138,7 +147,7 @@ export function createSpanEnvelope(spans: [SentrySpan, ...SentrySpan[]], client?
: spanToJSON;

const items: SpanItem[] = [];
for (const span of spans) {
for (const span of filteredSpans) {
const spanJson = convertToSpanJSON(span);
if (spanJson) {
items.push(createSpanEnvelopeItem(spanJson));
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/types-hoist/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import type { StackLineParser, StackParser } from './stacktrace';
import type { TracePropagationTargets } from './tracing';
import type { BaseTransportOptions, Transport } from './transport';

interface IgnoreSpanFilter {
Copy link
Member

Choose a reason for hiding this comment

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

m: I think we can type this a bit smarter, right? Currently, this allows users to pass in an empty object but we could type it in a way so that at least one prop is required

Suggested change
interface IgnoreSpanFilter {
type IgnoreSpanFilter = {
name: string | RegExp;
op?: string | RegExp;
} | {
name?: string | RegExp;
op: string | RegExp;
}

we should probably still check that this is also enforced when we apply the option (which we do already)

name?: string | RegExp;
op?: string | RegExp;
}

export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOptions> {
/**
* Enable debug functionality in the SDK itself. If `debug` is set to `true` the SDK will attempt
Expand Down Expand Up @@ -208,6 +213,13 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
*/
ignoreTransactions?: Array<string | RegExp>;

/**
* A list of span names or patterns to ignore.
*
* @default []
*/
ignoreSpans?: (string | RegExp | IgnoreSpanFilter)[];

/**
* A URL to an envelope tunnel endpoint. An envelope tunnel is an HTTP endpoint
* that accepts Sentry envelopes for forwarding. This can be used to force data
Expand Down
67 changes: 67 additions & 0 deletions packages/core/src/utils/should-ignore-span.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import type { ClientOptions } from '../types-hoist/options';
import type { SpanJSON } from '../types-hoist/span';
import { isMatchingPattern, stringMatchesSomePattern } from './string';

/**
* Check if a span should be ignored based on the ignoreSpans configuration.
*/
export function shouldIgnoreSpan(
span: Pick<SpanJSON, 'description' | 'op'>,
ignoreSpans: ClientOptions['ignoreSpans'],
): boolean {
if (!ignoreSpans?.length) {
return false;
}

if (!span.description) {
return false;
}
Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

l: I can't think of an actual example right now but theoretically, shouldn't we still match on the op of a span without a description?


// First we check the simple string/regex patterns - if the name matches any of them, we ignore the span
const simplePatterns = ignoreSpans.filter(isStringOrRegExp);
if (simplePatterns.length && stringMatchesSomePattern(span.description, simplePatterns)) {
return true;
}

// Then we check the more complex patterns, where both parts must match
for (const pattern of ignoreSpans) {
// Have already checked for simple patterns, so we can skip these
if (isStringOrRegExp(pattern) || (!pattern.name && !pattern.op)) {
continue;
}
Comment on lines +21 to +31
Copy link
Member

Choose a reason for hiding this comment

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

l: I think performance-wise, it makes more sense to handle everything in one for loop here, no? If users didn't pass simple patterns or none of them matched, we'd iterate twice over the ignoreSpans array. Also avoids calling isStringOrRegExp twice in this case.
(not the end of the world but it scales with the number of spans...)


const nameMatches = pattern.name ? isMatchingPattern(span.description, pattern.name) : true;
const opMatches = pattern.op ? span.op && isMatchingPattern(span.op, pattern.op) : true;

if (nameMatches && opMatches) {
return true;
}
Comment on lines +33 to +38
Copy link
Member

Choose a reason for hiding this comment

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

this is pretty clever, took me a minute to understand :D
maybe we can add a comment that it's only safe to return true here because we already bailed earlier if both op and name were undefined.

Suggested change
const nameMatches = pattern.name ? isMatchingPattern(span.description, pattern.name) : true;
const opMatches = pattern.op ? span.op && isMatchingPattern(span.op, pattern.op) : true;
if (nameMatches && opMatches) {
return true;
}
// because of the check above, only one one of them can be undefined, so falling
// back to `true` (in combination with the check below) is safe here
const nameMatches = pattern.name ? isMatchingPattern(span.description, pattern.name) : true;
const opMatches = pattern.op ? span.op && isMatchingPattern(span.op, pattern.op) : true;
if (nameMatches && opMatches) {
return true;
}

}

return false;
}

/**
* Takes a list of spans, and a span that was dropped, and re-parents the child spans of the dropped span to the parent of the dropped span, if possible.
* This mutates the spans array in place!
*/
export function reparentChildSpans(spans: SpanJSON[], dropSpan: SpanJSON): void {
const droppedSpanParentId = dropSpan.parent_span_id;
const droppedSpanId = dropSpan.span_id;

// This should generally not happen, as we do not apply this on root spans
// but to be safe, we just bail in this case
if (!droppedSpanParentId) {
return;
}

for (const span of spans) {
if (span.parent_span_id === droppedSpanId) {
span.parent_span_id = droppedSpanParentId;
}
}
}

function isStringOrRegExp(value: unknown): value is string | RegExp {
return typeof value === 'string' || value instanceof RegExp;
}
Loading
Loading