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

opentelemetry: add semconv exception fields #2135

Merged
merged 6 commits into from
Jun 23, 2022

Conversation

lilymara-onesignal
Copy link
Contributor

When an error value is recorded, it should add the exception.message and
exception.stacktrace fields from the opentelemetry semantic conventions to the
span/event. Ideally the exception.type field would be provided also, but there
is currently not a mechanism to determine the type name of a trait object.

Motivation

Motivated by comment on another PR - #2122 (comment)

@lilymara-onesignal lilymara-onesignal requested review from jtescher and a team as code owners May 20, 2022 20:21
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

So, this seems correct overall!

I am not sure if this is really going to be all that useful in practice yet, though: this branch will emit the semconv exception fields if a Span records a value of type Error. However, in typical tracing use, it's much more common for errors to be recorded by an event inside of a span. I think we may want to consider also attaching the exception fields to the current span whenever an event is recorded with an Error value. This should probably be configurable behavior, since some users may not want it.

If we do that, we'll also have to figure out what happens if multiple events with Errors occur in the same span. How does OpenTelemetry handle multiple fields with the same name?

tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Jun 7, 2022

@lilymara-onesignal any updates on this?

@lilymara-onesignal
Copy link
Contributor Author

Whoops, sorry I missed the comments on this. I'll see about adding configurability for these otel fields and adding them to the event's current span. On duplicate fields, it seems that the otel library will drop the older field and report only the newer field.

@lilymara-onesignal
Copy link
Contributor Author

@hawkw I think this may be ready for another pass, the behavior is configurable and I've added docs/tests

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

some minor style nits and documentation suggestions. beyond that, this seems really good, thank you!

tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
@lilymara-onesignal
Copy link
Contributor Author

Ok I think I've addressed everything but the defaults - on the fence there also. It seems to me like walking the "cause" chain is the most expensive part of all of this, and that's enabled by default and can't currently be turned off. I don't feel extremely strongly either way.

@lilymara-onesignal
Copy link
Contributor Author

There's also the possibility that we could set the status code on the event/span whenever we record an error event

@jtescher
Copy link
Collaborator

jtescher commented Jun 16, 2022

If we do that, we'll also have to figure out what happens if multiple events with Errors occur in the same span. How does OpenTelemetry handle multiple fields with the same name?

All otel attribute collections have unique key requirements, generally last write wins.

There's also the possibility that we could set the status code on the event/span whenever we record an error event

It's not always clear that the status of the span has changed when an error event occurs (e.g. operations with retries or concurrent alternatives)

@lilymara-onesignal
Copy link
Contributor Author

@hawkw could I get another review on this? I think the only outstanding item is the default behavior, and I'm not sure how to come to an answer on that.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

@lilymara-onesignal sorry I didn't get back to you on this sooner. I think this PR seems good overall, but I think we should have the exception fields be off by default, making them opt-in rather than opt-out, due to the additional overhead they incur. If we can find a way to reduce the overhead of recording these fields (perhaps if the changes to opentelemetry i proposed in open-telemetry/opentelementry-rust#809 are made?) we could change them from opt-in to opt-out.

Does that seem reasonable to everyone (cc @jtescher)?

tracing-opentelemetry/src/layer.rs Outdated Show resolved Hide resolved
@lilymara-onesignal
Copy link
Contributor Author

No problem, I think at this point I've resolved all of the points of contention

@lilymara-onesignal
Copy link
Contributor Author

Hmm, not really sure what to do about that MSRV check

@hawkw
Copy link
Member

hawkw commented Jun 22, 2022

Hmm, not really sure what to do about that MSRV check

That's fixed on v0.1.x, it's unrelated. Mind rebasing?

When an error value is recorded, it should add the `exception.message` and
`exception.stacktrace` fields from the opentelemetry semantic conventions to the
span/event. Ideally the `exception.type` field would be provided also, but there
is currently not a mechanism to determine the type name of a trait object.
Copy link
Collaborator

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @lilymara-onesignal

@jtescher jtescher added the crate/opentelemetry Related to the `tracing-opentelemetry` crate. label Jun 23, 2022
@jtescher jtescher merged commit b007591 into tokio-rs:v0.1.x Jun 23, 2022
@lilymara-onesignal lilymara-onesignal deleted the exception branch June 23, 2022 17:07
hawkw added a commit that referenced this pull request Jul 1, 2022
Currently, the `tracing-opentelemetry` docs indicate that the support
for OpenTelemetry's exception semantic conventions added in #2135 is
enabled by default. However, this is not the case --- this feature was
changed to opt-in rather than opt-out prior to merging PR #2135.

This branch updates the docs to state that these features are disabled
by default, rather than enabled by default.
hawkw added a commit that referenced this pull request Jul 1, 2022
## Motivation

Currently, the `tracing-opentelemetry` docs indicate that the support
for OpenTelemetry's exception semantic conventions added in #2135 is
enabled by default. However, this is not the case --- this feature was
changed to opt-in rather than opt-out prior to merging PR #2135.

## Solution

This branch updates the docs to state that these features are disabled
by default, rather than enabled by default.
hawkw added a commit that referenced this pull request Jul 1, 2022
# 0.17.4 (July 1, 2022)

This release adds optional support for recording `std::error::Error`s
using OpenTelemetry's [semantic conventions for exceptions][exn-semconv].

### Added

- `Layer::with_exception_fields` to enable emitting `exception.message`
  and `exception.backtrace` semantic-convention fields when an `Error`
  is recorded as a span or event field ([#2135])
- `Layer::with_exception_field_propagation` to enable setting
  `exception.message` and `exception.backtrace` semantic-convention
  fields on the current span when an event with an `Error` field is
  recorded ([#2135])

Thanks to @lilymara-onesignal for contributing to this release!

[thread-semconv]: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/exceptions/
[#2135]: #2135
hawkw added a commit that referenced this pull request Jul 1, 2022
# 0.17.4 (July 1, 2022)

This release adds optional support for recording `std::error::Error`s
using OpenTelemetry's [semantic conventions for exceptions][exn-semconv].

### Added

- `Layer::with_exception_fields` to enable emitting `exception.message`
  and `exception.backtrace` semantic-convention fields when an `Error`
  is recorded as a span or event field ([#2135])
- `Layer::with_exception_field_propagation` to enable setting
  `exception.message` and `exception.backtrace` semantic-convention
  fields on the current span when an event with an `Error` field is
  recorded ([#2135])

Thanks to @lilymara-onesignal for contributing to this release!

[thread-semconv]: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/exceptions/
[#2135]: #2135
davidbarsky pushed a commit to tokio-rs/tracing-opentelemetry that referenced this pull request Mar 21, 2023
# 0.17.4 (July 1, 2022)

This release adds optional support for recording `std::error::Error`s
using OpenTelemetry's [semantic conventions for exceptions][exn-semconv].

### Added

- `Layer::with_exception_fields` to enable emitting `exception.message`
  and `exception.backtrace` semantic-convention fields when an `Error`
  is recorded as a span or event field ([#2135])
- `Layer::with_exception_field_propagation` to enable setting
  `exception.message` and `exception.backtrace` semantic-convention
  fields on the current span when an event with an `Error` field is
  recorded ([#2135])

Thanks to @lilymara-onesignal for contributing to this release!

[thread-semconv]: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/exceptions/
[#2135]: tokio-rs/tracing#2135
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
When an error value is recorded, it should add the `exception.message` and
`exception.stacktrace` fields from the opentelemetry semantic conventions to the
span/event. Ideally the `exception.type` field would be provided also, but there
is currently not a mechanism to determine the type name of a trait object.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
## Motivation

Currently, the `tracing-opentelemetry` docs indicate that the support
for OpenTelemetry's exception semantic conventions added in tokio-rs#2135 is
enabled by default. However, this is not the case --- this feature was
changed to opt-in rather than opt-out prior to merging PR tokio-rs#2135.

## Solution

This branch updates the docs to state that these features are disabled
by default, rather than enabled by default.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.17.4 (July 1, 2022)

This release adds optional support for recording `std::error::Error`s
using OpenTelemetry's [semantic conventions for exceptions][exn-semconv].

### Added

- `Layer::with_exception_fields` to enable emitting `exception.message`
  and `exception.backtrace` semantic-convention fields when an `Error`
  is recorded as a span or event field ([tokio-rs#2135])
- `Layer::with_exception_field_propagation` to enable setting
  `exception.message` and `exception.backtrace` semantic-convention
  fields on the current span when an event with an `Error` field is
  recorded ([tokio-rs#2135])

Thanks to @lilymara-onesignal for contributing to this release!

[thread-semconv]: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/exceptions/
[tokio-rs#2135]: tokio-rs#2135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/opentelemetry Related to the `tracing-opentelemetry` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants