feat(core): Add ignoreSpans option
#17078
Conversation
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Span Ignoring Fails for Operations Without Descriptions
The shouldIgnoreSpan function returns false early if span.description is falsy. This prevents ignoreSpans patterns that match solely on the op field (e.g., { op: 'http.client' }) from being applied. Consequently, spans without a description but with a matching op are not ignored, as they never reach the logic designed for op-only pattern matching.
packages/core/src/utils/should-ignore-span.ts#L15-L18
sentry-javascript/packages/core/src/utils/should-ignore-span.ts
Lines 15 to 18 in dc0d9d1
Was this report helpful? Give feedback by reacting with 👍 or 👎
| if (!span.description) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
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?
| // update event with processed root span values | ||
| processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson)); | ||
| // Avoid processing if we don't have to | ||
| if (beforeSendSpan || ignoreSpans) { |
There was a problem hiding this comment.
l: This should save a bit of performance overhead in case the passed array is empty
| if (beforeSendSpan || ignoreSpans) { | |
| if (beforeSendSpan || ignoreSpans && ignoreSpans.length) { |
| 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; | ||
| } |
There was a problem hiding this comment.
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...)
| @@ -10,6 +10,11 @@ import type { StackLineParser, StackParser } from './stacktrace'; | |||
| import type { TracePropagationTargets } from './tracing'; | |||
| import type { BaseTransportOptions, Transport } from './transport'; | |||
|
|
|||
| interface IgnoreSpanFilter { | |||
There was a problem hiding this comment.
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
| 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)
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } |
This adds a new
ignoreSpansoption to all SDKs. This can be used as follows:this will drop spans before they are sent. Eventual child spans in the same envelope will be re-parented, if possible.
Closes #16820