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.

Sign up for GitHub

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

Jump to bottom

Add error.hint semantic convention #822

Closed
wants to merge 11 commits into from
Closed

Add error.hint semantic convention #822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d276c86
add error.hint semantic convention
mwear Aug 15, 2020
889c4fb
update error.hint commentary
mwear Aug 18, 2020
0d74a04
move error.hint convention to span-general
mwear Aug 20, 2020
2a89696
fix formatting for exceptions spec
mwear Aug 21, 2020
9604de8
separate exception special case in error.hint description
mwear Aug 24, 2020
c08bc0b
refactor RecordException description after rebase
mwear Aug 27, 2020
05293c3
lint fixes
mwear Aug 27, 2020
a3b8487
Merge branch 'master' into error-hint
carlosalberto Aug 27, 2020
d0be51e
Apply suggestions from code review
mwear Aug 28, 2020
9be5e86
improve attribute description
mwear Aug 28, 2020
78710b8
add changelog entry
mwear Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ release.

New:

- Add `error.hint` semantic convention
([#822](https://github.com/open-telemetry/opentelemetry-specification/pull/822))
- Add resource semantic conventions for operating systems
([#693](https://github.com/open-telemetry/opentelemetry-specification/pull/693))
- Clarification of the behavior of the Trace API, re: context propagation, in
Expand Down
17 changes: 12 additions & 5 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,11 +515,18 @@ The method MUST record an exception as an `Event` with the conventions outlined
the [exception semantic conventions](semantic_conventions/exceptions.md) document.
The minimum required argument SHOULD be no more than only an exception object.

If `RecordException` is provided, the method MUST accept an optional parameter
to provide any additional event attributes
(this SHOULD be done in the same way as for the `AddEvent` method).
If attributes with the same name would be generated by the method already,
the additional attributes take precedence.
If `RecordException` is provided, it MUST accept the following optional
parameters:

- Additional event attributes (this SHOULD be done in the same way
as for the `AddEvent` method). If attributes with the same name would be
generated by the method already, the additional attributes take precedence.

- A boolean error hint, as defined by the
Copy link
Member

Choose a reason for hiding this comment

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

There is a question I'd like to have an answer to before merging this: Why is this additional parameter in the RecordException spec instead of the AddEvent spec. Do we expect more intelligent guesses (beyond exception.escaped / #784) to be more likely for exceptions than any other event? Personally, I don't see a reason for that to be true, but there may be one.

I would thus propose to move this parameter to AddEvent. The "Note: RecordException may be seen as a variant of AddEvent with additional exception-specific parameters and all other parameters being optional" then implicitly already allows languages to add error.hint (as well as event name and timestamp) as additional optional parameters, though I would be fine with making these more explicit or even SHOULD requirements here (I'm also OK with a specific MUST requirement for error.hint for RecordException that references the actual spec wording in AddEvent.

The rationale being, that I'd like to see error.hint not unnecessarily coupled with exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would thus propose to move this parameter to AddEvent.

Why? I imagine two main cases for this semantic convention:

  1. There was some exception and you want to mark it as an actual irrecoverable error, thus you call RecordException(error_hint=true)
  2. There was something wrong with some parameters, a validation failed, or some timeout happened, and you want to set the convention directly (no need for an event to happen), without an actual event, so you call SetAttribute() for it.

If users actually want to have a error_hint parameter for AddEvent() we will know for sure, and we can add it in the future. It's easier to add it than to remove it later on IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

  1. There was some exception and you want to mark it as an actual irrecoverable error, thus you call RecordException(error_hint=true)

First, I don't think that it is usual to know whether an exception is irrecoverable or not, usually you only know if it escaped. Second, I would be OK with leaving the parameter a MUST for RecordException, I just don't want the primary definition to be associated with exceptions more than required. I fear that error.hint could become a more widely used but less consistent synonym for exception.escaped, and am happy about every bit of distance it has from exceptions.

  1. There was something wrong with some parameters, a validation failed, or some timeout happened, and you want to set the convention directly (no need for an event to happen), without an actual event, so you call SetAttribute() for it.

It's true that with the current semantic conventions, exceptions are special in that they are the only AFAIK semantic convention for events yet (besides gRPC stream messages). Yet no one is arguing to remove AddEvent. I just think that error.hint is more logical there but I agree that currently there is no use case for having it in AddEvent (other than manually building an exception event).

It's easier to add it than to remove it later on IMHO.

As far as that goes, since #874 it is possible to have any additional attributes in RecordException, so the errorHint parameter could be replaced by calling RecordException with that additional attribute and then calling SetAttribute("error.hint", true) (except for the "only set to false if not set yet" part, which would have to be substituted by "only call SetAttribute if true"). The main thing we accomplish by having this parameter, is guiding users to use the semantic attribute. And personally I am not sure if having an "intelligent guess" that goes beyond exception.escaped is common enough to have it baked into the API. To that end, I would be fine with removing the API changes without replacement (instead of moving/copying the parameter to AddEvent) too, maybe it would be even better after all to move the API changes to a separate PR.

[`error.hint` semantic convention](semantic_conventions/exceptions.md#attributes).
A matching `error.hint` attribute MUST be recorded on both the span and
corresponding event. Once a span has `error.hint` set to true, subsequent
calls to `RecordException` MUST not change the value of the span attribute.

Note: `RecordException` may be seen as a variant of `AddEvent` with
additional exception-specific parameters and all other parameters being optional
Expand Down
17 changes: 17 additions & 0 deletions specification/trace/semantic_conventions/span-general.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,20 @@ Examples of where `thread.id` and `thread.name` can be extracted from:
| Ruby | | `Thread.current.name` |
| C++ | `std::this_thread::get_id()` | |
| Erlang | `erlang:system_info(scheduler_id)` | |

## Error Attributes

A span or event can be annotated with an `error.hint` attribute to indicate that
an error condition was observed.

If there are [exception events](exceptions.md) on a span, the span attribute `error.hint`
SHOULD be set to `true`, if at least one of the event's `error.hint` attribute is `true`.
It's possible for more than one exception
event to have an `error.hint` attribute set. An `error.hint` represents an
instrumentation author's best judgement as to whether or not an error condition
was encountered. Tracing backends can use the `error.hint` attribute as a cue
for further analysis and can ultimately choose to honor or ignore the hint.

| Attribute name | Notes and examples |
| :------------- | :----------------------------------------------------------------------------------------------------------------------- |
| error.hint | A boolean value that represents an instrumentation author's best judgement as to whether an error condition was observed. |