Spans are marked as error though the exceptions are handled by @ControllerAdvice

[MALPI]

Describe the bug
I have a service that uses @ControllerAdvice by Spring Boot in order to handle exceptions globally and return the expected Problem response.

When the request is processed like processor.processRequest(request) throws Exception the processRequest method might throw an exception so that the ExceptionHandler is taking care of returning the required response.

processRequest it annotated with @WithSpan as we set custom attributes there. When an Exception is thrown, the Span is marked as error: true, though it shouldn't as the ExceptionHandler is setting status code 400 in the response.

What did you expect to see?

I would have expected to see a regular span with error: false.

What did you see instead?

span with error: true.

What version are you using?

v1.15.0-alpha

[mateuszrzeszutek]

Hey @MALPI ,
Would it be possible for you to provide a reproduction scenario? A minimal app that fails exactly the way you're experiencing

[MALPI]

Let me see if I can create something in the next days.

[mateuszrzeszutek]

Thanks!

[laurit]

@WithSpan sets span status to errored when there is an exception. It has no way of knowing that you are handling the exceptions in some way. Have you considered using the tracer api directly instead of using @WithSpan?

[MALPI]

But throwing an exception not always necessarily means that there is a technical failure?

It has no way of knowing that you are handling the exceptions in some way. Would it make sense to add something so that the implementor can indicate that?

Have you considered using the tracer api directly instead of using @WithSpan?

I'll try that as a workaround.

@mateuszrzeszutek I would say the PoC isn't required anymore.

[trask]

@mateuszrzeszutek @laurit this is an interesting topic. it's not entirely clear to me that we should capture exceptions when there is a non-remote parent span.

[MALPI]

I agree with @trask that it is unexpected that a span is marked as erroneous just because there is thrown an exception.

[mateuszrzeszutek]

@mateuszrzeszutek I would say the PoC isn't required anymore.

Yes, I agree. Sorry for misleading you, I concentrated on the @ControllerAdvice part and totally missed that you were using @WithSpan before.

it's not entirely clear to me that we should capture exceptions when there is a non-remote parent span.

I think that the current behavior makes perfect sense, as the default strategy. From the processRequest() point of view, it ends with an exception, which most often is a sign of failure (which may be recovered by a higher layer).
Perhaps we could make that behavior optional, by adding an option to ignore some exception types -- kind of similar to how Spring's @Transactional allows the user to choose which exceptions should not cause rollback.

[MALPI]

In general this would work, yes. But actually I do think that the semantics are a bit different here.

I would consider a span erroneous due to a technical failure which usually is caused by a dependency. Whereas one could use exceptions as well to indicate business failures, that definitely shouldn't be treated as an error.

[laurit]

Whereas one could use exceptions as well to indicate business failures, that definitely shouldn't be treated as an error.

We have no way to tell whether the exception represents an actual error or not. Given the choice between current behaviour and ignoring exceptions I'd go with what we currently have. The @WithSpan annotation is a convenience method, if it doesn't do exactly what you want there is always the option to use tracer api to create the span that has the exact attributes you need. If you'd still like to use the @WithSpan annotation you should consider submitting a PR that implements the idea that @mateuszrzeszutek proposed.

[trask]

@MALPI once we start populating exception.escaped, would this address your need? (#6410)

[MALPI]

Thanks for the efforts @trask, but I don't think so. If I understand correct it wouldn't avoid that the span is marked as error? This is the actual issue. Probably I would need to get a Tracer and do the according modification as proposed by @laurit.

[trask]

oh ya, I understand what you want now.

seems very related to this discussion: open-telemetry/opentelemetry-specification#1008

[trask]

Also related: #431

[MALPI]

Just thought a bit more, even with exception.escaped I probably could already identify and filter those traces in order not to consider them for SLO measurements.

[trask]

I'm not sure how many configuration options we want to add to @WithSpan.

@zeitlinger @jack-berg this could be a good use case for a convenience API somewhere in between @WithSpan and opentelemetry-api

linking to open-telemetry/opentelemetry-java-contrib#759

[zeitlinger]

To avoid surprises, users should be able to rely on @ControllerAdvice to be the autoritative source of error semantics.
I'm not sure if it's possible, but it would be great if controller advice could change the error flag of the @WithSpan - without the user doing anything.

Apart from that, open-telemetry/opentelemetry-java-contrib#759 is planning to allow you to customize the error setting behavior as well.

[MALPI]

Hey @trask @trask is there some estimation on when open-telemetry/opentelemetry-java-contrib#759 could be released?

[zeitlinger]

Hey @trask @trask is there some estimation on when open-telemetry/opentelemetry-java-contrib#759 could be released?

it was planned for the current release 2 days ago - but missed.
I think it'll go in the release one month from now on.

[trask]

as mentioned above, I don't think this can be detected automatically but can be resolved by calling Span.current().setStatus(StatusCode.OK) inside of the method that is annotated with @WithSpan, which will override the error status set by the automatic instrumentation:

https://github.com/open-telemetry/opentelemetry-java/blob/717d8b00320d4e0dadc15d96f9c48559bd2b2e20/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L437-L439