count exceptions #49
count exceptions #49
Conversation
|
replaces #48 |
|
@slipset I opened this PR instead of the other one
so either way the exception is eventually thrown - but if exception-status is provided it will be recorded before being thrown what do you think? I am still missing tests and documentation - will complete them when we agree on the changes |
|
solves #47 |
src/iapetos/collector/ring.clj
Outdated
| (cond (nil? response) {:status 404} | ||
| (exception? response) {:status (:exception-status options)} |
There was a problem hiding this comment.
I'd rather you pass exception-status as a param instead of the whole options map
There was a problem hiding this comment.
solved by cf51b3b
src/iapetos/collector/ring.clj
Outdated
| (defn- run-instrumented | ||
| [{:keys [handler] :as options} request] | ||
| (ex/with-exceptions (exception-counter-for options request) | ||
| (let [start-time (System/nanoTime) | ||
| response (handler request) | ||
| response (safe options #(handler request)) |
There was a problem hiding this comment.
I don't think you need safe, I think it suffices to do
response (try (handler request) (catch Exception e e))There was a problem hiding this comment.
we can do this but it introduces a subtle difference in behavior:
it means we will always catch an exception thrown in (handler request) and this exception will always be counted in the totals
it also means we will have to provide some default value to :exception-status since now there is no conditional relying on this value being provided
what do you think?
There was a problem hiding this comment.
If you could pass exception status as a param like
(defn- maybe-safe [catch-exceptions? f]
(if catch-exceptions?
(try (f) (catch Exception e e))
(f)))That'd be great.
You could then call it with (safe (:exception-status options) #(handler request))
src/iapetos/collector/ring.clj
Outdated
| (record-metrics! options delta request)) | ||
| (when (exception? response) (throw response)) |
There was a problem hiding this comment.
(if-not (exception? response) response (throw exception))There was a problem hiding this comment.
resolved by 370a50e
|
@slipset thx |
|
@slipset I added tests and documentation in the readme |
|
It looks good now I think. @MalloZup you have anything? |
|
@slipset just to tie this up - what is the release schedule for this library? |
|
Released as |
thanks! |
…mmons#49) The old behaviour was not to report thrown exceptions as 5XX status class to the http_requests_total metric. This change gives the client the option to count exceptions in the total as well as exceptions. Please refer to the Readme for further details.
No description provided.