fix(cloudflare): Skip binding instrumentation work when spans are not sent - #24655
Conversation
db6f184 to
28885a9
Compare
size-limit report 📦
|
|
h: I don't think this is a good approach. It's easy to forget to add this, moreover it goes against our recording of dropped spans. So users would see less spans than client outcomes report as dropped. If I understand the issue correctly, it mostly started out by looking at sql sanitization. Maybe we can keep it scoped to that and only do it if the span is recording, i.e. via |
| /** | ||
| * Returns `true` when an active span exists and is not sampled. Returns `false` when there is no active span. | ||
| */ | ||
| export function isInUnsampledSpan(): boolean { |
There was a problem hiding this comment.
m: I find this name non-ideal, as it implies there is def. a span 😅
I would adjust this to:
- check
hasSpansEnabled()first, which is generally what we use to guard such things - this will not cover the "unsampled parent" case, but I would really check this specifically only if there is a span. the current behavior would (in theory) also swallow stuff if there is simply no parent yet - realistically this will not matter today I assume because there will always be a parent, but we may as well guard this "properly"
Plus, hasSpansEnabled can generally be tree shaken via the SENTRY_TRACING flag thing (added benefit, yay)
So I'd adjust to:
function activeSpanIsUnsampled() {
const activeSpan = getActiveSpan();
return !!activeSpan && !spanIsSampled(activeSpan);
}
// every call site
if (!hasSpansEnabled() || activeSpanIsUnsampled() {
// opt out
}
…be sent The binding instrumentations (DO SQL, DO KV, sync KV, D1, R2, queue producer, agent callable RPC) built span data on every call, even when the SDK was disabled, had no DSN, had no tracing configured, or ran under an unsampled parent. On SQL-heavy Durable Objects the per-query sanitizing alone showed up as a large CPU regression with `tracesSampleRate: 0`. They now call the original method directly in these cases. D1 keeps adding breadcrumbs while the SDK is enabled, and sanitizes the query only when a statement runs. `setAlarm` keeps its span path, because it stores the span context for the alarm trace link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d7bf44a to
48b1d3e
Compare
|
@andreiborza now only the sql part is skipped. In fact it still adds a span, but without the calculations |
That would avoid copying core's logic. But two problems with that:
|
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
A few opportunities to polish up a bit, but I think this is good to move forward with.
One idea for follow-up: in packages/cloudflare/src/instrumentations/worker/instrumentD1.ts, D1 calls sanitizeSqlQuery on each prepare and each exec, whether or not spans are sampled. The breadcrumb also uses the sanitized text, so the same fast path does not drop in directly. I wouldn't stuff that into this PR, though. Should be its own change.
| const [query, ...bindings] = args as [string, ...unknown[]]; | ||
|
|
||
| const activeSpan = getActiveSpan(); | ||
| const spanIsNeverSent = !hasSpansEnabled() || (!!activeSpan && !spanIsSampled(activeSpan)); |
There was a problem hiding this comment.
Not a bug, but possibly a missed optimization: This predicts what createChildOrRootSpan and _startChildSpan in packages/core/src/tracing/trace.ts will decide, and covers "no tracing" and "unsampled parent", but not suppressTracing(), where _startChildSpan makes a non-recording child even under a sampled parent. So in that edge case, the code still sanitizes for nothing.
That's not a huge deal, but the risk is drift. If core adds a new reason to drop a child, this check will not know about it. In the unsafe direction (a span predicted dropped but actually sent, much less likely), users would get spans named exec with no db.query.text.
There's nothing like that now, which is why this isn't a bug. parentSpanIsAlwaysRootSpan picks the root span, which has the same sampling decision. An ignored child is never set active (makeSpanActive in startSpan), so the active span stays the sampled parent. An ignored root span is set active and is unsampled. Its children take the ignored drop reason, as before.
Suggestion: keep the check, but move it to a small named helper, for example spanWillNotBeRecorded(). Give it a short comment that it must agree with _startChildSpan in core. Also read getActiveSpan() only after hasSpansEnabled() returns true. As written, getActiveSpan() runs on every call, even with tracing off:
function childSpanWillNotBeRecorded(): boolean {
if (!hasSpansEnabled()) {
return true;
}
const activeSpan = getActiveSpan();
return !!activeSpan && !spanIsSampled(activeSpan);
}This also matches the shape @mydea proposed.
|
|
||
| const activeSpan = getActiveSpan(); | ||
| const spanIsNeverSent = !hasSpansEnabled() || (!!activeSpan && !spanIsSampled(activeSpan)); | ||
| if (spanIsNeverSent && !mayTargetCloudflareInternalTable(query)) { |
There was a problem hiding this comment.
_shouldIgnoreStreamedSpan runs before the parent sampling check. Before this PR, under an unsampled parent, a query that matched a name-based or db.query.text-based ignoreSpans rule recorded an ignored outcome.
Now the fast path starts the span as exec with no db.query.text, so such a rule no longer matches. The outcome becomes sample_rate. The count of dropped spans stays the same, but the reason changes. Rules that match on op: 'db.query' still match, because SPAN_ATTRIBUTES has the op. (Verified this by looking at the code in core, not with a test, so it's possible I'm getting it twisted somehow.)
Suggestion: accept it, but state it in the PR body. Or, if the difference matters, take the full path when getClient()?.getOptions().ignoreSpans?.length is set and span streaming is on. That costs one more options read per query. Personally, I'd just accept the difference and document it.
Co-authored-by: isaacs <i@izs.me>
…cannot be sent Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| function childSpanWillNotBeRecorded(): boolean { | ||
| if (!hasSpansEnabled()) { | ||
| return true; | ||
| } | ||
|
|
||
| const activeSpan = getActiveSpan(); | ||
| return !!activeSpan && !spanIsSampled(activeSpan); | ||
| } |
There was a problem hiding this comment.
Bug: The childSpanWillNotBeRecorded function fails to check for suppressTracing, causing a performance optimization for SQL instrumentation to be skipped unnecessarily.
Severity: LOW
Suggested Fix
Update childSpanWillNotBeRecorded to check if tracing is suppressed by calling isTracingSuppressed(). If it returns true, the function should also return true. This will require importing isTracingSuppressed from @sentry/core.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cloudflare/src/instrumentations/instrumentSqlStorage.ts#L84-L91
Potential issue: The function `childSpanWillNotBeRecorded` in the Cloudflare SQL
instrumentation incorrectly determines if a new span will be recorded. When a SQL query
is executed within a `suppressTracing` block, the core SDK correctly creates a
non-recording span. However, `childSpanWillNotBeRecorded` does not check for this
suppressed state and wrongly concludes that a span will be recorded. This failure
prevents a performance optimization from being applied, leading to the unnecessary
execution of expensive functions like `sanitizeSqlQuery` and `getSqlQuerySummary`.
Did we get this right? 👍 / 👎 to inform future reviews.
4 participants
The Durable Object SQL instrumentation sanitized and summarized each query, also when the span could not be sent: tracing not configured, or an unsampled parent. On SQL-heavy Durable Objects, this was a large CPU cost with
tracesSampleRate: 0.In these cases, the query is not sanitized now.
startSpanstill runs with the nameexecand the static attributes, so an unsampled span still records itssample_rateoutcome. A query that can target acf_table takes the full path, because an internal query must not start a span.Under span streaming with an unsampled parent, an
ignoreSpansrule that matches the span name ordb.query.textdoes not match now. Thus the dropped span is reported assample_rate, notignored. The number of dropped spans does not change. Rules that matchop: 'db.query'are not affected.