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())