Add Span#RecordError method to simplify adding error events to spans

[Aneurysm9]

Fixes #248.

I wonder whether Tracer#WithSpan should invoke this new method with the result of the user-provided function.

[lizthegrey]

span.Error() feels slightly weird to me. ReportError() or SetError() as per original bug? dunno, curious what other approvers think.

[paivagustavo]

Agree with @lizthegrey.

And since this is an implementation of the OpenTelemetry specs, I think we shouldn't be adding new methods to the Span API before changing the specs, I don't know the exactly process for these changes but I'm sure others could help.

[Aneurysm9]

Understandable. I had thought that since #248 wasn't tagged as "blocked on spec" and the new method related specifically to Go error handling it might not require spec changes. I'd be happy to help draft and shepherd changes to the spec through the process if someone can point me in the right direction and maybe show some examples of similar changes that have made it through the process.

[lizthegrey]

am discussing at spec meeting now (you're welcome to join)

[lizthegrey]

someone filled me in, what we want is open-telemetry/opentelemetry-specification#427

[lizthegrey]

We're clear to use our judgment for 0.3, but we may need to change it between here and 0.4

[Aneurysm9]

Ok, so based on a brief reading of open-telemetry/opentelemetry-specification#427 it looks like the direction to go would be changing this to span.RecordError() and adding an event rather than setting an attribute and status code. If that seems appropriate I'll take another swing at this.

[lizthegrey]

I think that setting the attribute/status code is actually fine, because otherwise it'll require more complex cross-span/event correlation where just setting an attribute would do. I'll chime in to that effect on the upstream spec discussion

[jmacd]

changing this to span.RecordError() and adding an event rather than setting an attribute and status code.

+1

[wingyplus]

I try with Jaeger exporter on my forked of gomongowrapper, it's seems like it's needs to called SetStatus to mark span to be an error. So I start thinking that ErrorConfig should accept status code to make it configurable. And then set status in RecordError if status is not zero. Not sure what do you think?

[Aneurysm9]

That's a good point and something lost when moving from status/attributes to recording an event. I've added a WithErrorStatus(codes.Code) option that will set the span status if given a value != codes.OK.

[krnowak]

Did some nitpicking.

[krnowak]

One last thing (honest!), otherwise looks good.

[jmacd]

This line raises questions for me about the SetStatus API, which is tangential to this PR. The Span.Status field has two fields, the code (passed here) and a message. This should probably set the same err.Error() as used for the event message, but the SetStatus API doesn't accept a message string (which is a separate matter).

This has me wondering whether we want to use RecordError(..., WithStatusCode()) as the conventional pattern for recording an error and setting the status. I'm not sure I'd pick this approach -- I'm inclined for there to be two methods, one to record an error event, and one to record an error event plus set the status (i.e., two methods RecordErrorEvent and RecordErrorStatus).