-
-
Notifications
You must be signed in to change notification settings - Fork 467
fix(spring-jakarta): [Queue Instrumentation 11] Guard entire span lifecycle in Kafka producer interceptor #5281
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
base: fix/queue-instrumentation-root-scopes
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,25 +53,26 @@ public SentryProducerInterceptor(final @NotNull IScopes scopes) { | |
| return record; | ||
| } | ||
|
|
||
| final @NotNull SpanOptions spanOptions = new SpanOptions(); | ||
| spanOptions.setOrigin(TRACE_ORIGIN); | ||
| final @NotNull ISpan span = activeSpan.startChild("queue.publish", record.topic(), spanOptions); | ||
| if (span.isNoOp()) { | ||
| return record; | ||
| } | ||
| try { | ||
| final @NotNull SpanOptions spanOptions = new SpanOptions(); | ||
| spanOptions.setOrigin(TRACE_ORIGIN); | ||
| final @NotNull ISpan span = | ||
| activeSpan.startChild("queue.publish", record.topic(), spanOptions); | ||
| if (span.isNoOp()) { | ||
| 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) { | ||
| // Header injection must not break the send | ||
| // Instrumentation must never break the customer's Kafka send | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as in the original, should we log anything here? |
||
| } | ||
|
|
||
| span.setStatus(SpanStatus.OK); | ||
| span.finish(); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Span leaks unfinished when exception occurs after creationMedium Severity If an exception is thrown by Reviewed by Cursor Bugbot for commit 6d91bdc. Configure here. |
||
| return record; | ||
| } | ||
|
|
||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: If
injectHeaders()throws an exception inonSend(), the created span is never finished, causing a span leak. Thecatchblock bypasses thespan.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 atry-finallyblock, ensuringspan.finish()is called in thefinallysection.Prompt for AI Agent
Did we get this right? ๐ / ๐ to inform future reviews.