motiv-labs / janus

An API Gateway written in Go
https://hellofresh.gitbooks.io/janus
MIT License
2.8k stars 320 forks source link

PTII-145 Feature circuit breaker #269

Closed c4pone closed 6 years ago

c4pone commented 6 years ago

What does this PR do?

This will enable circuit breakers for each proxy

codecov[bot] commented 6 years ago

Codecov Report

Merging #269 into master will increase coverage by 0.4%. The diff coverage is 66.03%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #269     +/-   ##
=========================================
+ Coverage   45.32%   45.72%   +0.4%     
=========================================
  Files          68       67      -1     
  Lines        2533     2539      +6     
=========================================
+ Hits         1148     1161     +13     
+ Misses       1325     1322      -3     
+ Partials       60       56      -4
Impacted Files Coverage Δ
pkg/api/api.go 85.71% <ø> (ø) :arrow_up:
pkg/proxy/register.go 0% <0%> (ø) :arrow_up:
pkg/proxy/definition.go 64.7% <0%> (ø) :arrow_up:
pkg/proxy/transport.go 0% <0%> (ø) :arrow_up:
pkg/loader/loader.go 57.14% <100%> (+17.14%) :arrow_up:
pkg/loader/api_loader.go 88.33% <100%> (+3.55%) :arrow_up:
pkg/config/specification.go 57.37% <58.33%> (+0.23%) :arrow_up:
pkg/plugin/oauth2/manager_factory.go 0% <0%> (ø) :arrow_up:
pkg/plugin/oauth2/oauth.go 79.48% <0%> (ø) :arrow_up:
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 135391c...d879990. Read the comment docs.

rafaeljesus commented 6 years ago

We need to rethink this implementation, the failing target url should be remove from the balancer when CB trips and added back when it closes, otherwise we might have healthy target services but the failed one can trip the CB. Consul manages this out of the box, but if you don't operate janus in such way the CB won't be accurate

italolelis commented 6 years ago

@rafaeljesus can you take this task with @c4pone? It would be nice to have this feature as soon as possible, with the scenario that you just mentioned

c4pone commented 6 years ago

@italolelis I will close this pr for now and will think about a better solution next week together with @rafaeljesus. We will have a look into how we can trip the circuit breaker for each individual target without tripping for the whole endpoint. Also we were thinking about using a different circuit breaker library as hystrix-go is not the way to go.