Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

amanakin
Copy link
Contributor

@amanakin amanakin commented Sep 2, 2024

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.

Copy link

codecov bot commented Sep 2, 2024

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

Impacted file tree graph

@@           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

@amanakin amanakin changed the title add WithErrorStatus to SpanEndOption and EventOption add WithErrorStatus option to span.RecordError and span.End Sep 3, 2024
@amanakin amanakin changed the title add WithErrorStatus option to span.RecordError and span.End Add WithErrorStatus option to span.RecordError and span.End Sep 3, 2024
trace/config.go Outdated
var _ SpanEndEventOption = errorStatusOption(true)

// WithErrorStatus sets the flag to set span's status to error if error or panic is occurred.
func WithErrorStatus(b bool) SpanEndEventOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amanakin amanakin changed the title Add WithErrorStatus option to span.RecordError and span.End Add trace.WithStatus option for span.RecordError and trace.WithStatusOnPanic option for span.End Sep 16, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -8,6 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Added

- Add the `trace.WithStatus` option for `span.RecordError`. (#5762)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants