Otel suggestions - #1982
Merged
lukebakken merged 4 commits intoJul 30, 2026
Merged
Conversation
When the send throws and publisher confirmations are enabled, MaybeHandleExceptionWithEnabledPublisherConfirmations faults the confirm TCS with the exception and the finally's confirmation await re-raises that same instance. The send catch and the finally's catch therefore both called SetActivityError on the same exception, so a publish on a closed connection recorded AlreadyClosedException twice. Track the exception recorded by the send catch and gate the finally's catch on a ReferenceEquals filter, so the same instance is recorded at most once while a distinct exception from the confirmation await is still captured. Filtering on reference equality is safe here: the TCS is faulted with the original exception object and awaited via ExceptionDispatchInfo, so the re-raised instance is the one stored. The conservative failure mode is a missed dedupe (two events), never a dropped real error.
error.type was gated behind IsAllDataRequested while the exception event and Error status were set unconditionally. With a listener sampling only PropagationData (IsAllDataRequested == false) the span received the exception event and the Error status but not error.type - the expensive signals recorded and the cheap string tag dropped. error.type is a single SetTag of a fully-qualified type name, so it is no more expensive than the signals already firing; gate them together or not at all. Set it unconditionally so a sampled failure still reports the one Stable messaging attribute. See issue rabbitmq#1967.
A caller cancelling its own token is not a messaging or connection failure, but the error paths recorded OperationCanceledException on the span all the same: the publish span (now widened to cover the confirmation await, so the finally's await surfaces it), the connection span, and each tcp-connection-attempt span. That inflated error rates for graceful shutdown and client-side timeouts. Use exception filters (when cancellationToken.IsCancellationRequested) on OperationCanceledException so caller cancellation rethrows without being recorded, while genuine timeouts and broker failures still set all three error signals. The publish send-catch uses an inline guard rather than a filter because confirmation tracking cleanup (faulting the TCS, decrementing the sequence number) must still run on cancellation, and a filter that skipped the catch would skip that cleanup too. See issue rabbitmq#1967.
HasRecordedException read Events.First(), so it could not detect a span that recorded the same exception twice - exactly the duplicate the publish failure path produced. Filter to exception events and assert exactly one, so any future duplicate recording fails the test. The ambient-pollution test set VerifyParent = false on all recorders, leaving parenting unasserted. The publish it performs runs after the app activity's scope has ended, so its parent is root; assert that, so a parenting regression on the frame-writing path (the same path the ownership fix touches) would surface here rather than only in TestOpenTelemetry. See issue rabbitmq#1967.
Collaborator
Author
|
These are also genie findings that I manually reviewed with the best of my knowledge |
Collaborator
|
Thank you @danielmarbach!! |
Collaborator
|
Merged into fix/gh-1967-opentelemetry-tracing (#1978) via a real merge, so your four commits keep their authorship and history. Kept your cancellation fix (including the extension to connection open and per-endpoint attempts), the filter-based dedup, the unconditional One thing I kept from #1978: the ambient-activity test there publishes inside the live app-activity scope, so the assertion is Also added a regression test for the cancellation fix, which landed without one. CI is green across all ten checks. Closing in favour of #1978. Thanks for the careful review, Daniel. |
lukebakken
added a commit
that referenced
this pull request
Jul 31, 2026
Rebasing this branch onto main dropped the merge commit that had reconciled the #1982 commits with the review-feedback commit that preceded them, so git replayed both sides independently and silently undid two fixes. Daniel's "Apply all three SetActivityError signals consistently" is comment-only: it was written against a parent where the guard was still `activity is null`, so replaying it onto the narrowed guard left a commit whose message says one thing and whose code does the opposite, with two contradictory comment blocks stacked on the helper. Restore the guard to `activity is null` and fold the two blocks into one. A span sampled as PropagationData is still delivered to ActivityStopped with its tags and exception events intact, so gating error.type alone recorded the expensive signals and dropped the only Stable attribute in the messaging convention. The review doc had likewise reverted to describing a HasRecordedExceptionOnce helper that no longer exists, and to the superseded claim that all three signals sit behind one IsAllDataRequested test. Both passages now match the code. No behaviour change relative to the branch before the rebase: this only undoes what the rebase undid.
lukebakken
added a commit
that referenced
this pull request
Jul 31, 2026
Rebasing this branch onto main dropped the merge commit that had reconciled the #1982 commits with the review-feedback commit that preceded them, so git replayed both sides independently and silently undid two fixes. Daniel's "Apply all three SetActivityError signals consistently" is comment-only: it was written against a parent where the guard was still `activity is null`, so replaying it onto the narrowed guard left a commit whose message says one thing and whose code does the opposite, with two contradictory comment blocks stacked on the helper. Restore the guard to `activity is null` and fold the two blocks into one. A span sampled as PropagationData is still delivered to ActivityStopped with its tags and exception events intact, so gating error.type alone recorded the expensive signals and dropped the only Stable attribute in the messaging convention. The review doc had likewise reverted to describing a HasRecordedExceptionOnce helper that no longer exists, and to the superseded claim that all three signals sit behind one IsAllDataRequested test. Both passages now match the code. No behaviour change relative to the branch before the rebase: this only undoes what the rebase undid.
lukebakken
added a commit
that referenced
this pull request
Jul 31, 2026
Rebasing this branch onto main dropped the merge commit that had reconciled the #1982 commits with the review-feedback commit that preceded them, so git replayed both sides independently and silently undid two fixes. Daniel's "Apply all three SetActivityError signals consistently" is comment-only: it was written against a parent where the guard was still `activity is null`, so replaying it onto the narrowed guard left a commit whose message says one thing and whose code does the opposite, with two contradictory comment blocks stacked on the helper. Restore the guard to `activity is null` and fold the two blocks into one. A span sampled as PropagationData is still delivered to ActivityStopped with its tags and exception events intact, so gating error.type alone recorded the expensive signals and dropped the only Stable attribute in the messaging convention. The review doc had likewise reverted to describing a HasRecordedExceptionOnce helper that no longer exists, and to the superseded claim that all three signals sit behind one IsAllDataRequested test. Both passages now match the code. No behaviour change relative to the branch before the rebase: this only undoes what the rebase undid.
lukebakken
added a commit
that referenced
this pull request
Aug 3, 2026
Rebasing this branch onto main dropped the merge commit that had reconciled the #1982 commits with the review-feedback commit that preceded them, so git replayed both sides independently and silently undid two fixes. Daniel's "Apply all three SetActivityError signals consistently" is comment-only: it was written against a parent where the guard was still `activity is null`, so replaying it onto the narrowed guard left a commit whose message says one thing and whose code does the opposite, with two contradictory comment blocks stacked on the helper. Restore the guard to `activity is null` and fold the two blocks into one. A span sampled as PropagationData is still delivered to ActivityStopped with its tags and exception events intact, so gating error.type alone recorded the expensive signals and dropped the only Stable attribute in the messaging convention. The review doc had likewise reverted to describing a HasRecordedExceptionOnce helper that no longer exists, and to the superseded claim that all three signals sit behind one IsAllDataRequested test. Both passages now match the code. No behaviour change relative to the branch before the rebase: this only undoes what the rebase undid.
lukebakken
added a commit
that referenced
this pull request
Aug 13, 2026
Rebasing this branch onto main dropped the merge commit that had reconciled the #1982 commits with the review-feedback commit that preceded them, so git replayed both sides independently and silently undid two fixes. Daniel's "Apply all three SetActivityError signals consistently" is comment-only: it was written against a parent where the guard was still `activity is null`, so replaying it onto the narrowed guard left a commit whose message says one thing and whose code does the opposite, with two contradictory comment blocks stacked on the helper. Restore the guard to `activity is null` and fold the two blocks into one. A span sampled as PropagationData is still delivered to ActivityStopped with its tags and exception events intact, so gating error.type alone recorded the expensive signals and dropped the only Stable attribute in the messaging convention. The review doc had likewise reverted to describing a HasRecordedExceptionOnce helper that no longer exists, and to the superseded claim that all three signals sit behind one IsAllDataRequested test. Both passages now match the code. No behaviour change relative to the branch before the rebase: this only undoes what the rebase undid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Please describe the big picture of your changes here to communicate to the
RabbitMQ team why we should accept this pull request. If it fixes a bug or
resolves a feature request, be sure to link to that issue.
A pull request that doesn't explain why the change was made has a much
lower chance of being accepted.
If English isn't your first language, don't worry about it and try to
communicate the problem you are trying to solve to the best of your abilities.
As long as we can understand the intent, it's all good.
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.
CONTRIBUTING.mddocumentFurther Comments
If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.