Should Span.RecordException() support more options or be replaced by an optional AddEvent() parameter?

[Oberon00]

Required for #784, #822

From #784 (comment)

RecordException is currently rather inflexible:

• Many PRs Add exception.escaped attribute. #784, Add error.hint semantic convention #822, Add SeverityNumber to Span Events #698 (comment) proposed or were proposing additional parameters to the API. If we just add all of them, the API could become very unwieldy. Do we take this risk (assuming no further PRs will want to modify RecordException) or do we change RecordException to be more scalable? Should we leave this to the languages? E.g. in some with named and default parameters, a large number of parameters might not be seen as a problem at all, in others a Builder pattern could be used.
• Nice to have: The API for RecordException could provide options to (not) record the stacktrace. In languages with typed exceptions, the message may also be made optional. Additionally, users might want to add additional attributes to the event. Should a list/map of attributes be added as additional optional parameter?

[Oberon00]

CC @bogdandrutu @carlosalberto @tigrannajaryan

[tigrannajaryan]

Also related proposal to add severity to RecordException: #697 (comment)

[Oberon00]

Since #822 is now the third (second?) PR that wants to add a parameter to RecordExcetpion, I suggest a different solution: Remove RecordException and replace it with an optional parameter Exception to AddEvent.

[Oberon00]

I was tempted to add required-for-ga, but RecordException seems to be an API that is a tiny burden to support if we decide to (additionally) have the Exception parameter for AddEvent.

[iNikem]

That would be fast: removing an API in like, a month, after adding it?

[Oberon00]

There has been no spec release since adding the API.

[iNikem]

Look at this PR this way. Error WG has meet several times, discussed the current variety of proposals and came up with #697 which expressed the consensus of that Working Group. Now a person outside that group makes a proposal to rollback that decision, already accepted and merged to spec, without discussing that with the working group.

Error Working Group is still active, we meet every week and discuss further work. #822 is one such step further. If you are interested in this topic (as your activity indicates), please come to a meeting and participate in WG job.

[mwear]

I think there could be some value in allowing it to accept additional options, depending on the use case, like setting relevant span level attributes. For example, #822 recommends adding the ability to set a span level attribute error.hint as a way to encourage (and help) developers annotate a span with as much relevant data as possible. I can also see arguments for keeping the API lean and focused.

[Oberon00]

@iNikem

Now a person outside that group makes a proposal to rollback that decision

Sorry, I don't propose to remove the functionality! I actually think having a convenience method to record exceptions is essential. In #814 (comment) I was just thinking of adding the exception parameter as argument to AddEvent. Then you could do something like mySpan.addEvent(Event.builder().setException(myException).setAttribute("exception.escaped", true)) (or my_span.add_event(exeption=myException, attributes={"exception.escaped": True})`, depending on the language)

[Oberon00]

@mwear

like setting relevant span level attributes

I think recording an event and setting, possibly overriding, a span attribute in the same method call is potentially confusing. Recording an event is simple, as you don't have to think about how subsequent calls to SetAttribute, AddEvent, RecordException, ... will interact with this call: every event is isolated and immutable. If you have "append (an event)" semantics for some parameters and "set/override (an attribute)" for others, this can lead to surprises.

[Oberon00]

I updated the issue description.

[iNikem]

We have a general purpose Span.AddEvent method. And we have a convenience methods Span.RecordException. This is good and does not need to change, IMO.

Thus the question seems to be: what other convenience methods we should/want to add, if any? We currently have two proposals to add different boolean flags to Span.RecordException. In the future there may be more. From API evolution point of view the most sensible decision would be to postpone any API change until later, when more people use API in real life and can give as feedback about their needs and wants.

Thus I propose not to change API right now. #822 can easily be reduced to just semantic convention. #784 can potentially be reduced to semantic convention on events and usage of general purpose Span.AddEvent when that attribute should have non-default value.

[Oberon00]

The problem is that AddEvent does not support exceptions currently. I don't think specifying the exception attributes manually (especially stacktrace, qualified type name, ...) is something we want users to do manually. So I don't think you can call RecordException a "convenience" method. It is definitely not a primitive, but it does do some actual work beyond just providing syntax sugar.

#784 can potentially be reduced to semantic convention on events and usage of general purpose Span.AddEvent

I don't think I want to basically write out a full re-implementation of RecordException just to set exception.escaped. I'd rather not set the attribute if I were a user then.