Attach original Exception to Event in Span#recordException

[HaloFour]

I am using the SDK reference implementation Tracer with an implementation of SpanExporter that wraps Apache log4j2. I am adding exception events to the current Span via the recordException method which takes the exception and appends the details as attributes, including the full stack trace. I'd like to be able to include the exception information with stack trace in the logged span, however without being able to retrieve the original Throwable instance I am unable to rely on the configured pattern in log4j2 to manage formatting. As the project in question is built on Spring WebFlux the stack traces are quite large so having the ability to concisely format the exception and stack trace is important.

An ideal solution would be to have a way to negotiate original exception from the events appended to the SpanData, such as a new interface that extends from SpanData.Event that exposes the exception through a member:

public interface ExceptionEvent extends SpanData.Event {
    Throwable getException();
}

My SpanExporter implementation could then iterate through the span events to find an event that implements this interface to obtain the original exception and write it to the log, e.g.:

Throwable exception = null;
if (spanData.getTotalRecordedEvents() > 0) {
    for (SpanData.Event event : spanData.getEvents()) {
        if (event instanceof ExceptionEvent) {
            exception = ((ExceptionEvent) event).getException();
            break;
        }
    }
}

if (exception != null) {
    logger.info(spanId.toLowerBase16());
} else {
    logger.error(spanId.toLowerBase16(), exception);
}

[jkwatson]

Thanks for this issue.

As you say, at the moment, there is a method on the API to recordException on a Span, which creates an Event for you under the hood, based on the provided Throwable. I think an alternative solution to this might be to have the SDK (optionally?) keep the original Throwable around, attached to the Event itself, which could then be passed down to exporters via the SpanData version of the Event. I'd rather not create an additional interface for that, though, and just have the Throwable optionally attached directly to the event.

I feel like there was some reason we didn't do this originally. @anuraaga do you remember the thinking here?

[Oberon00]

The reason not to do this is simple: Throwable is not a valid attribute type. And recordException is specified as as a wrapper around addEvent, so it should not do anything that is not also possible with addEvent.

[Oberon00]

I understand the use case, but I would rather go with allowing any object type as attribute generally, like OpenTracing did, and it is then up to the exporters (or should we add AttributeProcessors) to translate them to something their particular protocol supports.

[jkwatson]

The reason not to do this is simple: Throwable is not a valid attribute type. And recordException is specified as as a wrapper around addEvent, so it should not do anything that is not also possible with addEvent.

I was thinking of having this be a special, non-attribute case, and storing it directly as a new field on the Event API.

[Oberon00]

I think that would solve this use case nicely. But I am worried that this might be a symptom of a deeper design issue, and by adding the workaround of adding the exception as a special case in recordException (transforming it from a convenience wrapper to a primitive), we lessen the pressure to fix the design issue, and carry it with us longer. Also there might be memory issues if we keep exceptions around for longer. That's not something you usually do with exceptions, so they might be quite heavy, being designed with the expectation to be alive only for the duration of a single throw/catch.

[jkwatson]

I think that would solve this use case nicely. But I am worried that this might be a symptom of a deeper design issue, and by adding the workaround of adding the exception as a special case in recordException (transforming it from a convenience wrapper to a primitive), we lessen the pressure to fix the design issue, and carry it with us longer. Also there might be memory issues if we keep exceptions around for longer. That's not something you usually do with exceptions, so they might be quite heavy, being designed with the expectation to be alive only for the duration of a single throw/catch.

Yes, it would be pretty easy to create a new type of attribute, if we wanted to go that way. But, that wouldn't alleviate the memory concerns, but potentially exacerbate them. It does seem like this might a case where we would want to expose the API to end-users, but strongly recommend that auto/library instrumentation didn't use it. Did you have something else in mind to deal with the memory issues?

[HaloFour]

Alternatively what do you think about exposing a way to get back to the original wrapped Event or SpanData.Event implementation? That way I could write my own ExceptionEvent and pass it to Span#addEvent manually rather than using Span#recordException, and then I could manually negotiate my way back to the custom implementation in my SpanExporter?

[jkwatson]

@HaloFour with any of these suggestions, we'd need to get a change to the specifications, since they are pretty limited for the Event API at the moment (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#add-events). Would you mind opening an issue in the specs repo to work towards a change?

[HaloFour]

@jkwatson

Does the specification preclude the ability for the an implementation of the interface to provide a superset of the specified members?

[jkwatson]

@jkwatson

Does the specification preclude the ability for the an implementation of the interface to provide a superset of the specified members?

Not necessarily, but we're trying very hard to make the API consistent across all the languages. If we want to add new features to the API, we would definitely want them to go through the spec process, so that the languages don't diverge in their feature set. The differences between languages should be almost entirely limited to differences in language idiom, rather than functionality.

The ability to have a custom event type propagate through the SDK's span pipeline to the exporters would definitely need cross-language discussion before being adopted.

[HaloFour]

I opened open-telemetry/opentelemetry-specification#955. It's a bit open ended but I hope I conveyed enough information about my use case.

[breedx-splk]

It seems to me that a lot of the exception details are now recorded on the event, and this issue might be stale now?

@HaloFour do you still have a programatic need to get the actual exception instance, or is having the type, message, and stack recorded enough for your needs?

[HaloFour]

@breedx-splk

do you still have a programatic need to get the actual exception instance, or is having the type, message, and stack recorded enough for your needs?

I want the original exception so that I can control the formatting of those values, particularly the stack trace. I want the ability to abbreviate class names and exclude unnecessary frames from that stack trace, which is a feature that most logging frameworks offer out of the box but only if you provide the original exception instance.

[breedx-splk]

I want the original exception so that I can control the formatting of those values, particularly the stack trace. I want the ability to abbreviate class names and exclude unnecessary frames from that stack trace, which is a feature that most logging frameworks offer out of the box but only if you provide the original exception instance.

Got it. Thanks for clarifying!

[jack-berg]

Resolved in #4162.