afex / hystrix-go

Netflix's Hystrix latency and fault tolerance library, for Go
MIT License
4.24k stars 477 forks source link

support context and ignore circuit metrics for canceled contexts #79

Closed brildum closed 6 years ago

brildum commented 6 years ago

This is an attempt to appropriately support context in hystrix-go.

It adds to new functions to the hystrix package: GoC and DoC which behave the same as Go and Do but take context as the first parameter.

It keeps backwards compatibility by adapting Go and Do and calling GoC and DoC with context.Background().

The most controversial choice is how I've chosen to deal with contexts. I've chosen to treat context errors separately from command errors. That is -- if the context passed in is canceled or deadline exceeded, we won't modify any circuit breaker metrics. The command in this case did not fail, just the computation is no longer relevant.

This has the side effect of not triggering any 'Attempt' metrics in the case of context cancelation, which seems wrong, but avoids a mismatch between attempts and other failure conditions. We could consider adding a new metric 'cancels' and trigger 'attempts' and 'cancels' in the case we detect context canceled.

brildum commented 6 years ago

On further thought -- I'd argue that any time the context passed to hystrix has a cancelation or has exceeded its deadline, it shouldn't trigger circuit breaker increments -- because it is not the command itself that triggered the error but something else.

I've updated this branch to not modify circuit breaker metrics if the context is done before the command completes.

afex commented 6 years ago

thanks for the contribution!