Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support context and ignore circuit metrics for canceled contexts #79

Merged
merged 5 commits into from
Mar 29, 2018

Conversation

brildum
Copy link

@brildum brildum commented Jan 5, 2018

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 brildum closed this Jan 5, 2018
@brildum
Copy link
Author

brildum commented Jan 5, 2018

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.

@brildum brildum reopened this Jan 5, 2018
@afex
Copy link
Owner

afex commented Mar 29, 2018

thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants