Add exception.event event attribute. #747
Conversation
Oberon00
added
area:error-reporting
|
Can you provide a use-case or some example when and how this new field could be used? |
|
A handled exception is something entirely different from a thrown exception, I think we need a way to distinguish this. So the use case would be to display this information in the backend and use it in error analysis. I will add some examples. 👍 |
|
@iNikem Since you are familiar with the original exception attributes, what kinds of exception events do you expect to be recorded? |
|
|
||
| static void someMethodThatMayThrow() { | ||
| someMethodWithCleanup(); | ||
| } // Instrumentation may record a *rethrown* exception here |
There was a problem hiding this comment.
I wonder if we should have a special escaped event for this special case where an exception was detected to leave a function by an instrumentation, even though there is no catch block in it (the instrumentation may have added one).
|
My biggest concern about this PR is that I cannot imagine how auto-instrumentations will provide this information. E.g. I detect that exception was thrown, I record that on the current span right away. Next that exception was actually handled, but I cannot change that attribute any more, event is already recorded. And that exception handling can occur in totally different class/method, although still during the same span. Do you propose to call |
|
No, actually I recommend to only record thrown and maybe rethrown (please read the diff). I also expect that the other enum values will not see much use (EDIT: Except for manual instrumentation). Part of the reason for them to exist to make clear what does not count as (re)thrown. |
If you want to record both events, you should create two events for the same exception object. If not, this PR recommends to record only thrown. EDIT: Quoting the PR:
But if you want to record both events, now you can.
If you want to record multiple events for the same exception object (which this PR does not recommend), then yes, that's how I imagine it. After, all as I said in the PR description
|
There was a problem hiding this comment.
I agree with @iNikem, it's not even really about auto instrumentation, but any library instrumentation will generally only be able to receive errors based on some error handler or similar callback. Without the actual library providing this information, I don't see how instrumentation could ever derive it except for very rare cases.
|
Instrumentations will know if they last saw the exception in flight or being handled/swallowed (often they only can detect events of he former kind, e.g. by wrapping a fumction in a try/catch). I think my current choice of enum values may be poor, but this binary distinction is what I care about most. |
|
Ok,you want to distinguish two classes of exceptions: handled and not handled. But even inside one span there may be significant distance between where exception is thrown and where it is caught and handled. That distance may even cross library boundaries. For auto-instrumentation (and for library authors in fact) this means a choice between two ways to handle exception. One is extremely simple: you see exception, you record it. Another is much more complicated: you see exception, wait until it gets caught/handled or thrown outside of the span boundaries, then decide which event to record. I hope to be mistaken, but at the moment I think this proposal will significantly complicate my job as auto-instrumentation maintainer without that much of benefit for an end-user. |
In my past in Plumbr we looked at how far exceptions/error statuses got propagated up from the span where they were created. This gave us almost the same information, IMHO. |
Why do you think this proposal changes this? |
|
A typical auto-instrumentation will wrap things in catch blocks and record throw exceptions, then rethrow. That's it. The only difference with this proposal would be that it would additionally set |
Won't this information usually be in the documentation in the callback? Sometimes you can even do things like return a boolean whether to rethrow or swallow the exception (instrumentation would then indicate rethrow I guess and write |
And then this exception will be caught by the method above and handled. If end user relies on different events "thrown/handled" to reflect what has actually happened, the above situation will be misleading. I think my objection boils down to the following. Current |
I am not suggesting to change that.
Instrumentation can only tell what happened at the point where it saw the exception. It will not be able to tell what happened later. I think you object because you think that I want this to change, but I don't. I think you think this because the enum would be mostly useless otherwise, and I'm starting to agree. I will probably replace this WIP PR with a new one that simply adds the requirement:
|
Yes, that is correct summary of my thoughts :)
Do you mean here that if auto-instrumentation does not know what will happen in the future with this exception, then it is Ok for it to record this exception? I hope so :) Bun on the other hand, why do you want to forbid application developers to record arbitrary exceptions, even handled ones, if they wish so? |
Hmm, for that case something like the enum would be useful 🤔 Or maybe a boolean |
Yes, because I think the typical case for auto-instrumentation would be to observe an exception flying out of some function, and having no idea what happens afterwards. This case should definitely be allowed and simple. |
|
Superseded by #761 |
Exceptions themselves are objects, not events, but several events related to them can happen. This PR adds a required semantic convention attribute to describe which one it was.
Be sure to read the discussion at #697 (comment).