otelhttp: Cannot record error for spans when producing new semantic convention

[XSAM]

Description

otelhttp only uses span.RecordError(err) when errType is not empty

opentelemetry-go-contrib/instrumentation/net/http/otelhttp/transport.go

Lines 132 to 140 in b857249

	if err != nil {
	// set error type attribute if the error is part of the predefined
	// error types.
	// otherwise, record it as an exception
	if errType := t.semconv.ErrorType(err); errType.Valid() {
	span.SetAttributes(errType)
	} else {
	span.RecordError(err)
	}

But ErrorType method never returns an empty attribute.

opentelemetry-go-contrib/instrumentation/net/http/httptrace/otelhttptrace/internal/semconv/httpconv.go

Lines 443 to 458 in b857249

	func (n CurrentHTTPClient) ErrorType(err error) attribute.KeyValue {
	t := reflect.TypeOf(err)
	var value string
	if t.PkgPath() == "" && t.Name() == "" {
	// Likely a builtin type.
	value = t.String()
	} else {
	value = fmt.Sprintf("%s.%s", t.PkgPath(), t.Name())
	}
	if value == "" {
	return semconvNew.ErrorTypeOther
	}
	return semconvNew.ErrorTypeKey.String(value)
	}

Expected behavior

I don't have a preference on whether we should record error.type with exception.type at the same time. But the comment and the logic here seem conflicted.

[dmathieu]

I don't know what was the intent there. But it feels like ErroTypeOther should be the case where we want to fallback to recording an error?

[XSAM]

But it feels like ErroTypeOther should be the case where we want to fallback to recording an error?

I don't find a way to make it return ErrorTypeOther. In most cases, it returns semconv.ErrorTypeKey.String("*errors.errorString").

For instance, errors created by errors.New() like driver.ErrBadConn. The type is *errors.errorString.

I know it is difficult (or impossible) to get this via reflect, but do you think returning semconv.ErrorTypeKey.String("database/sql/driver.ErrBadConn") is the ideal result for driver.ErrBadConn?

where we want to fallback to recording an error?

I feel we should always make span recording the error, as this is the only way users can choose to record exception.message and exception.stacktrace. While only know the error type does not seem to help a lot, these two attributes are very helpful when debugging.

@open-telemetry/go-maintainers What do you think about this issue?

[XSAM]

More information: https://opentelemetry.io/docs/specs/otel/trace/exceptions/ (not in the semconv doc)

The spec about exceptions says

An exception SHOULD be recorded as an Event on the span during which it occurred if and only if it remains unhandled when the span ends and causes the span status to be set to ERROR.

It even has a pseudo code that sets the error.type attribute for spans and calls recordException at the same time.

Span span = myTracer.startSpan(/*...*/);
try {
  // Code that does the actual work which the Span represents
} catch (Throwable e) {
  span.recordException(e);
  span.setAttribute(AttributeKey.stringKey("error.type"), e.getClass().getCanonicalName())
  span.setStatus(StatusCode.ERROR, e.getMessage());
  throw e;
} finally {
  span.end();
}

---

So, a good start for us is to change the code from

		// set error type attribute if the error is part of the predefined
		// error types.
		// otherwise, record it as an exception
		if errType := t.semconv.ErrorType(err); errType.Valid() {
			span.SetAttributes(errType)
		} else {
			span.RecordError(err)
		}

		span.SetStatus(codes.Error, err.Error())

to

		// ErrorType should always return a valid error.type` for error
		span.SetAttributes(t.semconv.ErrorType(err))
		span.RecordError(err)
		span.SetStatus(codes.Error, err.Error())

[pellared]

What is the value of recording the error as event if the type is set as attribute and value (err.Error()) as status description?

An exception SHOULD be recorded as an Event on the span during which it occurred if and only if it remains unhandled when the span ends and causes the span status to be set to ERROR.

I argue that error result is not an exception. For me a panic that was not being recovered is an unhandled exception. We already cover this part of this specification here:

https://github.com/open-telemetry/opentelemetry-go/blob/bc531cb3f9ce9c328b12e473b6409784728dc608/sdk/trace/span.go#L465-L482

Additionally see: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/recording-errors.md#recording-errors-on-spans

Therefore, we should not call span.RecordError(err).

[dashpole]

@pellared does that mean span.RecordError should essentially never be called by instrumentation, since panic recovery is already handled by the SDK?

Almost makes it seem like we should deprecate RecordError in the API, and document that End should handle recording exceptions for panics.

[pellared]

My proposal is to not use span.RecordError for terminating errors per recording errors on spans semantic conventions.

During the SIG meeting, it was also noted that span.RecordError captures a stack trace. However, since the stack trace reflects where the error was recorded, not where it originated, I don’t find it particularly valuable. Additionally, capturing and emitting a stack trace has a performance cost. In most languages with exception handling (such as Java or .NET), the stack trace is already included in the thrown exception so the cost of capturing the stack trace was already taken and the stack trace is taken in the place where the exception was thrown.

@pellared does that mean span.RecordError should essentially never be called by instrumentation, since panic recovery is already handled by the SDK? Almost makes it seem like we should deprecate RecordError in the API,

I believe span.RecordError remains valuable, particularly for non-terminating errors. A common use case is retry logic, where errors occur but don't immediately end the span. This is especially relevant in the context of custom instrumentation, where developers have full control and can explicitly record such intermediate errors.

While instrumentation libraries typically just return errors, they could also benefit from span.RecordError if the underlying library provides hooks into retry or failure logic. In such cases, recording retry attempts as span events via span.RecordError gives users better visibility into transient issues without prematurely ending the span.

For these reasons, I don’t think span.RecordError should be deprecated. It might be worth capturing these use cases in the error recording semantic conventions (cc @lmolkova).

Side note: There is an OTEP open-telemetry/opentelemetry-specification#4333 that proposes to record exceptions/errors as log records instead of span events.

and document that End should handle recording exceptions for panics.

I think that documenting that span.End should handle recording exceptions for panics is a good idea.

In my opinion, this is how we implement recording an exception in the most possible user-friendly way.