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

RecordException: Allow additional attributes. #874

Merged
merged 11 commits into from
Aug 27, 2020
Merged

RecordException: Allow additional attributes. #874

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
9038240
RecordException: Allow additional attributes.
Oberon00 Aug 25, 2020
f861f30
Update CHANGELOG.
Oberon00 Aug 25, 2020
deb27aa
Typo.
Oberon00 Aug 25, 2020
fa7288b
Address comments, improve linebreaks.
Oberon00 Aug 25, 2020
e11486c
Typo.
Oberon00 Aug 25, 2020
8f6200d
overload -> variant
Oberon00 Aug 25, 2020
75e4ce5
Comma
Oberon00 Aug 26, 2020
d8aa27b
Merge branch 'master' into record-exception
arminru Aug 26, 2020
94fd6fc
Merge branch 'master' into record-exception
arminru Aug 26, 2020
ce4fdfd
Merge branch 'master' into record-exception
arminru Aug 26, 2020
a77db15
Merge branch 'master' into record-exception
arminru Aug 27, 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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ New:
- Clarification of the behavior of the Trace API, re: context propagation, in
the absence of an installed SDK
- Add Span API and semantic conventions for recording exceptions
([#697](https://github.com/open-telemetry/opentelemetry-specification/pull/697))
([#697](https://github.com/open-telemetry/opentelemetry-specification/pull/697),
[#874](https://github.com/open-telemetry/opentelemetry-specification/pull/874))

Updates:

Expand Down
31 changes: 22 additions & 9 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,9 @@ the ordering of the events' timestamps.
Note that the OpenTelemetry project documents certain ["standard event names and
keys"](semantic_conventions/README.md) which have prescribed semantic meanings.

Note that [`RecordException`](#record-exception) is a specialized variant of
`AddEvent` for recording exception events.
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea that RecordException is a variant of AddEvent. Perhaps go a bit further and call it AddExceptionEvent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really have an opinion which name is better and I don't want to expand the scope of this PR. So perhaps this could be done in a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

A follow-up sounds good.

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 forestalling that follow-up PR but just before I forget that idea: I'd even call it AddEventWithException. It may look like a minor nuance but it will be a lot more discoverable since it will be suggested by IDEs when users type AddEve....

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that @iNikem had a strong opinion on renaming RecordException here #814 (comment)

Copy link
Contributor

@iNikem iNikem Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have very strong opinion on renaming RecordException to AddExceptionEvent or similar. The comment above was about removing AddExceptionEvent altogether and replacing it with generic AddEvent


#### Set Status

Sets the [`Status`](#status) of the `Span`. If used, this will override the
Expand Down Expand Up @@ -502,15 +505,25 @@ This API MUST be non-blocking.
#### Record Exception

To facilitate recording an exception languages SHOULD provide a
`RecordException` convenience method. The signature of the method is to be
determined by each language and can be overloaded as appropriate. The method
MUST record an exception as an `Event` with the conventions outlined in the
[exception semantic conventions](semantic_conventions/exceptions.md) document.

Examples:

- `RecordException(exception: Exception)`
- `RecordException(type: String, message: String, stacktrace: String)`
`RecordException` method if the language uses exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

if the language uses exceptions

Do we explicitly rule out Go here? Is the intent? I know that there is an open issue to discuss Go, exceptions and errors (#764) but would like to understand if this change is intentionally excluding Go for now.

Copy link
Member Author

@Oberon00 Oberon00 Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not really. 😕 I thought I was stating the obvious here, but if it's confusing, I can remove this change. EDIT: Technically that now does not say that Go SHOULD provide that method anymore, but it does also not say that Go SHOULD NOT provide it 😃

This is a specialized variant of [`AddEvent`](#add-events),
so for anything not specified here, the same requirements as for `AddEvent` apply.

The signature of the method is to be determined by each language
and can be overloaded as appropriate.
The method MUST record an exception as an `Event` with the conventions outlined in
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.

Note: `RecordException` may be seen as a variant of `AddEvent` with
additional exception-specific parameters and all other parameters being optional
(because they have defaults from the exception semantic convention).

### Span lifetime

Expand Down