Skip to content

Set correct span status for Send and SendAsync#2397

Merged
iancooper merged 2 commits into
BrighterCommand:masterfrom
easyfy-fredrik:otel-span-status-false-negative
Dec 6, 2022
Merged

Set correct span status for Send and SendAsync#2397
iancooper merged 2 commits into
BrighterCommand:masterfrom
easyfy-fredrik:otel-span-status-false-negative

Conversation

@easyfy-fredrik
Copy link
Copy Markdown
Contributor

OpenTelemetry instrumentation

Span status (Error/OK) gets flipped for Send() and SendAsync() while being correct for Publish() and PublishAsync().
EndSpan() has a success parameter which sets the span status to Error if true which is a bit confusing.

This change sets Error status directly on exception and assumes success if no other status is set, this should fix the flipped status and remove the need for the success parameter.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 3, 2022

CLA assistant check
All committers have signed the CLA.

@iancooper
Copy link
Copy Markdown
Member

Thanks @easyfy-fredrik I am going to pull in @preardon to review, as he wrote our original telemetry support

Copy link
Copy Markdown
Member

@preardon preardon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@iancooper
Copy link
Copy Markdown
Member

@easyfy-fredrik We have a couple of other fixes in the pipe, if they are there in the next 24 hours I will fold into a release, otherwise I will get this out then

@iancooper iancooper merged commit c5aee1c into BrighterCommand:master Dec 6, 2022
DevJonny pushed a commit to DevJonny/Brighter that referenced this pull request Feb 28, 2026
…icitly on exception (BrighterCommand#2397)

Co-authored-by: Ian Cooper <ian_hammond_cooper@yahoo.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants