Override status with @WithSpan-annotated methods

[HaloFour]

I was wondering if there was interest in exploring a way to override the default manner in which the WithSpan aspects determine the status of the Span created for a method?

Currently that status is driven by whether or not the method throws an exception. I think this works in the majority of cases. However we've found in our code bases occasions where sometimes we throw a specific exception but we consider that to be the correct behavior for the method in that circumstance and not really an "error" case. An example might be a Spring controller method that enforces constraints on the parameters and throws a ConstraintViolationException.

We currently have our own Spring aspect for OpenTelemetry that was written quite some time ago that I am trying to retire in favor of the Instrumentation aspect. The way we solved for this functionality in our aspect was that our flavor of the @WithSpan annotation had an ignoredExceptions element of type Class<? extends Exception>[] and if the method threw that exception the aspect would compare the type of the exception against that list. It worked for our purposes but it was pretty limited in that you couldn't programmatically override the status from within the method.

Another method I considered could be to have the aspect tracer inspect the current status of the span. If the span status was already set then the tracer would not override it, so if the method set the status to OK then it would remain OK. However, as the API Span is write-only this would only really work for implementations of ReadableSpan.

Thoughts?

[mateuszrzeszutek]

If the span status was already set then the tracer would not override it, so if the method set the status to OK then it would remain OK.

There's an ongoing effort in the spec to introduce this behavior: https://github.com/open-telemetry/opentelemetry-specification/pull/1685/files
After this is implemented once you set something != Unset then status will freeze and won't be overwritten by an instrumentation.

[iNikem]

@HaloFour can you please express your support for that spec change? :) As it is getting some resistance we need as much votes in favour as possible :)

[HaloFour]

Weighed in.

I'm having trouble trying to process what "hook" means here. Seems a bit handwavey to me. It makes me think of something that lives far from the instrumented method, where the information used to determine the status would not be readily available.

But reading through the proposed spec I'm also not sure I agree with some of the details, such as "first wins" or "OK overrules them all." (If I'm misinterpreting the proposal please correct me). As a stupid developer, to me the easiest route would seemingly be to add a new method to Span called isStatusSet which would return true if status != UNSET. Maybe breaks some of the rules of the spec as Spans are intended to be write-only, but to me that feels like the easiest way to attain this goal without requiring side channels or weird precedence rules.

[HaloFour]

Another option, what if instrumentation could return a delegated Span implementation that could detect if setStatus is called and allow the instrumentation tracer to detect this? Would that break spec or require spec changes?

[iNikem]

"OK overrules them all" was how Ok was designed. At least that is the most prevalent understanding right now.

[HaloFour]

Ok, so this is more about clarifying the understood behavior in the spec then? That makes sense to me. At the end of the day it doesn't matter to me that much as to how it is implemented as long as the developer experience is simple and intuitive.