A way to ignore certain type of errors

[RidgeA]

Hello!

I use the request module with the opossum to communicate between microservices.

In case if other microservice returns HTTP 4xx error, this is treated as an error and could lead to breaker opening. But in major cases, 4xx HTTP status is a valid and expected result.

So, I need somehow to tell opossum to ignore certain types of error and return them as is and don't treat them as an error.

Is it feasible?

[lance]

All opossum does is proxy calls to the function it is provided. You could write a function that wraps your request calls, checking for acceptable responses and return a resolved promise when success or a rejected promise when failure.

[RidgeA]

Yeah, I could do this.
But 4xx errors I want to pass through opossum and return this response as an error (the same as without opossum) but without changing internal circuit breaker's stats. The subsequent code expects to receive an error in this case.
Other errors (such as 5xx or network errors) should lead to breaker opening.

[ComputerWolf]

I agree that a 4xx error does not necessarily mean that the service is unhealthy and should be configurable to not count as a failure, but it still makes sense to pass the error through as expected.

[lance]

I really think this is in the domain of the client application and not something that opossum should be doing. It does not make sense to inspect the return value of all functions protected by a circuit breaker, determine if the function is a remote call receiving a 404 and then performing some special behavior. This is code that belongs in userland, not the module.

[ComputerWolf]

@lance, while agree that any logic should happen outside of the module, right now it is all or nothing. There is no way to conditionally decide if a response should trip the circuit breaker and I would argue that there are some cases where a response should bypass the breaker. In those cases it is up to the module to provide the functionality to do so.

[RidgeA]

I totally agree that opossum should not inspect the errors and make a decision on to count this as an error or not. The logic should belong to called code and depends on many factors and business logic.

Let's imagine the following example (pretty close to that I encountered with):

const request = require("request-promise-native")
const {StatusCodeError} = require("request-promise-native/errors")

//....

let result;

//....

try {
   result = await request(url);
   //....
} catch (e) {
   if (e instanceof StatusCodeError && e.statusCode === 404)  {
      // an expected outcome - just to return an empty collection
      result = [];
   } else {
      // could be any other 4xx error or any of 5xx errors or any network error
      // this in an unexpected case and the error should be re-thrown
      throw e;
   }
}

Now I've got a task to add a circuit breaker to the request. I change the code:

const request = require("request-promise-native");
const {StatusCodeError} = require("request-promise-native/errors");
const opossum = require("opossum");

cosnt safeRequest = opossum(request, {});
//....

let result;

//....

try {
   result = await safeRequest(url).fire();
   //....
} catch (e) {
   if (e instanceof StatusCodeError && e.statusCode === 404)  {
      // expected outcome - just to return an empty collection
      result = [];
   } else {
      // could be any other 4xx error or any 5xx error or any network error - this in an unexpected case and the error should be re-thrown
      throw e;
   }
}

In this case StatusCodeError, that will be thrown by the request that wrapped with opossum will cause incrementing statistics counters.
With the default options, this will cause breaker opening and next call wont happen at all and will return Breaker is open` error.

This behaviour could be mitigated by configuration - volumeThreshold option, but this is rather hiding an error than an actual solution.

To my opinion, this could be managed in such a way:

1. Add to the CircuitBreaker one more option, e.g. errorHandler that will be a function with signature, for instance, f(err): boolean
2. the function could be called in

   opossum/lib/circuit.js

   Line 543 in b0d4488

   function fail (circuit, err, args, latency) {

   function
3. if errorHandler return true - the statistics shouldn't be incremented and this error shouldn't lead to opening the breaker if the errorHandler return false - the error is treated as unexpected and should lead to breaker opening.

With the changes, described above, the code would be look like:

const request = require("request-promise-native");
const {StatusCodeError} = require("request-promise-native/errors");
const opossum = require("opossum");

cosnt safeRequest = opossum(request, {
  errorHandler: funciton (error) {
    return error instanceof StatusCodeError && error.statusCode === 404;
  }
});
//....

let result;

//....

try {
   result = await safeRequest(url).fire();
   //....
} catch (e) {
   if (e instanceof StatusCodeError && e.statusCode === 404)  {
      // expected outcome - just to return an empty collection
      result = [];
   } else {
      // could be any other 4xx error or any 5xx error or any network error - this in an unexpected case and the error should be re-thrown
      throw e;
   }
}

errorHandler - is just a name from the top of my head, I don't like the name, but nothing better don't come to my mind right now.

I already made similar changes and need to polish the code and naming. After that, I would be ready to introduce PR with the feature, if you would accept it.

errorHandler could be not a function - it could be an instance of some class if you prefer such an approach.

I not a Java-guy, but AFAIK Hystrix allows for user's code to bypass metrics by wrapping an exception by wrapping the error with https://netflix.github.io/Hystrix/javadoc/com/netflix/hystrix/exception/HystrixBadRequestException.html

[lance]

OK - I can see the value of this. I'll reopen and give it some thought.

[lance]

@RidgeA after more thought and reading your suggestion a bit more carefully, yes I would definitely accept this change. I would like to see tests and docs along with it as well, please. If you want to submit what you have now as as a work in progress PR, that's fine. As for naming, I don't like errorHandler too much either, but if docs are good it doesn't really matter. Thank you for contributing!

[jahredhope]

I'm attempting to use opossum on a Axios HTTP client and the ability to only open the circuit on 5xx errors is pretty important.

Not sure I like errorHandler but I'm not sure I have a better name. Perhaps incrementError or something?

The signature that @RidgeA presented looks good: (error: Error) => boolean

const httpClient = () => {
  const breaker = opossum(axiosClient, {
    // Return true if the error
    incrementError: error => error.statusCode <= 500 && error.statusCode <= 599 &
  });


  return (...params) => breaker.fire(...params);
}

• Has anyone started work on this @RidgeA or @lance ?
• If so, thanks!
• If not, are you open to a PR?

[lance]

@jahredhope thanks for your offer of a PR. I had this on my todo list for tonight, and it was a pretty simple change. Please have a look at it. #281