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.

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

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Nov 15, 2023

… 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.

@jsuereth jsuereth requested review from a team November 15, 2023 19:58
Copy link
Member

@jack-berg jack-berg left a 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.

Copy link
Member

@trask trask left a 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?

@tigrannajaryan
Copy link
Member

I agree with should stabilize. The only question to me is if we are happy with exception. as the namespace. This came up in a recent discussion about events. One possible alternate surfaced was runtime.exception.

@jsuereth
Copy link
Contributor Author

jsuereth commented Nov 15, 2023

I agree with should stabilize. The only question to me is if we are happy with exception. as the namespace. This came up in a recent discussion about events. One possible alternate surfaced was runtime.exception.

In Semconv we use reasonably unique namespaces that are meant to be minimal. runtime carries a lot of baggage here and walks into the errors vs. exceptions space. I don't think that discussion is worth revisiting again.

I'm assuming this means we'll mark the exception.* attributes as stable in the semantic-conventions repo?

Yes, we can/should mark some of these as stable. This specification update only outlines what fields are filled out, not what they contain.

Copy link
Member

@cijothomas cijothomas left a 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.!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
@carlosalberto
Copy link
Contributor

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?

@tigrannajaryan
Copy link
Member

@jack-berg I did not attend the last Events WG meeting. Have you had a chance to discuss this?

@jack-berg
Copy link
Member

@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.

@lmolkova
Copy link
Contributor

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?

@jsuereth
Copy link
Contributor Author

@lmolkova See #3769 (comment)

Yes, we'll be looking to stabilize those too.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 29, 2023
@arminru
Copy link
Member

arminru commented Nov 29, 2023

Not stale, just making sure there was enough time for reviewing before merging it.

@arminru arminru removed the Stale label Nov 29, 2023
@arminru arminru merged commit 77405db into open-telemetry:main Dec 1, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.