Proposal for a Span#Error

[jpkrohling]

When instrumenting the Jaeger Operator, I find myself repeating calls like this:

	if err != nil {
		span.SetStatus(codes.Internal)
		span.SetAttribute(key.String("error", err.Error()))
		return err
	}

Perhaps it's a sign that the API could have a span.Error() function, so that it can be called as follows:

	if err != nil {
		return span.Error(err)
	}

This new function could accept an array of ErrorOptions, allowing callers to override the error code and possibly the attribute name.

[dmathieu]

How about an internal handleError method in your operator:

func handleError(span trace.Span, err error) error {
  span.SetStatus(codes.Internal)
  span.SetAttribute(key.String("error", err.Error()))
  return err
}

Then, we're not setting up a new public method for error handling, and you can still reduce code repetition.

[jpkrohling]

That is certainly a solution, but perhaps this is a pattern that will repeat for pretty much all other Go projects ?

[dmathieu]

Another solution is the discussion in #174.

[jpkrohling]

I understand #174 to be a way for my application to be called back when there's an error in an internal OpenTelemetry component. This issue here is about adding "error" attributes to an active span.

[jmacd]

(Yes, #174 is unrelated.)

This issue points out a flaw in the current implementation of SetStatus(), which takes a code but not a message string. The specification says a status is a pair of a code and a string--you shouldn't have to set the message as a separate span attribute. (See https://github.com/open-telemetry/opentelemetry-proto/blob/7b88843aed39bc270ffa991dabe2d18f7b717c0a/opentelemetry/proto/trace/v1/trace.proto#L252)

Go 1.13 has some new error handling support, see https://blog.golang.org/go1.13-errors

I would consider a proposal that simplified your code to:

if err != nil {
	span.SetStatus(err)
	return err
}

What I'd really prefer is:

func F() (err error) {
   // ... start a span
   defer span.SetStatus(err)
   // ... do things
   if err != nil {
 	return err
   }

[jpkrohling]

If SetStatus(err) would return the same err, then it would be even better to have this:

if err != nil {
	return span.SetStatus(err)
}

(the name SetStatus would be probably slightly inappropriate, though)

[jmacd]

I could also agree that SetStatus is less appropriate because Go uses "error" extensively, that SetError is appropriate. I also suspect that having this method return the argument error will strike some as a source of cognitive overhead, which is not in the spirit of Go.

[lizthegrey]

open-telemetry/opentelemetry-specification#427 covers this

[lizthegrey]

Resolved at spec meeting: we're okay to proceed for 0.3 but may need to change it for 0.4