Skip to content
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

Change SpanStatusExtractor to use a builder that can set status description #6035

Merged
merged 6 commits into from
May 19, 2022

Conversation

HaloFour
Copy link
Contributor

Currently the SpanStatusExtractor can only extract a StatusCode from the response and any error and never sets an appropriate description for the status of the span. This change adds a method to the SpanStatusExtractor to use a helper interface SpanStatusBuilder which delegates the setStatus methods to the underlying Span so that the extractor may set the status code and description.

@HaloFour
Copy link
Contributor Author

I assume that we don't want to take breaking changes to the instrumenter interfaces, so we either need to break the new functionality into a new interface, or munge it into the existing one so that existing implementations (including lambdas) continue to work.

I also did not touch the HttpSpanStatusExtractor or GrpcSpanStatusExtractor implementations in this pass.

@trask
Copy link
Member

trask commented May 16, 2022

I assume that we don't want to take breaking changes to the instrumenter interfaces

instrumentation-api has not been declared stable yet (though getting close), so we can (and should) make breaking changes if it makes for a better api

@HaloFour
Copy link
Contributor Author

@trask

instrumentation-api has not been declared stable yet (though getting close), so we can (and should) make breaking changes if it makes for a better api

Good to know, that will make this much cleaner.

@HaloFour
Copy link
Contributor Author

HaloFour commented May 16, 2022

Updated AbstractHttpClientTest may be more involved since the status description may be different for each instrumented library. I've opened another PR which adds a new assertion method hasStatusSatisfying which would allow the tests to more flexibly test the status of the span.

Just realized that SpanDataAssert also has a generic satisfies method which will enable asserting the status with more flexibility.

if (error != null) {
return StatusCode.ERROR;
spanStatusBuilder.setStatus(StatusCode.ERROR, error.toString());
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear about this part of the spec:

When the status is set to Error by Instrumentation Libraries, the Description SHOULD be documented and predictable.

and whether an exception message is considered "predictable"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that statement makes sense for error conditions as interpreted by the instrumentation, e.g. a failing status code or the like. I can see where exceptions might be a bit murkier, but as long as the exception is being recorded as an event perhaps having it also be the description isn't as necessary (although I guess technically an exception event doesn't need to be the reason that the span status is "Error" either).

I can certainly unwind the rest of the changes here so the exception message is not the description. Would certainly make it easier to not have to deal with all of the exceptions that it broke. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted setting the error message as the description by default, that's something we can revisit. This PR will instead only provide the SpanStatusExtractor that is capable of setting that description.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
The exception message can contain pretty much anything (see #3039 for a really bad example), I too wouldn't call it "predictable". And we're already capturing it as the exception.message attribute, adding the same string as status description would be duplicating the data a little bit.

@HaloFour HaloFour force-pushed the status-extractor-description branch from 8a84989 to 3609646 Compare May 17, 2022 13:16
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Thanks!
LGTM 👍

Comment on lines 31 to 35
if (status.isOk()) {
return StatusCode.UNSET;
spanStatusBuilder.setStatus(StatusCode.UNSET);
} else {
spanStatusBuilder.setStatus(StatusCode.ERROR);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could be

      if (!status.isOk()) {
        spanStatusBuilder.setStatus(StatusCode.ERROR);
      }

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx!

@mateuszrzeszutek
Copy link
Member

Hey @HaloFour
Looks like this PR already got all approvals it needed -- do you want to make it ready for review?

@HaloFour HaloFour marked this pull request as ready for review May 19, 2022 16:34
@HaloFour HaloFour requested a review from a team May 19, 2022 16:34
@trask trask merged commit b7fc80c into open-telemetry:main May 19, 2022
@trask
Copy link
Member

trask commented May 19, 2022

thx @HaloFour!

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
…iption (open-telemetry#6035)

* Change SpanStatusExtractor to use a builder that can set status descripion

* Refactor SpanStatusExtractor to only support builder approach to setting status

* Revert setting the exception as the status description

* PR feedback

* Re-fix graghql instrumentation span extractor

* Spotless
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants