-
Notifications
You must be signed in to change notification settings - Fork 295
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
Interceptors: provide exception class when there is no message #526
Conversation
@@ -30,7 +30,8 @@ | |||
(get interceptor :name (pr-str interceptor))) | |||
|
|||
(defn- throwable->ex-info [^Throwable t execution-id interceptor stage] | |||
(ex-info (str "Interceptor Exception: " (.getMessage t)) | |||
(ex-info (str "Interceptor Exception: " (or (.getMessage t) |
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 the intent to also fall-back to the exc class name if user supplied an empty message?
e.g: if the user did (SomeException. "")
?
In which case, perhaps need to do something like (non-empty (.getMessage t))
.
Otherwise, looks fine.
Thanks for pulling this together! I'm going to address it in a few separate changes shortly. |
Sounds great. Anything that improves these error messages would be a big improvement cutting down time to diagnose issues. :) |
Just so you're aware. There is a lot more information in the ex-data than just in the log message. Often times what I do is setup the error handler interceptor to catch and format exceptions while I'm developing (adding to its patterns as business cases come up) |
This improvement is now on master. Thanks so much! |
Very nice, thanks! |
Excellent to hear, you're very welcome! |
@ohpauleez I'm not sure how feasible this is but — following situation: I have a bunch of interceptors and they also carry some additional data. Now when an exception occurs I'd like to access the full interceptor to be able to inspect that data. For this reason it would be handy to be able to obtain a reference to the first-throwing interceptor in the error handler. One way of doing that would be providing my own |
I'll look into it today. Certain feasible, just have to see: A.) Does it make sense to do that (vs something else, like giving better access to the context, so you know what interceptor fired based on that) B.) Do any changes to how data is packaged up break the error-handler interceptor C.) Should we advise developers against smugglng additional data in the interceptor itself :) Could you elaborate on what kind of data you're smuggling in the interceptor and why you chose to do that over alternative options? |
Sure.
For interceptors that run without throwing I store originating data structure in the context under Hope this helps, if you want to discuss more also feel free to ping me in the Clojurians Slack (same nick). EDIT Oh, and I vote against option C 😛 |
This would make serializing exceptions more tricky and so I'm not sure if that's a good path to go down but a simple/obvious solution could just store the entire interceptor in |
Hey @ohpauleez — was just wondering if you came up with any further thoughts on this? Would be happy to come up with a PR but I guess code is the easier part here :) |
Hi @martinklepsch -- Sorry it's taken me a bit to follow up here! When you turn the data structures into Interceptors, have you considered programmatically generating names for them? This might help you when debugging and handling errors. For example, Vase requires names in the data structure (a very direct approach), but you might consider adding unique pieces instead of calling them all Smuggling interceptors through to errorsIn the past, I've had success smuggling the interceptor into the data of an ex-info generated exception. If you take this approach your "erroring/failing" interceptor needs to catch the core exception and rethrow it. You can get the "current" interceptor from the context by looking at the top of the stack,
|
Thanks @ohpauleez, np about the wait :) Generating names programatically is something I considered but the configuration I hand to those interceptors is not encodable inside a keyword (e.g. some take URLs as parameters). The advice with catching & rethrowing is good and I might use that. That said this still seems useful to be included in the library by default & as you mentioned you've used this hack yourself so seems you also came up with a situation where you needed it :) |
@martinklepsch I haven't needed it since I introduced That said, I appreciate the feedback, conversation, and support. We'll make sure the suggestion gets captured on the backlog. Thanks! |
Some exceptions don't have an exception message which leads to (unhelpful) exception messages like
This change falls back to the exception class if there's no message, common cases could be arithmetic exceptions or
(rand-nth [])
. With the proposed change the above would change to this:We also see
Interceptor Exception: Interceptor Exception: ...
lots of times, not sure if that is intended or not?