-
Notifications
You must be signed in to change notification settings - Fork 651
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
Update Span.record_exception to to adhere to specs #1314
Update Span.record_exception to to adhere to specs #1314
Conversation
Can you add a link to the specs changes in question that fixes? Since there was no issue created for this, a small description of the changes would be great. |
Co-authored-by: Leighton Chen <[email protected]>
Hi @lzchen, Thanks for the suggestion. I have updated the description. |
self, | ||
exception: Exception, | ||
attributes: types.Attributes = None, | ||
timestamp: Optional[int] = None, |
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.
Is timestamp
defined in the specs?
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.
From specs
To facilitate recording an exception languages SHOULD provide a RecordException method if the language uses exceptions. This is a specialized variant of AddEvent, so for anything not specified here, the same requirements as for AddEvent apply
An API to record a single Event where the Event properties are passed as arguments. This MAY be called AddEvent. This API takes the name of the event, optional Attributes and an optional Timestamp which can be used to specify the time at which the event occurred
@lonewolf3739 @codeboten Looks like we have an issue and PR open for this already. However if we can get this one approved we can just merge this one. |
Co-authored-by: Leighton Chen <[email protected]>
Co-authored-by: Leighton Chen <[email protected]>
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.
Thanks for implementing this change!
Description
Spec says,
To facilitate recording an exception languages SHOULD provide a
RecordException
method if the language uses exceptions. This is a specialized variant ofAddEvent
, so for anything not specified here, the same requirements as forAddEvent
apply.AddEvent
accepts optional event attributes and timestamp. This PR makesSpan.record_exception
to to adhere to specsHow Has This Been Tested?
tox
Checklist: