Invalid bad_request and exception_thrown events in case of client-error

[u6f6o]

We use prometheus together with prometheus-hystrix in order to gather data for our Grafana dashboards and to trigger alarms in case something weird happens. Lately we also added some alarm triggers for our clojure projects that use clj-http-hystrix underneath.

As we use standard REST calls, HTTP status codes like 404 appear quite regularly. We noticed though that our clojure services (in opposite to the java services) trigger certain hystrix events ("bad_request" and "exception_thrown") that caused our Grafana dashboards to look quite bad:

I figured out that these Hystrix events are triggered due to HystrixBadRequestExceptions that are thrown here in case http/client-error? returns false. I believe this is a wrong behaviour though, as the javdoc states:

NOTE: This should only be used when an error is due to user input such as IllegalArgumentException otherwise it defeats the purpose of fault-tolerance and fallback behavior.

Also there is a test in the hystrix' core library that indicates that this exception should be used for different purposes.

[u6f6o]

I also added a PR to fix this behaviour: #14

[joelittlejohn]

Hi @u6f6o. If I understand it correctly, the quote from the Hystrix javadoc is implying that one should not be too broad in applying the HystrixBadRequestException because it means that circuit breaking will rarely happen. Your PR removes circuit breaking behaviour from all client errors (so these are always considered to be normal responses) which also "defeats the purpose of fault-tolerance and fallback behaviour". In the javadoc, the Hystrix team is recommending that you should avoid categorising too many types of error as client errors, so your PR which will avoid categorising 4xx as any kind of error at all (bad request or otherwise) could receive the same criticism I think :)

I understand the problem here though - you consider many 4xx responses as normal and not client errors, so you don't want the statistics to show these as 'bad requests'. I don't think we should force users of this library to treat all 4xx responses as normal errors though.

I see you have updated your PR so that the bad-request-pred can still be used to categories some errors as 'normal' and some as 'exceptional'. Could you reinstate this test:

(fact "errors will cause circuit to break if bad-request-pred is false"

I think it's still valid.

[u6f6o]

Hi @joelittlejohn, Thx for your remarks. I'll have a look at the particular fact. I think you are right; while in my specific case 4xx status codes might be the normal case, it most likely will not be true for all users. Maybe we could think about a solution that could satisfy both scenarios (treat 4xx as success vs. indicate a bad request).

Skimming through the hystrix code a bit, I realised that the HystriBadRequestException is mostly seen as an ignorable exception and thus would not automatically trigger a circuit open. Think I have to check it again :-)

[joelittlejohn]

Although it complicates the docs a bit, I think the only complete solution here (if you want to remove that bad request count from your dashboard) is to offer two predicates. One predicate that identifies all 'failed' requests, and another predicate that can categorise some unsuccessful requests as 'bad request' instead of errors in the service being called.

By default, failure-pred would be (complement http.client/success?) and bad-request-pred would be http.client/client-error?.

In your organisation, you would set failure-pred to http.client/server-error?.

[u6f6o]

Sounds reasonable to me, will try to find time on the weekend.