From 9f62aff5dbb3e6575d4009fdedc61ad445da7d96 Mon Sep 17 00:00:00 2001 From: Ulf Gitschthaler Date: Fri, 13 Apr 2018 17:04:54 +0200 Subject: [PATCH 1/2] 4xx http status code should not trigger a bad_request event --- src/clj_http_hystrix/core.clj | 26 +++-------- test/clj_http_hystrix/core_test.clj | 67 +++++++++++------------------ 2 files changed, 32 insertions(+), 61 deletions(-) diff --git a/src/clj_http_hystrix/core.clj b/src/clj_http_hystrix/core.clj index 989f37e..bb4b708 100644 --- a/src/clj_http_hystrix/core.clj +++ b/src/clj_http_hystrix/core.clj @@ -5,11 +5,11 @@ [slingshot.slingshot :refer [get-thrown-object]] [slingshot.support :refer [wrap stack-trace]]) (:import [com.netflix.hystrix HystrixCommand - HystrixThreadPoolProperties - HystrixCommandProperties - HystrixCommand$Setter - HystrixCommandGroupKey$Factory - HystrixCommandKey$Factory] + HystrixThreadPoolProperties + HystrixCommandProperties + HystrixCommand$Setter + HystrixCommandGroupKey$Factory + HystrixCommandKey$Factory] [com.netflix.hystrix.exception HystrixBadRequestException] [org.slf4j MDC])) @@ -43,8 +43,7 @@ :hystrix/timeout-ms 1000 :hystrix/breaker-request-volume 20 :hystrix/breaker-error-percent 50 - :hystrix/breaker-sleep-window-ms 5000 - :hystrix/bad-request-pred client-error?}) + :hystrix/breaker-sleep-window-ms 5000}) (def ^:private hystrix-keys (keys hystrix-base-configuration)) @@ -109,20 +108,10 @@ (fn [f req] (if (not-empty (select-keys req hystrix-keys)) (let [req (merge defaults req) - bad-request-pred (:hystrix/bad-request-pred req) fallback (:hystrix/fallback-fn req) - wrap-bad-request (fn [resp] - (if (bad-request-pred req resp) - (throw - (HystrixBadRequestException. - "Ignored bad request" - (wrap {:object resp - :message "Bad request pred" - :stack-trace (stack-trace)}))) - resp)) wrap-exception-response (fn [resp] ((http/wrap-exceptions (constantly resp)) - (assoc req :throw-exceptions true))) + (assoc req :throw-exceptions (not (client-error? req resp))))) configurator (configurator req) logging-context (or (MDC/getCopyOfContextMap) {}) command (proxy [HystrixCommand] [configurator] @@ -137,7 +126,6 @@ (-> req (assoc :throw-exceptions false) f - wrap-bad-request wrap-exception-response)))] (handle-exception #(.execute command) req)) (f req))))) diff --git a/test/clj_http_hystrix/core_test.clj b/test/clj_http_hystrix/core_test.clj index 3144702..474d6a0 100644 --- a/test/clj_http_hystrix/core_test.clj +++ b/test/clj_http_hystrix/core_test.clj @@ -113,7 +113,7 @@ :throw-exceptions false})] (:status response) => 503))) -(fact "errors will not cause circuit to break if bad-request-pred is true, with :throw-exceptions false" +(fact "errors will not cause circuit to break if client-error? is true, with :throw-exceptions false" (rest-driven [{:method :GET :url "/"} @@ -125,13 +125,12 @@ (let [command-key (keyword (str (UUID/randomUUID)))] (dotimes [_ 30] (http/get url {:throw-exceptions false - :hystrix/command-key command-key - :hystrix/bad-request-pred client-error?}) => (contains {:status 400})) + :hystrix/command-key command-key}) => (contains {:status 400})) (Thread/sleep 600) ;sleep to wait for Hystrix health snapshot (http/get url {:throw-exceptions false :hystrix/command-key command-key}) => (contains {:status 200})))) -(fact "errors will not cause circuit to break if bad-request-pred is true, with :throw-exceptions true" +(fact "errors will not cause circuit to break if client-error? is true, with :throw-exceptions true" (rest-driven [{:method :GET :url "/"} @@ -143,27 +142,11 @@ (let [command-key (keyword (str (UUID/randomUUID)))] (dotimes [_ 30] (http/get url {:throw-exceptions true - :hystrix/command-key command-key - :hystrix/bad-request-pred client-error?}) => (throws ExceptionInfo)) + :hystrix/command-key command-key}) => (throws ExceptionInfo)) (Thread/sleep 600) ;sleep to wait for Hystrix health snapshot (http/get url {:throw-exceptions false :hystrix/command-key command-key}) => (contains {:status 200})))) -(fact "errors will cause circuit to break if bad-request-pred is false" - (rest-driven - [{:method :GET - :url "/"} - {:status 400 - :times 30}] - (let [command-key (keyword (str (UUID/randomUUID)))] - (dotimes [_ 30] - (http/get url {:throw-exceptions false - :hystrix/command-key command-key - :hystrix/bad-request-pred (constantly false)}) => (contains {:status 400})) - (Thread/sleep 600) ;sleep to wait for Hystrix health snapshot - (http/get url {:throw-exceptions false - :hystrix/command-key command-key}) => (contains {:status 503})))) - (fact "status-codes predicate matches only given status codes" (let [predicate (status-codes 100 200 300)] (predicate {} {:status 100}) => true @@ -197,17 +180,17 @@ (fact "add-hook with user-defaults will override base configuration, but not call configuration" (rest-driven - [{:method :GET - :url "/"} - {:status 500 - :times 3}] - (make-hystrix-call {}) - => (throws clojure.lang.ExceptionInfo "clj-http: status 500") + [{:method :GET + :url "/"} + {:status 500 + :times 3}] + (make-hystrix-call {}) + => (throws clojure.lang.ExceptionInfo "clj-http: status 500") ;set custom default for fallback-fn - (remove-hook) - (add-hook {:hystrix/fallback-fn (constantly "bar")}) - (make-hystrix-call {}) => "bar" - (make-hystrix-call {:hystrix/fallback-fn (constantly "baz")}) => "baz") + (remove-hook) + (add-hook {:hystrix/fallback-fn (constantly "bar")}) + (make-hystrix-call {}) => "bar" + (make-hystrix-call {:hystrix/fallback-fn (constantly "baz")}) => "baz") (remove-hook) (add-hook)) @@ -216,19 +199,19 @@ ;verify hystrix is enabled by exceeding the default timeout (1000 ms) (http/with-additional-middleware [wrap-hystrix] (rest-driven - [{:method :GET - :url "/"} - {:status 200 - :after 1500}] - (make-hystrix-call {}) - => (throws clojure.lang.ExceptionInfo "clj-http: status 503"))) + [{:method :GET + :url "/"} + {:status 200 + :after 1500}] + (make-hystrix-call {}) + => (throws clojure.lang.ExceptionInfo "clj-http: status 503"))) ;verify custom defaults are supported (http/with-additional-middleware [(partial wrap-hystrix {:hystrix/fallback-fn (constantly {:status 404})})] (rest-driven - [{:method :GET - :url "/"} - {:status 500}] - (make-hystrix-call {}) - => (throws clojure.lang.ExceptionInfo "clj-http: status 404")))) + [{:method :GET + :url "/"} + {:status 500}] + (make-hystrix-call {}) + => (throws clojure.lang.ExceptionInfo "clj-http: status 404")))) From 37485c8c3f8bd13f2f07641b033fcc76069e387e Mon Sep 17 00:00:00 2001 From: Ulf Gitschthaler Date: Mon, 16 Apr 2018 13:24:27 +0200 Subject: [PATCH 2/2] Add bad-request-pred function again --- src/clj_http_hystrix/core.clj | 6 ++++-- test/clj_http_hystrix/core_test.clj | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/clj_http_hystrix/core.clj b/src/clj_http_hystrix/core.clj index bb4b708..7a61932 100644 --- a/src/clj_http_hystrix/core.clj +++ b/src/clj_http_hystrix/core.clj @@ -43,7 +43,8 @@ :hystrix/timeout-ms 1000 :hystrix/breaker-request-volume 20 :hystrix/breaker-error-percent 50 - :hystrix/breaker-sleep-window-ms 5000}) + :hystrix/breaker-sleep-window-ms 5000 + :hystrix/bad-request-pred client-error?}) (def ^:private hystrix-keys (keys hystrix-base-configuration)) @@ -108,10 +109,11 @@ (fn [f req] (if (not-empty (select-keys req hystrix-keys)) (let [req (merge defaults req) + bad-request-pred (:hystrix/bad-request-pred req) fallback (:hystrix/fallback-fn req) wrap-exception-response (fn [resp] ((http/wrap-exceptions (constantly resp)) - (assoc req :throw-exceptions (not (client-error? req resp))))) + (assoc req :throw-exceptions (not (bad-request-pred req resp))))) configurator (configurator req) logging-context (or (MDC/getCopyOfContextMap) {}) command (proxy [HystrixCommand] [configurator] diff --git a/test/clj_http_hystrix/core_test.clj b/test/clj_http_hystrix/core_test.clj index 474d6a0..d358a64 100644 --- a/test/clj_http_hystrix/core_test.clj +++ b/test/clj_http_hystrix/core_test.clj @@ -113,7 +113,7 @@ :throw-exceptions false})] (:status response) => 503))) -(fact "errors will not cause circuit to break if client-error? is true, with :throw-exceptions false" +(fact "errors will not cause circuit to break if bad-request-pred is true, with :throw-exceptions false" (rest-driven [{:method :GET :url "/"} @@ -130,7 +130,7 @@ (http/get url {:throw-exceptions false :hystrix/command-key command-key}) => (contains {:status 200})))) -(fact "errors will not cause circuit to break if client-error? is true, with :throw-exceptions true" +(fact "errors will not cause circuit to break if bad-request-pred is true, with :throw-exceptions true" (rest-driven [{:method :GET :url "/"}