Fix the behavioural defects found in the OpenTelemetry tracing review - #1978
Fix the behavioural defects found in the OpenTelemetry tracing review#1978lukebakken wants to merge 13 commits into
Conversation
|
A few minor suggestions with additional context in the commit messages #1982 |
|
I wanted to follow up on judgment call 17f82c5 from the review notes, the one about marking the span as Error even when the exception is handled by publisher confirmations. I compared this against how ASP.NET Core handles the same question in its hosting layer. Their That approach is simpler, but it comes with a cost. Every cancelled request, every client disconnect, every graceful shutdown creates an Error span. That noise makes it harder to spot the signals you actually care about. The RabbitMQ code separates caller-initiated cancellation from everything else. When the caller cancels, the span stays clean. When the operation genuinely fails, the span gets the Error status. That distinction is possible here because the code has access to the CancellationToken and can check I think the RabbitMQ direction is the right one. The library has the context to make the call, and it uses it. The only thing I would suggest is making sure this is called out in the release notes, so consumers know that cancelled operations will no longer produce Error spans after upgrading. That is a visible change if someone has dashboards or alerts keyed on error-type counts. |
88789f9 to
15ab647
Compare
|
Thanks, that framing is more useful than the one I had. I was treating "do not mark caller cancellation as an error" as self-evidently correct; the ASP.NET Core comparison makes clear it is a choice that depends on having the token, and that the simpler unconditional approach is a defensible default when you do not. Worth separating the two judgment calls, since the commit you linked (17f82c5) is the cancellation one but your opening line describes the other:
Both stand as-is. Added a For the 7.3.0 release notes section to the PR description covering the three visible behaviour changes: the ~10x publish span duration increase, cancelled operations no longer producing
|
f51c5b6 to
e432da1
Compare
| * a filter that skipped this catch would skip the cleanup too. | ||
| * See issue #1967. | ||
| */ | ||
| bool isCallerCancellation = |
There was a problem hiding this comment.
Why a separate local instead of having the condition in the if directly?
Same below.
|
I would like to provide a piece of context regarding exposing exception details via OpenTelemetry. This is somewhat orthogonal to the design decisions mentioned; however, it could influence the concrete implementation approach. OpenTelemetry is in the process of deprecating the Span Event API in favour of the Logs API. In other words, it is assumed that sooner or later all Events API calls will be migrated to the Logs API calls. The change assumes this process will take some time and provides explicit guidance for instrumentation authors. Span.AddException eventIn the context of exceptions (as of 3.08.2026), semantic conventions for exceptions (spans) that describe capturing exception details as events on a span have already been deprecated. The spec defines both a new, preferred approach and a migration strategy for existing instrumentations (repeating the general migration guidance for instrumentation authors). In short, the recommendation says (this is my simplification): instead of an event on a span, exception details (type, message, and details) should be captured as a RabbitMQ ClientIn terms of implementation, this has at least two potential implementation consequences:
|
e432da1 to
f57bbb0
Compare
|
Thanks @tmasternak for your input. Personally, I know next-to-nothing about OTel. Do you have a client library or libraries that the 🧞 (AI) and I can use as a good, modern reference? It's either that or I depend on the goodwill and free time of people like you to get this work shipped, or I yolo-it and hope for the best 😸 Thanks! |
|
@lukebakken @tmasternak and I collaborated on the Otel skill I sent you together with Mauro who was the original author. We tried to incorporate as much as possible the spec as well as our practices. We will continue to refine it as we go. It should get you and the genie pretty far. The log integration change probably requires deferral because, as far as I understand, it would require hooking up ILogFactory somehow to the client, and that most likely requires a bit more thinking given it is a new dependency too. |
|
@danielmarbach thanks for reminding me of that. I didn't put 2 + 2 together, obviously 😅 |
|
Note Written by Claude (Anthropic's Claude Code), directed and approved by @lukebakken. The specification findings are @tmasternak's; the verification, and the error it turned up, are Claude's. Thanks @tmasternak. I went through the raw specification markdown rather than the rendered pages, and your points hold. Checking them also turned up an error of mine in this PR, which is now fixed. Your claims, confirmed
Also worth noting for the record: the six-month dual-emission maintenance commitment is a release-planning constraint, not just an implementation detail. Where I was wrongTwo things, both mine. " I had also been reading the wrong document. I thought the spec said nothing about span status. It does, in
One place the spec cuts against usSince I am citing
Our behaviour is that a publish whose failure is reported through publisher confirmations still marks the span Conversely, judgment call 1 gets independent support from your document. Why 7.3.0 still ships span eventsThe guidance for stable major versions is to stay behaviorally compatible for now, with migration planned for the next major. Note the rc version of The .NET ecosystem has also not moved, verified rather than assumed:
Your second consequence is a real defectThe logging-in-span-context point is the one with a concrete consequence here, and it is worse than hypothetical.
In Costs nothing today, because the span event goes on inside the scope. But an application logging from its On the seam: All of this is now in #1992, and the review doc records the findings. @lukebakken on the reference-library question: there is no .NET instrumentation to copy yet. |
The comment on ErrorType, the SetActivityError doc comment, and the
tracing review doc all claimed error.type is the only Stable attribute
in the messaging convention. That is false: the producer-span table in
messaging-spans.md also marks server.address and server.port Stable,
and the consumer tables add network.peer.address and
network.peer.port. Every messaging.* attribute is Development; the
Stable ones in those tables are borrowed from other registries.
The conclusion the claim was supporting is unaffected, since
error.type is Stable, is Conditionally Required "if and only if the
messaging operation has failed", and was set nowhere before this
branch. The phrasing is narrowed to what is true, and the review doc
records the correction explicitly.
The doc comment also said nothing about where the three signals come
from, having been written against docs/exceptions/exceptions-spans.md,
which is deprecated. docs/general/recording-errors.md is the governing
document, and messaging-spans.md defers to it ("Span status SHOULD
follow the Recording Errors document"). It prescribes exactly what
SetActivityError does on an error: set the status code to Error, set
error.type, and set the status description to the exception message.
It also says the status code MUST be left unset when the operation
ended without errors, which is why the helper is only called from
failure paths. Both the comment and the doc now quote it.
Finally, note that AddException is the one signal of the three on a
deprecation path. The exceptions-on-spans convention is deprecated in
favour of recording exceptions as log records; the status and
error.type are unaffected. The review doc gains a section with the
verbatim migration block, the verified state of the .NET stack
(DiagnosticSource 9.0.4 marks none of AddException, AddEvent or
SetStatus obsolete; OTEL_SEMCONV_EXCEPTION_SIGNAL_OPT_IN appears in no
OpenTelemetry .NET assembly including OpenTelemetry.Api 1.17.0;
OpenTelemetry.Instrumentation.AspNetCore 1.17.0 still uses
Activity.AddException), and the two places the migration will be
difficult: the missing recording seam in the core, and the deliver
activity's scope closing before OnCallbackExceptionAsync reports the
same exception.
That second one is a real defect, verified with a probe: at the
reporting call site Activity.Current has reverted to the enclosing
activity and the deliver span is already stopped. It costs nothing
today because the span event is added inside the scope, and it becomes
a MUST violation once exceptions are log records. Deliberately not
fixed here; it is a scope change with no present-day symptom, so it
belongs with the migration and its own test.
The review doc also records that caller-initiated cancellation is
correctly not treated as an error, with the specification support that
neither reviewer cited at the time: exceptions-logs.md assigns DEBUG
severity to "exceptions that don't indicate an actual issue", its
worked example being a request cancelled on the client side.
The specification findings behind all of this were raised by
@tmasternak on #1978.
See #1967 and #1992.
|
Thinking out loud here: So the callback approach is interesting because effectively the consumer interface is such a callback mechanism if I'm not mistaken. |
Adds docs/internal/opentelemetry-tracing-review.md, the write-up for issue #1967. It is the audit half of that issue; the fixes follow separately. Every claim marked "verified" in the doc was settled by driving the real SDK pipeline against a live broker rather than by reading code, per the issue's own direction. Defects found: * Ambient-span pollution. The three Activity.Current reads in SessionBase and Connection do not check whether the ambient activity belongs to this library, and TransmitAsync is on the path of every AMQP method, so any RPC issued inside a caller's span writes ten tags onto it. This is pre-existing in 7.2.1, not a regression from the connection-tracing work. * No publisher or subscriber span ever records an error, because the catch blocks sit outside the activity's using scope. error.type, the only Stable attribute in the RabbitMQ convention, is set nowhere. * BasicGetEmpty hardcodes a destination name of amq.default, which is factually wrong rather than merely non-conforming. * messaging.rabbitmq.delivery_tag matches no registry attribute. * Tracing configuration is process-global and last-writer-wins across TracerProviders. Those members shipped in 7.2.1, so this one is a public API question. Semantic-convention gaps are recorded against the specification at main, including the two existing test assertions that currently lock in the receive span-kind mismatch. Also records what was checked and found clean, so it is not re-audited: context propagation against eight malformed-header cases, span parentage under ConsumerDispatchConcurrency, activity disposal, guard pairing, connection-span exception recording, API parity across target frameworks, and packaging. Documents the SetNetworkTags guard shape as well, since the missing publisher gate inside that helper looks like a bug and is not one.
Group A of issue #1967: the findings that are outright wrong behaviour rather than semantic-convention conformance. None of these changes an attribute value or span name that a conforming consumer keys off, so they ship ahead of the convention work (group B) and the public API question (group C), which stack on this branch. Ambient-span pollution. SessionBase.TransmitAsync and Connection.WriteAsync tagged whatever Activity.Current happened to be, because the frame-writing path has no reference to the publish activity it belongs to, and TransmitAsync is on the path of every AMQP method. Any RPC issued inside a caller's span wrote ten messaging and network tags onto a span this library does not own. Fixed with an ownership check, IsPublisherActivity, testing the publisher source specifically rather than "any activity from this library": the connection spans are ours too but are not publish operations, so a library-wide gate would have left "connection attempt" carrying messaging.message.envelope.size from the handshake frames. The two ambient call sites now read Activity.Current inside the helper, behind the cheap HasListeners() test, since that is an AsyncLocal read on a per-frame path. Failed operations never recorded an error. No publisher or subscriber span ever set a status, because the catch blocks sat outside the activity's using scope, so a mandatory publish that raised PublishReturnException and a consumer callback that threw on every message both traced as completely successful. Fixed via one SetActivityError helper setting all three of the exception event, an Error status, and error.type; a backend reads an unset status as success, so an exception event alone is not enough. error.type is the only Stable attribute in the RabbitMQ convention and was set nowhere. The publish case needed two catches, not one. PublishReturnException surfaces from MaybeEndPublisherConfirmationTrackingAsync, which runs in the finally because the confirmation is only awaited once the send has been issued, so moving the using out of the inner try and adding a catch there does not see it. That was the primary verified case, so the obvious fix would have looked complete and covered nothing. Moving the using also means the publish span now starts before flow control rather than after, so its duration includes any flow-control blocking. Nothing became slower, but publish-latency dashboards will shift. A publish failure that is handled by the confirmation mechanism now also marks the span Error, on the grounds that the publish did fail; revisit if that proves noisy. Smaller items: * OpenTcpConnection was the only activity factory returning a bare activity and leaving its call site to tag it, so it now takes the endpoint and sets the server tags itself. * OpenTelemetryContextExtractor relied on a blanket catch swallowing a NullReferenceException once per propagator field when Headers was null. It now returns early, like DefaultContextExtractor already did, and the catch is documented as defensive-only. * DefaultContextSetter's comment claimed it preserved an existing header; it always overwrote. The overwrite is correct, so the comment was replaced. Five regression tests, each of which fails without its fix. They go in SequentialIntegration because ActivityRecorder is process-global. Three of them need UseRoutingKeyAsOperationName pinned false, since it defaults true and the recorder matches span names exactly, so a recorder built for "publish" silently records nothing against a span named "publish <routing-key>". The new scope guard restores the prior value on dispose, unlike the existing tests, which mutate that global and leave it - group C's defect reproducing in our own suite. Verified against a live broker: 57 tracing tests, the full SequentialIntegration and Integration suites, and 127 unit tests, with netstandard2.0 and the OpenTelemetry package building clean.
Four items from a review of the group A branch itself. Three were verified with a probe against a live broker rather than by reading code. The two catches recorded one failure twice. Recording in both the inner catch and the one in the finally was necessary - the unroutable mandatory publish only surfaces from the finally - but it double-recorded on a more common path. Publishing on a closed connection with confirmations and tracking enabled throws from the send, the inner catch records it and hands it to the confirmation task so the publish counts as handled and does not rethrow there, and then the finally awaits that same task, which re-raises the identical instance through ExceptionDispatchInfo. Verified: events=2 with AlreadyClosedException twice, against events=1 for the unroutable mandatory case, where the exception originates in the confirmation await and the inner catch never sees it. Fixed by remembering what the inner catch recorded and comparing by reference in the finally. ReferenceEquals is the precise test here, since the TCS re-raises the same object, so a genuinely different exception from the confirmation await is still recorded. The tests could not have caught that, because HasRecordedException only inspects Events.First(). Added HasRecordedExceptionOnce, asserting Assert.Single over the exception events, and pointed RecordsFailure at it, so every failure test now asserts the count rather than just the first event. SetActivityError gated its three signals inconsistently. AddException and SetStatus were unconditional while error.type sat behind IsAllDataRequested. Verified with a listener sampling PropagationData: the span got the exception event and the Error status but no error.type. That inverts the cost - AddException allocates an ActivityEvent with a tag list, error.type is one string already in hand - so the expensive signal was recorded on spans the listener had asked not to fill in and the cheap one was dropped. All three now sit behind one IsAllDataRequested test. A span that is not AllData is not exported, so nothing observable is lost, and it keeps the allocation off the per-delivery consumer path when nothing is recording. Span parenting was unasserted; every new recorder sets VerifyParent = false. That is unavoidable through the recorder, because ExpectedParent has to be set before the recorder sees anything and the ambient activity does not exist that early, so the ambient test now asserts Assert.Same(appActivity, publishActivity.Parent) directly. Scoping the tags to the publisher source must not also detach the publish span from the caller's trace. A detached span would report a null Parent, so the assertion is not vacuous. The publish also moved inside the ambient scope, so the app's span is checked for stolen tags after a publish rather than only after RPCs. The null-Headers extractor guard had no test. Worth being explicit that TestContextExtractorHandlesPropertiesWithNoHeaders_GH1967 would also have passed before that fix, since swallowing the NullReferenceException reached the same result. It pins the observable contract - no headers extracts to default, without throwing - so the blanket catch can later be narrowed or removed safely, which is what the fix was for. The review also measured the publish span's duration change, which the earlier write-up understated as "includes any flow-control blocking". The earlier start is the minor half; the dominant effect is that the finally, where the confirmation is awaited, is now inside the activity's scope. Over 300 warm iterations with confirmations and tracking enabled the span went from p50 37us to p50 372us, from 4.7% of the wall-clock publish call to 96% of it. Nothing became slower, but that is roughly an order of magnitude more reported duration, and it belongs in the release notes rather than in an aside. The doc and the PR body now carry the measurements. Verified against a live broker: 59 tracing tests, up from 57, the full SequentialIntegration suite at 71 passed, the Integration suite at 193, and 127 unit tests. The duplicate-event test was confirmed to fail without its fix. netstandard2.0 and the OpenTelemetry package build clean.
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 #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 #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 #1967.
The merged fix stops recording a caller-cancelled publish as a failure, but shipped without a regression test. Add one. Blocking the connection holds off the broker confirmation, so a publish with confirmations enabled parks in the confirmation await that BasicPublishCoreAsync runs in its finally, after the send activity is created. Cancelling the token throws OperationCanceledException from that await deterministically, with no dependence on broker timing. The test asserts the publish span is not marked Error, carries no exception event, and has no error.type. Removing the finally's cancellation guard fails it with an Error status on the span.
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.
The comment on ErrorType, the SetActivityError doc comment, and the
tracing review doc all claimed error.type is the only Stable attribute
in the messaging convention. That is false: the producer-span table in
messaging-spans.md also marks server.address and server.port Stable,
and the consumer tables add network.peer.address and
network.peer.port. Every messaging.* attribute is Development; the
Stable ones in those tables are borrowed from other registries.
The conclusion the claim was supporting is unaffected, since
error.type is Stable, is Conditionally Required "if and only if the
messaging operation has failed", and was set nowhere before this
branch. The phrasing is narrowed to what is true, and the review doc
records the correction explicitly.
The doc comment also said nothing about where the three signals come
from, having been written against docs/exceptions/exceptions-spans.md,
which is deprecated. docs/general/recording-errors.md is the governing
document, and messaging-spans.md defers to it ("Span status SHOULD
follow the Recording Errors document"). It prescribes exactly what
SetActivityError does on an error: set the status code to Error, set
error.type, and set the status description to the exception message.
It also says the status code MUST be left unset when the operation
ended without errors, which is why the helper is only called from
failure paths. Both the comment and the doc now quote it.
Finally, note that AddException is the one signal of the three on a
deprecation path. The exceptions-on-spans convention is deprecated in
favour of recording exceptions as log records; the status and
error.type are unaffected. The review doc gains a section with the
verbatim migration block, the verified state of the .NET stack
(DiagnosticSource 9.0.4 marks none of AddException, AddEvent or
SetStatus obsolete; OTEL_SEMCONV_EXCEPTION_SIGNAL_OPT_IN appears in no
OpenTelemetry .NET assembly including OpenTelemetry.Api 1.17.0;
OpenTelemetry.Instrumentation.AspNetCore 1.17.0 still uses
Activity.AddException), and the two places the migration will be
difficult: the missing recording seam in the core, and the deliver
activity's scope closing before OnCallbackExceptionAsync reports the
same exception.
That second one is a real defect, verified with a probe: at the
reporting call site Activity.Current has reverted to the enclosing
activity and the deliver span is already stopped. It costs nothing
today because the span event is added inside the scope, and it becomes
a MUST violation once exceptions are log records. Deliberately not
fixed here; it is a scope change with no present-day symptom, so it
belongs with the migration and its own test.
The review doc also records that caller-initiated cancellation is
correctly not treated as an error, with the specification support that
neither reviewer cited at the time: exceptions-logs.md assigns DEBUG
severity to "exceptions that don't indicate an actual issue", its
worked example being a request cancelled on the client side.
The specification findings behind all of this were raised by
@tmasternak on #1978.
See #1967 and #1992.
bf06375 to
0cee52b
Compare
Two points from the OpenTelemetry review read as issues at a glance and invite a future reviewer to re-open them, so pin the rationale where the code lives. In `BasicPublishCoreAsync`, recording `Error` when `MaybeHandleExceptionWithEnabledPublisherConfirmations` returns `true` looks like the spec's "do not record handled or retried errors" case. It is not: "handled" means the exception was routed onto the confirmation task, not swallowed, and the `finally` re-raises it to the caller. Every path that records `Error` is one the caller observes as a failure. In `SetActivityError`, firing the allocating `AddException` even on a PropagationData-sampled span looks like a missing `IsAllDataRequested` guard. It is deliberate: failure paths are not hot, and keeping the three signals together makes the eventual logs migration a single edit. Tighten the same handled-exception argument in the review doc, correcting "swallows" to the routed-through-confirmations behaviour.
The generated CHANGELOG.md and HISTORY.md carry only the mechanical PR and issue list, so user-visible behaviour notes have no home in them and get reconstructed from memory at release time, if at all. Add a step to the main (7.x) release process that sweeps merged PRs in the milestone labeled `release-notes` and folds each PR's release-note section into the GitHub Release body, which is where upgraders actually read them.
|
@danielmarbach I pointed the OTel skills at this PR and followed the guidance. You can see that this PR is only part of #1979, so I feel OK merging it. Thoughts? |
Note
Claude wrote the code, tests, and documentation in this PR, except for the commits merged from #1982, which @danielmarbach wrote. I directed the work, reviewed the findings, ratified the three judgment calls flagged below, and requested the code review that produced the follow-ups section.
Group A of #1967: the findings that are outright wrong behaviour rather than semantic-convention conformance.
The audit itself is in
docs/internal/opentelemetry-tracing-review.md(first commit on this branch, already reviewed as part of #1967). Every claim marked verified there was settled by driving the real SDK pipeline against a live broker rather than by reading code.Why group A ships on its own
None of these changes an attribute value or span name that a conforming consumer keys off, so they can land without anyone downstream having to re-key. Groups B (semantic-convention conformance, which breaks four existing test assertions) and C (per-provider tracing configuration, which must land before #1923) will stack on this branch as separate PRs.
One caveat on that: the publish span's duration does change substantially, as measured below. No attribute or name changes, but latency dashboards keyed on that span will move.
Ambient-span pollution
SessionBase.TransmitAsyncandConnection.WriteAsynctagged whateverActivity.Currenthappened to be, because the frame-writing path has no reference to the publish activity it belongs to.TransmitAsyncis on the path of every AMQP method, so any RPC issued inside a caller's span wrote ten messaging and network tags onto a span this library does not own - an application's own span, or an ASP.NET request span, picking upserver.addressandmessaging.message.envelope.sizefrom an incidentalQueueDeclare.Pre-existing in 7.2.1, not a regression from the connection-tracing work.
Fixed with an ownership check that tests the publisher source specifically, not "any activity from this library". That distinction is load-bearing: the connection spans are ours too, but they are not publish operations, so a library-wide gate would have left
connection attemptstill carryingmessaging.message.envelope.sizefrom the handshake frames.The two ambient call sites now read
Activity.Currentinside the helper, behind the cheapHasListeners()test, since that is anAsyncLocalread on a per-frame path.SessionBaseactually improves here - it previously readActivity.Currentunconditionally to pass as an argument.Failed operations never recorded an error
No publisher or subscriber span ever set a status, because the
catchblocks sat outside the activity'susingscope. A mandatory publish raisingPublishReturnException, and a consumer callback throwing on every message, both traced as completely successful.Fixed via one
SetActivityErrorhelper setting all three of the exception event, anErrorstatus, anderror.type. This is what the spec prescribes, not just a defensible choice:docs/general/recording-errors.md, whichmessaging-spans.mddefers to ("Span status SHOULD follow the Recording Errors document"), says the status code MUST be left unset on success, and that on error instrumentation SHOULD set the status toError, SHOULD seterror.type, and SHOULD set the status description to the exception message.SetActivityErrordoes exactly those three.error.typeis Stable in the messaging convention, isConditionally Requiredon failure, and was set nowhere before this PR. The helper also replaced five hand-rolledAddException+SetStatuspairs on the connection spans, so all three span types now report failures uniformly.The publish case needed two catches, not one.
PublishReturnExceptionsurfaces fromMaybeEndPublisherConfirmationTrackingAsync, which runs in thefinallybecause the confirmation is only awaited once the send has been issued. Moving theusingout of the innertryand adding acatchthere does not see it. That was the primary verified case, so the obvious fix would have looked complete and covered nothing.Three judgment calls
Called out explicitly because they are choices, not defects:
1. The publish span now covers the full publish-and-confirm round trip. It starts above
MaybeStartPublisherConfirmationTrackingandMaybeEnforceFlowControlAsync, and - because the confirmation is awaited in thefinally, which is now inside the activity's scope - it also ends after that await returns. That second half is the dominant effect. Measured over 300 warm iterations with publisher confirmations and tracking enabled:mainThe span went from ~5% of the wall-clock publish call to ~96% of it, about a 10x increase in reported duration. Nothing became slower: wall-clock publish time was the same order in both runs (p50 776us on
main, 388us on this branch), and that gap is run-to-run noise between two separate probe processes, not a speedup. What changed is what the span means - "publish, including waiting for the broker to confirm" rather than "hand the frames to the socket". Publish-latency dashboards keyed on this span will jump by roughly an order of magnitude on upgrade to 7.3.0.I think end-to-end is the more useful semantic, and it is what a consumer would expect a
publishspan to cover when confirmations are enabled. But it is a visible change, so it needs to be in the release notes and not buried as a flow-control aside.2. A handled exception still marks the span
Error. IfMaybeHandleExceptionWithEnabledPublisherConfirmationsswallows,SetActivityErrorhas already run. The publish did fail, it was just reported through confirmations rather than by throwing.There is spec text cutting against this, and it deserves to be argued rather than left unmentioned.
recording-errors.md: "Errors that were retried or handled (allowing an operation to complete gracefully) SHOULD NOT be recorded on spans or metrics that describe this operation." The deprecation ofexception.escapedrests on the same reasoning. The argument for keeping theErrorstatus is the parenthetical: the operation did not complete gracefully. A nacked or returned publish is a failed publish, and the caller learns about it through the confirmation task instead of a throw. That is unlike the spec's own example, whereResourceAlreadyExistsExceptionmeanscreateIfNotExistssucceeded in its contract. So this is a judgment call against soft guidance rather than a conformance defect, and worth revisiting if it proves noisy. Flagging it explicitly because it is the one place in this PR where the spec's default and our behaviour differ.3. Two exception events if both the
tryand thefinallythrow. Accurate, but noisier than one. This turned out to reproduce on a common path rather than an exotic one, and is fixed - see the follow-ups below.For the 7.3.0 release notes
Three behaviour changes here are visible to anyone with dashboards or alerts on this client's spans. Collecting them in one place so they do not have to be reconstructed from the sections above at release time.
publishspan will move.Errorspans. A publish or connection open that the caller cancels rethrows without setting a status, exception event, orerror.type. Anything keyed onerror.typecounts will see a drop that reflects reporting, not reliability - this matters most for apps that cancel routinely on graceful shutdown or client-side timeouts. The spec supports this independently of the ASP.NET Core comparison that prompted it:exceptions-logs.mdassigns DEBUG severity to "exceptions that don't indicate an actual issue", and its worked example is a request "cancelled on the client side".Errorspans at all, which is the flip side: publisher and subscriber spans previously never set a status, so a nacked publish or a throwing consumer traced as a success. Error rates keyed on these spans go from zero to real.Thanks @danielmarbach for pushing on the second one - the ASP.NET Core comparison in this comment is the clearest statement of why the distinction is worth making when the library has the token to make it.
Smaller items
OpenTcpConnectionwas the only activity factory returning a bare activity and leaving its call site to tag it, which made it the one place a new caller could silently produce an untagged span. It now takes the endpoint and sets the server tags itself.OpenTelemetryContextExtractorrelied on a blanketcatchswallowing aNullReferenceExceptiononce per propagator field whenHeaderswas null. It now returns early, asDefaultContextExtractoralready did, and thecatchis documented as defensive-only. This is the weakest finding in the set.DefaultContextSetter's comment claimed it preserved an existing header; it always overwrote. The overwrite is correct, so the comment was fixed.Tests
Eight regression tests, each verified to fail without its fix - I removed the fixes and confirmed the failures rather than assuming. Seven are in
TestActivitySourceand one inTestOpenTelemetry. They live inSequentialIntegrationbecauseActivityRecorderand the activity sources are process-global.Worth knowing for anyone writing tracing tests:
UseRoutingKeyAsOperationNamedefaults totrue, so a publish span is namedpublish <routing-key>, andActivityRecordermatchesOperationNameexactly. A recorder built for"publish"therefore records zero activities under the default configuration and fails withExpected: 1 / Actual: 0, with no hint that the name is the problem. Three of the tests hit this. The newPlainOperationNamesscope guard pins the flag and restores the prior value on dispose - unlike the existing tests, which mutate that global and leave it, making outcomes order-dependent. That is group C's defect reproducing inside our own test suite, which is a fair argument for fixing it.Verified against a live broker on Linux, with this branch rebased onto
mainat #1971:TestActivitySource+TestOpenTelemetry)SequentialIntegration(full)IntegrationUnitnet8.0,netstandard2.0, and the OpenTelemetry package all build with zero warnings.I flagged one unverified risk when this went up as a draft: netstandard2.0 is built but not run locally, and
Activity.AddExceptioncomes from the DiagnosticSource package on that target, while this PR increases its call sites to include the per-delivery path. CI has since come back green on all 10 checks, includingbuild-win32andsequential-integration-win32(which is where the new tracing tests run), so that risk is closed.Review follow-ups
All four addressed; out of draft. A review pass over the branch turned these up, three of them verified with a probe against a live broker:
catchand thefinally'scatchcalledSetActivityError, becauseMaybeHandleExceptionstores the exception on the confirmation TCS and thefinally's await then re-raises it. Publishing on a closed connection yieldedevents=2withAlreadyClosedExceptionrecorded twice; an unroutable mandatory publish correctly yieldedevents=1. Judgment call 3 above, but on a common path rather than an exotic one. Fixed with an exception filter comparing by reference against the exception the inner catch recorded, so the duplicate is suppressed while a genuinely different exception from the confirmation await is still recorded. The tests missed it becauseActivityAssert.HasRecordedExceptiononly inspectedEvents.First(); it now assertsAssert.Singleover the"exception"events, so all the failure tests assert the count.SetActivityErrorapplied its three signals inconsistently under sampling.AddExceptionandSetStatuswere unconditional whileerror.typesat behindIsAllDataRequested. With a listener samplingPropagationData, the span got the exception event and theErrorstatus but noerror.type- aPropagationDataspan is still delivered toActivityStoppedwith its tags and exception events intact, so the gate recorded the expensive signals and dropped the cheap one. Fixed by making all three unconditional, gated only on a null activity.OpenTelemetryContextExtractornull-Headersguard. Fixed:TestContextExtractorHandlesPropertiesWithNoHeaders_GH1967.VerifyParent = false, so span parenting was unasserted. Fine for the failure-recording tests, but the ambient-activity test is exactly where a parenting regression would show up. Fixed: that test now asserts the publish span is a child of the caller's activity, so scoping the tags to the publisher source cannot silently detach the span from the caller's trace.Review notes
Two of the three judgment calls stand as choices: the span now covering the confirmation round trip, and
Errorstatus on a confirmation-handled failure. @lukebakken has ratified both, and they are the parts most worth a second opinion - particularly the first, given the size of the duration change. @danielmarbach has since reviewed the second and agrees with the direction. The third (duplicate exception events) is fixed.@danielmarbach independently reviewed the same fixes and opened #1982; those commits are merged into this branch, so the caller-cancellation fix, the filter-based dedup, the unconditional
SetActivityError, and the strengthenedHasRecordedExceptionare all his. My gating oferror.typebehindIsAllDataRequestedwas wrong and is reverted - aPropagationDataspan is still delivered toActivityStoppedwith tags intact, so gating it dropped the one signal that error-rate queries actually key off.Tagging the people with the most history on this client's tracing implementation, since group A sets the error-recording and ownership conventions that #1980 (semantic-convention conformance) and #1970 (recovery spans) will both build on - easier to change now than after 7.3.0 ships:
@stebet (the original implementation and the OpenTelemetry registration/propagation work), @tmasternak (recommended by @danielmarbach on #1970), @iinuwa (brought the attributes in line with the current messaging conventions), @lmolkova (the spec/conventions questions), @eerhardt (opened #1731, the parent of the connection-span work), @aygalinc (implemented the connection spans this extends).
No obligation to engage. The conventions established here are summarised in #1979.
Span events are on a deprecation path (#1992)
@tmasternak raised that OpenTelemetry is deprecating the span-event API in favour of the Logs API. Verified against the raw specification markdown:
exceptions-spans.mdis marked Status: Deprecated,exception.escapedis deprecated outright, and both exception documents carry a normative block telling existing instrumentations to introduceOTEL_SEMCONV_EXCEPTION_SIGNAL_OPT_INwithlogsandlogs/dup, keeping span events the default while unset.This does not change what ships here. Of the three signals
SetActivityErrorsets, onlyAddExceptionis affected; theErrorstatus anderror.typeare prescribed byrecording-errors.mdand are unaffected. And nothing in the .NET stack supports the migration yet: on DiagnosticSource 9.0.4 none ofAddException,AddEventorSetStatuscarries[Obsolete], the env var appears in no OpenTelemetry .NET assembly includingOpenTelemetry.Api1.17.0, andOpenTelemetry.Instrumentation.AspNetCore1.17.0 still records viaActivity.AddException.RabbitMQ.Client7.x is a stable major version, so 7.3.0 keeps span events. Having all three signals behind one helper is what makes the eventual swap a single edit.One related defect found while checking this, deliberately not fixed here: in
AsyncConsumerDispatcherthedeliveractivity's scope closes at line 55 whileOnCallbackExceptionAsyncfires at line 77, outside it, soActivity.Currenthas already reverted (verified with a probe). Harmless today, butexceptions-logs.mdrequires exception events to be associated with the corresponding span context, so it becomes a MUST violation once exceptions are logs. It is a scope change with no present-day symptom, so it belongs with the migration and its own test rather than in a bug-fix PR. Tracked with the rest in #1992.Scope
Part of #1979. Closes nothing on its own - #1967 stays open as the review of record, with the remaining work split into #1980 (group B, semantic-convention conformance), #1981 (group C, process-global configuration, which gates #1923), and #1992 (span events to Logs API).