RecordException and Span.Status

[iNikem]

It remained unclear to me after #966 if Span.RecordException should or should not also result in setting Span.Status to ERROR? My question is not "should API method do both", but more "should Instrumentation Library set status to ERROR whenever they record exception or not?"

[Oberon00]

As recordException can also be used for handled exceptions, I don't think it should. Also, then you would need to set the status back to UNSET if you disagreed with the default behavior, which is something we want to avoid. I think setting the status should always be explicit.

CC @tedsuo

[Oberon00]

That said, I think we can recommend that if you record an escaped exception (also setting exception.escaped=true) one should probably (manually) set the status to ERROR.

[jkwatson]

I think that if some instrumentation is explicitly calling the recordException API, then we should assume they are recording it as an ERROR, and hence it should be set. Otherwise, will we add another "recordExceptionAsError" for what I think will be the most common use-case?

[Oberon00]

I think that if some instrumentation is explicitly calling the recordException API, then we should assume they are recording it as an ERROR

I don't think so. A handled exception might be interesting because it may make the operation slower than it needs to be but is an unreachable cache, for example, an error for the parent operation? I think we should not hardcode this decision in recordException.

[Oberon00]

Otherwise, will we add another "recordExceptionAsError" for what I think will be the most common use-case?

I don't think so. We also do not set exception.escaped to true by default, even though it will be the most common use case for auto instrumentation (we don't even provide an overload to set it conveniently, see #784).

[jkwatson]

Otherwise, will we add another "recordExceptionAsError" for what I think will be the most common use-case?

I don't think so. We also do not set exception.escaped to true by default, even though it will be the most common use case for auto instrumentation (we don't even provide an overload to set it conveniently, see #784).

I think we're making the API hard to use correctly for the most common use-case for an end-user, which is simply to mark something as an error, with the associated exception. We should make the most common use-case easy for the end user, not complicated and hard to figure out.

[andrewhsu]

from the issue triage mtg today with TC, allowing changes related to this issue for GA if editorial changes only

[iNikem]

I think this is very important distinction and it seems that different people have very different opinions about that.

#966 says, and I agree, that default status should be Unset. The Error status should be set by auto-instrumentation only if semantic convention dictates that. E.g. HTTP semantic convention will dictate that response statuses 500+ should set Error status. The idea of that, at least to me, is to avoid "blanket" rules that mark too much as Error. If in doubt, leave status as Unset. Thus, I think, that when auto-instrumentation calls Span.recordException method, span status should remain Unset.

We should make the most common use-case easy for the end user, not complicated and hard to figure out.

I don't think that recordException which clearly states that it does NOT modify span status and suggests using setStatus is that complicate and hard to figure out :)

[jkwatson]

I don't think that recordException which clearly states that it does NOT modify span status and suggests using setStatus is that complicate and hard to figure out :)

When we had 16 statuses, it definitely didn't make sense to set one of those statuses when recording an exception. We have a much simpler set of statuses now, so it becomes (IMO) much more intuitive to do it in one step, rather than 2.

[Oberon00]

I think you will need to set an error status maybe 75% of the time, but in the other case, you will have a wrong status code of ERROR which is IMHO worse than the missing information represented by UNSET. And nothing prevents Java from providing a helper like

public static void doTraced(SpanBuilder spanBuilder, Consumer<Span> body) {
  final Span span = spanBuilder.start();
  try {
    body(span);
  } catch (Throwable t) {
    span.recordException(t, Attributes.of(SemanticAttributes.EXCEPTION_ESCAPED, true));
    span.setStatus(Status.of("Exit by exception", StatusCode.ERROR));
    throw t;
  } finally {
    span.end();
  }
}

I think this is a very common pattern, but having recordException set an error implicitly by default is certainly not what we want. I suggest providing a convenience method like the above or some recordFatalException, recordUnhandledException, etc., that set escaped=true and also a status of ERROR. That would make sense, but should be a separate convenience method (preferably not even in the core API). We should avoid too much feature creep in recordException. It seems to attract that 😃