feat(core): Apply ignoreSpans to streamed spans#19934
feat(core): Apply ignoreSpans to streamed spans#19934Lms24 wants to merge 5 commits intolms/feat-span-firstfrom
ignoreSpans to streamed spans#19934Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Nuxt
Other
Bug Fixes 🐛Ci
Other
Documentation 📚
Internal Changes 🔧Core
Deps
Deps Dev
Other
🤖 This preview updates automatically when you update the PR. |
Lms24
force-pushed
the
lms/feat-span-first
branch
from
5963170 to
de2e194
Compare
Lms24
force-pushed
the
lms/feat-core-ignorespans-streaming
branch
from
e90d6d3 to
11cfe2b
Compare
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
clankers gonna clank
| { | ||
| category: 'transaction', | ||
| quantity: 4, | ||
| category: 'span', |
There was a problem hiding this comment.
a couple of pre-existing tests (like this one) needed adjustments because they previously reported the wrong telemetry type (for span streaming). Also the number of dropped spans increased because we now also count child spans.
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [Sentry.spanStreamingIntegration()], | ||
| ignoreSpans: [/ignore/], | ||
| parentSpanIsAlwaysRootSpan: false, |
There was a problem hiding this comment.
worth noting: For this test, I enabled parentSpanIsAlwaysRootSpan which due to the lack of async context is our default behaviour in browser. I enabled it here to ensure that the parenting order in the test (see file below) works as expected in core/browser.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Core path reads
opfrom wrong property for ignore check- Fixed _shouldIgnoreStreamedSpan to check spanArguments.op before spanArguments.attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP], aligning with the OTel implementation and ensuring op-based ignoreSpans patterns work correctly in the core/browser streaming path.
Or push these changes by commenting:
@cursor push 6b34ec41de
Preview (6b34ec41de)
diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts
--- a/packages/core/src/tracing/trace.ts
+++ b/packages/core/src/tracing/trace.ts
@@ -594,7 +594,10 @@
}
return shouldIgnoreSpan(
- { description: spanArguments.name || '', op: spanArguments.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP] },
+ {
+ description: spanArguments.name || '',
+ op: spanArguments.op || spanArguments.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP],
+ },
ignoreSpans,
);
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
|
||
| return shouldIgnoreSpan( | ||
| { description: spanArguments.name || '', op: spanArguments.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP] }, | ||
| ignoreSpans, |
There was a problem hiding this comment.
Core path reads op from wrong property for ignore check
Medium Severity
In _shouldIgnoreStreamedSpan, the op is read from spanArguments.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP], but at this point in the flow the op lives on spanArguments.op (set by parseSentrySpanArguments which spreads StartSpanOptions). The SEMANTIC_ATTRIBUTE_SENTRY_OP attribute is only populated later when the SentrySpan constructor runs. This means any ignoreSpans pattern that filters by op (e.g. { op: 'http.server' }) will silently fail to match in the core/browser streaming path. The OTel path correctly handles this by checking both: options.op || options.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP].
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
AIUI, it looks like this dodges the issue of "don't have all the data to decide to ignore spans until they're already started, which is too late to ignore them" by making the decision limited to description and op? If so, lgtm, pending investigating/resolving the nit from cursor about reading from the attributes vs the settings object directly.
2 participants
This PR Implements applying the
ignoreSpansoption to streamed spans (previously this option had no effect). It covers both, our core/browser- as well as the OTel/Node-based implementation.Behaviour on all implementations:
ignoredclient outcome for each dropped span (segment + children)ignoredclient outcome for the dropped childspanin a callback can always be accessed as we do with sampling.sample_rateclient report outcomes for negatively sampled child spans. Meaning, when our sampling logic makes a negative decision, we record outcomes for the segment span + every created child span of the negatively sampled segment. This is additional logic toignoreSpansbut we need to differentiate between outcome reason (ignoredvs.sample_rate), so I had to add this to this PR. This is expected behaviour and something we would have needed to do for span streaming anyway.Implementation Node/OTel:
SentrySamplerto effectively make sampling decisions forignoreSpansprior to applying our actual sampling options. So to be clear: There's no actual sampling logic applied but using the sampler allows us to modify the OTel context, which we can then later use to correctly either drop all spans of a segment or just one span (see above). We do this by adding flags to thetraceState, which the context manager picks up and changes its context accordingly.Implementation Core/Browser:
ignoreSpansright on thestart*span*APIs, prior to sampling.dropReasonon theSentrySpanclass that allows us to report the correct client outcome for dropped child spans, both for sampling as well asignoreSpans