RecordException: Allow additional attributes. #874
Conversation
Oberon00
added
area:api
|
Closing & reopening this PR because the bot did not assign anyone. Maybe this was because I originally created a draft PR? Is some "ready for review" event missing in the bot's triggers? |
|
Assigning @bogdandrutu as responsible TC member because the bot seems to not support assigning ready-for-review draft PRs, and @bogdandrutu has already commented on this topic on #784. |
|
|
||
| - `RecordException(exception: Exception)` | ||
| - `RecordException(type: String, message: String, stacktrace: String)` | ||
| `RecordException` method if the language uses exceptions. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😃
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
| @@ -450,6 +450,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. | |||
There was a problem hiding this comment.
I like the idea that RecordException is a variant of AddEvent. Perhaps go a bit further and call it AddExceptionEvent?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
A follow-up sounds good.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
Note that @iNikem had a strong opinion on renaming RecordException here #814 (comment)
There was a problem hiding this comment.
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
|
@Oberon00 Please rebase. |
|
Enough approvers to be merged, but needs a rebase. I would recommend the author to allow maintainers to re-sync the branch |
|
@bogdandrutu Done! |
|
I would consider then to use a fork in your account instead of a fork in a company org to allow that. Again this needs a rebase |
* RecordException: Allow additional attributes. * Update CHANGELOG. * Typo. * Address comments, improve linebreaks. * Typo. * overload -> variant Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * Comma Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Fixes #814.
Changes
Related PRs