-
Notifications
You must be signed in to change notification settings - Fork 888
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 error.hint semantic convention #822
Closed
Closed
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 889c4fb
update error.hint commentary
mwear 0d74a04
move error.hint convention to span-general
mwear 2a89696
fix formatting for exceptions spec
mwear 9604de8
separate exception special case in error.hint description
mwear c08bc0b
refactor RecordException description after rebase
mwear 05293c3
lint fixes
mwear a3b8487
Merge branch 'master' into error-hint
carlosalberto d0be51e
Apply suggestions from code review
mwear 9be5e86
improve attribute description
mwear 78710b8
add changelog entry
mwear File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 ofAddEvent
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I imagine two main cases for this semantic convention:
RecordException(error_hint=true)
SetAttribute()
for it.If users actually want to have a
error_hint
parameter forAddEvent()
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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).
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 callingSetAttribute("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.