-
Notifications
You must be signed in to change notification settings - Fork 889
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
Mark exceptions as stable. We do not plan to change this behavior in… #3769
Mark exceptions as stable. We do not plan to change this behavior in… #3769
Conversation
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.
We've talked about potential long terms plans to instead record span events as event logs records. This isn't a blocker for stabilizing recording exception as span events since anything we do replacing span events with event log records must be done in a way that allows the spec to evolve gracefully.
Stabilizing this is long overdue.
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.
I agree. It's hard to argue against stabilizing this since it's part of all of the stable SDKs already.
I'm assuming this means we'll mark the exception.*
attributes as stable in the semantic-conventions repo?
I agree with should stabilize. The only question to me is if we are happy with |
In Semconv we use
Yes, we can/should mark some of these as stable. This specification update only outlines what fields are filled out, not what they contain. |
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!
Languages like OTel.NET have shipped this already as part of stable API un-intentionally for a very long time.!
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
Overall LGTM, but I'm slightly wary of the fact the actual semconv values are mentioned at the bottom of this document. Should we remove them altogether and keep the link to their semconv section? |
@jack-berg I did not attend the last Events WG meeting. Have you had a chance to discuss this? |
No we did not. The 11/24 Event WG is cancelled due to US holiday, so the next opportunity is 12/08. Personally, I think there are strong reasons to stabilize this without the agreement of the Event WG, although this is not to say that I anticipate disagreement from the Event WG. I think the Event WG needs to find a way to evolve around this behavior rather than delaying stabilization and shifting course. |
I assume once it's merged we're going to also mark corresponding attributes as stable in the yaml and md in the semconv repo? |
Yes, we'll be looking to stabilize those too. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Not stale, just making sure there was enough time for reviewing before merging it. |
…etry#3769) Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
… breaking ways.
We discussed the current exception specification. Today, the specification defines how exceptions show up in spans. It's been in use for a while and we don't want to create breaking changes. We believe we can evolve the specification we have as needed in non-breaking ways.