fix(spring-jakarta): [Queue Instrumentation 11] Guard entire span lifecycle in Kafka producer interceptor#5281
Conversation
…terceptor Wrap all span operations (startChild, setData, injectHeaders, finish) in a single try-catch so instrumentation can never break the customer's Kafka send. The record is always returned regardless of any exception in Sentry code. Co-Authored-By: Claude <noreply@anthropic.com>
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
Sentry Build Distribution
|
Performance metrics 🚀
|
| span.finish(); | ||
| } catch (Throwable ignored) { | ||
| // Header injection must not break the send | ||
| // Instrumentation must never break the customer's Kafka send |
There was a problem hiding this comment.
Same as in the original, should we log anything here?
![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 is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6d91bdc. Configure here.
|
|
||
| span.setStatus(SpanStatus.OK); | ||
| span.finish(); | ||
|
|
There was a problem hiding this comment.
Span leaks unfinished when exception occurs after creation
Medium Severity
If an exception is thrown by setData(), injectHeaders(), or setStatus() after startChild() successfully creates a span, the catch block swallows the exception but never calls span.finish(). This leaves an orphaned, unfinished span. This is a regression from the previous code where injectHeaders failures were caught independently and span.finish() still ran. The pattern used elsewhere in this codebase (e.g., SentryCacheWrapper) uses finally { span.finish() } to guarantee cleanup.
Reviewed by Cursor Bugbot for commit 6d91bdc. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return record; | ||
| } | ||
|
|
||
| span.setData(SpanDataConvention.MESSAGING_SYSTEM, "kafka"); | ||
| span.setData(SpanDataConvention.MESSAGING_DESTINATION_NAME, record.topic()); | ||
| span.setData(SpanDataConvention.MESSAGING_SYSTEM, "kafka"); | ||
| span.setData(SpanDataConvention.MESSAGING_DESTINATION_NAME, record.topic()); | ||
|
|
||
| try { | ||
| injectHeaders(record.headers(), span); | ||
|
|
||
| span.setStatus(SpanStatus.OK); | ||
| span.finish(); | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Bug: If injectHeaders() throws an exception in onSend(), the created span is never finished, causing a span leak. The catch block bypasses the span.finish() call.
Severity: MEDIUM
Suggested Fix
Ensure span.finish() is always called after a span is created, even if an exception occurs. This can be done by moving the span creation and finishing logic into a try-finally block, ensuring span.finish() is called in the finally section.
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:
sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/kafka/SentryProducerInterceptor.java#L56-L72
Potential issue: In `SentryProducerInterceptor.onSend()`, the refactored exception
handling introduces a potential span leak. The `span.finish()` call is now inside a
`try` block. If `injectHeaders()` throws an exception, control will jump to the `catch`
block, skipping the `span.finish()` call. The `catch` block does not finish the span,
causing it to remain unfinished in the parent transaction's span list. This is a
regression from the previous code, which correctly ensured the span was always finished
by placing the call outside the `try` block for header injection.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
PR Stack (Queue Instrumentation)
📜 Description
Wrap the entire span lifecycle in
SentryProducerInterceptor.onSend()in a single try-catch so instrumentation can never break the customer's Kafka send. TheProducerRecordis always returned regardless of any exception in Sentry code.Previously, only header injection was guarded. Span creation (
startChild),setData(), andfinish()were unguarded — any exception from these would propagate and fail the customer's Kafka send per theProducerInterceptor.onSend()contract.The inner try-catch around
injectHeadersis removed since the outer catch now covers it.💡 Motivation and Context
Kafka's
ProducerInterceptor.onSend()contract requires that interceptors not throw exceptions — any exception propagates and fails the send. Observability code must never cause customer message loss.💚 How did you test it?
SentryProducerInterceptorTesttests pass📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Nothing.
#skip-changelog