Treat bad requests separately from other errors

[tamccall]

Adding custom error interface so requests that do not represent system failures do not count against circuit metrics

Adding here since development in the original package seems to have come to a standstill

See issue in afex#72

[vermapratyush]

Thanks for the PR @tamccall
Can you please update the metricCollector as well. Currently it is used to report metrics to various plugins
https://github.com/myteksi/hystrix-go/blob/master/hystrix/metrics.go#L68

[vermapratyush]

This can be another error like ErrCircuitOpen, ErrMaxConcurrencyetc.
ref: https://github.com/myteksi/hystrix-go/blob/master/hystrix/hystrix.go#L46

[tamccall]

Are you looking for another CircuitError variable? I'm not sure that that is in line with @epivovar original request here as an implementation based on a struct would not allow for the various errors implemented in client packages to be treated as BadRequests themselves

[vermapratyush]

My bad, all this time I though https://godoc.org/github.com/pkg/errors is part of the official errors package.
Here, is my opinion:

Create ErrBadRequest is a type of CircuitError ErrBadRequest = CircuitError{Message: "BadRequest"}
Then a simple comparison err == ErrBadRequest can be used to differentiate between the types of error within hystrix-go package or even outside of it.
Now, in order to surface the original cause of bad request, clients should use errors.Wrap to wrap the original error in ErrBadRequest. It would be possible get the error out from errChan using err.Cause().

Edit: Just looked up the original request seems to me like errors.Wrap is a more extensible compared to the solution mentioned by epivovar.

[tamccall]

I tend to side more with @epivovar. I'm not sure how using errors.Wrap would work to solve this problem, and I think that making BadRequests a concrete type actually makes for a less extensible solution.

I put together this unit tests to try and showcase some of my reservations around making bad request a circuit error. I hope that helps.

//Something like this might work but will still involve type assertions
type circuitErrorWithCause struct {
	CircuitError
	cause error
}

func TestProposal(t *testing.T) {
	badRequest := CircuitError{
		Message: "BadRequest",
	}

	Convey("My issues with the making bad requests a circuit error", t, func() {
		Convey("Wrapped errors will not equal BadRequests", func() {

			wrappedError := errors.Wrap(fmt.Errorf("some bad request"), badRequest.Error())
			So(badRequest == wrappedError, ShouldBeFalse)

		})

		//Here is some code that i could see working
		Convey("Client code is messy", func() {

			//Example handler func
			err := func() error {
				//the client does some work
				err := errors.New("then an error occurs because of bad input")

				//the client then has to check their error to see if it is a bad request
				if err != nil  {
					//then they have to return the bad request so that it doesn't count against the circuit errors
					//this will cover up the original cause
					return badRequest
				}

				return  nil
			}()


			Convey("This is the only way that a simple equality check would work", func() {
				So(err == badRequest, ShouldBeTrue)
			})
		})

		Convey("Adding cause to circuit errors still requires type assertions", func() {
			wrappedError := circuitErrorWithCause{
				CircuitError: badRequest,
				cause: fmt.Errorf("client error"),
			}

			//Example handler func
			err := func() error {
				return wrappedError
			}()

			Convey("Simple Equality check won't work", func() {
				So(err == badRequest, ShouldBeFalse)
			})

			Convey("Would have to assert to the wrapped type so I don't see what we would be gaining", func() {
				assertedType, ok := err.(circuitErrorWithCause)
				So(ok, ShouldBeTrue)
				Convey("You may still need to check the circuit error in the asserted type", func() {
					So(assertedType.CircuitError == badRequest, ShouldBeTrue)
				})
			})
		})
	})
}

Overall i think what you want something more akin to the HystrixBadRequestException which handlers in java can throw themselves or extend in their own Exception implementations.

[tamccall]

What exactly are we looking for with an update to metrics? Do we just want it to start counting BadRequests in addition to the metrics that it currently is reporting?

[vermapratyush]

Yes, metrics to count the number of BadRequest needs to be added so that the plugins/ and eventstream.go reflect the correct state of command execution.

eventstream.go is backward compatible with the Netflix's version of dashboard (https://github.com/Netflix/Hystrix/wiki/Dashboard)

Edit: This may require adding a new attribute (rollingCountBadRequests) in the struct streamCmdRollingCountMetric
ref: https://github.com/Netflix/Hystrix/blob/master/hystrix-dashboard/src/main/webapp/components/hystrixCommand/hystrixCommand.js#L169

[tamccall]

I'll hold off on implementing the metrics untill that we can agree on the best implementation of bad request, but more generally i think metric reporting may need to be rethought in this package since every new metric seems to break the metric collector interface. See comments here: afex#75 (comment)