Add trace.WithStatus option for span.RecordError and trace.WithStatusOnPanic option for span.End

[amanakin]

Closes #1677

In this commit I add WithErrorStatus as SpanEndOption and EventOption.
With this option we could:

• Call span.RecordError(err, trace.WithStatus()) and error span status will be set.
• Call span.End(trace.WithStatusOnPanic()) and if panic will occur error span status will be set.

For example, otelhttp handler doesn't set span's status if panic is occurred:
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/1808301d09ad7f9695cb7ebd01244102f49df0d1/instrumentation/net/http/otelhttp/handler.go#L160
With this commit is will be possible to pass trace.WithErrorStatus(true) here and set status.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (80e18a5) to head (84fa504).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5762     +/-   ##
=======================================
+ Coverage   84.5%   84.6%   +0.1%     
=======================================
  Files        272     272             
  Lines      22776   22782      +6     
=======================================
+ Hits       19255   19285     +30     
+ Misses      3178    3153     -25     
- Partials     343     344      +1     

see 4 files with indirect coverage changes

[dmathieu]

Do we need the boolean? If we call the option, we want it enabled. Otherwise, we just don't add the parameter?

[amanakin]

Hm, yes, you are right.
I wanted to make it consistent with other options like WithStackTrace

opentelemetry-go/trace/config.go

Line 268 in fb7cc02

func WithStackTrace(b bool) SpanEndEventOption {

However, I am not sure if it's acceptable to use options like:
span.RecordError(err, trace.WithStackTrace(true), trace.WithErrorStatus())

What do you think?

[dmathieu]

I tend to think options with a boolean don't need that parameter. But for consistency, we could maybe still have it here.
Let's wait for other opinions.

[amanakin]

Additionally I thought about splitting WithErrorStatus option:

• WithStatus for span.RecordError call.
• WithStatusOnPanic for span.End call.
  Because it may be not clear what means WithErrorStatus option when you call span.End.

But that also creates inconsistency with WithStackTrace option 🙃

[dmathieu]

I'm thinking the WithStatusOnPanic name would be nice, as it makes things clearer.

[pellared]

These should be better split into two separate PRs - one option for each PR.

[pellared]

This does not seem to be compliant with the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#record-exception. I think that in order to add such option the specification should be updated.

Have you done a research what other languages are doing?

[amanakin]

Yeah, you are absolutely right. RecordError (or RecordException as it is named in the specification) should be just a specialized version of AddEvent, so users should set status manually.
This behavior can also be seen in the Java SDK and Python SDK:
https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L444
https://github.com/open-telemetry/opentelemetry-python/blob/d5fb2c4189a561bd36186d19923373761d4b3e7a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L1014

It is a little bit boilerplate code on every place, where we record exception. But nothing ugly in this case, that would require changing specification.
For example python aiohttp lib:
https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py#L297

So, probably we could only add trace.WithStatusOnPanic(). There is nothing in the specification that explicitly prohibits such an option. I believe it would be useful for users to avoid having a separate defer for catching panics and setting the status.
@pellared WDYT?

[amanakin]

However, I found comments from Ted Young in slack, which states:

It should be an option to set the status when recording the exception, kinda bummed to see that did not make it into the spec. We should add this, or add a convenience method which sets both

I'm gonna ask about this option in specification community.