Added a circuit breaker for http and tchannel client calls #539

abhishekparwal · 2019-01-08T21:48:41Z

Added a circuit breaker for http and tchannel client calls for preventing downstream from failures/outages/high latency

CLAassistant · 2019-01-08T21:48:48Z

All committers have signed the CLA.

coveralls · 2019-01-08T22:47:26Z

Coverage increased (+0.4%) to 68.629% when pulling fd04ba9 on circuit_breaker into 6f0903f on master.

ChuntaoLu

Overall looking good, left comments on a few minor things that need to be fixed.

A couple of more important things:

the client call wrapper is not free, we should know the performance implications. Let's benchmark this change and compare with master, read more at https://github.com/uber/zanzibar/blob/master/benchmarks/README.md.
the circuit breaker should be configurable, i.e., users can decide whether to have circuit breaker or not. That said, we can make it opt-in by default.

ChuntaoLu · 2019-01-08T22:58:54Z

codegen/templates/http_client.tmpl

@@ -5,6 +5,7 @@ package {{$instance.PackageInfo.PackageName}}
 import (
 	"context"
 	"fmt"
+	"github.com/afex/hystrix-go/hystrix"


The first import group should be stdlib, third party libraries should go into the following group.

ok, while doing codegen we are running goimports

That is correct, but consistency is good.

ChuntaoLu · 2019-01-08T23:21:30Z

runtime/gateway.go

@@ -23,6 +23,8 @@ package zanzibar
 import (
 	"context"
 	"fmt"
+	"github.com/afex/hystrix-go/hystrix/metric_collector"


import grouping

ChuntaoLu · 2019-01-08T23:28:28Z

test/endpoints/tchannel/baz/baz_metrics_test.go

@@ -74,7 +74,7 @@ func TestCallMetrics(t *testing.T) {
 	)
 	assert.NoError(t, err)

-	numMetrics := 10
+	numMetrics := 14


We should have assertions for the new metrics, not just the count.

ChuntaoLu · 2019-01-09T02:01:45Z

runtime/plugins/m3_aggregator.go

+package plugins
+
+import (
+	"github.com/uber-go/tally"


import grouping

same as above

ChuntaoLu · 2019-01-09T02:28:10Z

codegen/templates/http_client.tmpl

@@ -167,7 +176,11 @@ func (c *{{$clientName}}) {{$methodName}}(
 	}
 	{{- end}}

-	res, err := req.Do()
+	var res *zanzibar.ClientHTTPResponse
+	err = hystrix.Do("{{$clientID}}", func() error {


We should call hystrix.DoC with the proper context here, so the context is respected.

ChuntaoLu · 2019-01-09T02:31:00Z

codegen/templates/tchannel_client.tmpl

@@ -5,6 +5,7 @@ package {{$instance.PackageInfo.PackageName}}
 import (
 	"context"
 	"errors"
+	"github.com/afex/hystrix-go/hystrix"


import grouping

same as above

ChuntaoLu · 2019-01-09T23:22:59Z

Copyright change is landed on master, rebase would get rid of the noises.

abhishekparwal · 2019-01-10T01:56:00Z

#################################################

benchmark output circuit_breaker

#################################################
#################################################

Running 30s test @ http://localhost:8093/contacts/foo/contacts
12 threads and 400 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 32.00ms 33.25ms 1.08s 88.01%
Req/Sec 1.20k 203.56 2.12k 70.97%
429092 requests in 30.04s, 47.06MB read
Socket errors: connect 0, read 235, write 0, timeout 0
Non-2xx or 3xx responses: 14
Requests/sec: 14284.97
Transfer/sec: 1.57MB

#################################################

benchmark output master

#################################################
#################################################

Running 30s test @ http://localhost:8093/contacts/foo/contacts
12 threads and 400 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 35.75ms 32.25ms 392.37ms 65.52%
Req/Sec 1.05k 152.10 2.02k 77.17%
376117 requests in 30.04s, 41.25MB read
Socket errors: connect 0, read 253, write 0, timeout 0
Requests/sec: 12520.04
Transfer/sec: 1.37MB
Switched to branch 'circuit_breaker'

…ting downstream from failures/outages/high latency

ChuntaoLu

Could you add a short passage in the readme doc to describe the circuit breaker feature and how to enable/disable and config?

ChuntaoLu · 2019-01-11T04:35:31Z

runtime/gateway.go

-	"github.com/opentracing/opentracing-go"
+	metricCollector "github.com/afex/hystrix-go/hystrix/metric_collector"
+	"github.com/uber/zanzibar/runtime/plugins"
+


unnecessary blank line

ChuntaoLu · 2019-01-11T04:40:54Z

examples/example-gateway/config/production.json

 	"clients.multi.ip": "127.0.0.1",
 	"clients.multi.port": 4003,
 	"clients.multi.timeout": 10000,
+	"clients.multi.maxConcurrentRequests": 1000,


We should have a setting where circuit breaker is disabled as a configuration example.

ChuntaoLu · 2019-01-11T04:57:26Z

test/endpoints/baz/baz_metrics_test.go

@@ -95,7 +95,7 @@ func TestCallMetrics(t *testing.T) {
 	metrics := cg.M3Service.GetMetrics()
 	// we don't care about jaeger emitted metrics
 	for key := range metrics {
-		if strings.HasPrefix(key, "jaeger") {
+		if strings.HasPrefix(key, "jaeger") || strings.Contains(key, "circuitbreaker") {


I actually think circuitbreaker metrics are critical (in the sense we will create dashboard and monitor/alert).

And ideally we should have tests that covers failure scenarios to verify our integration with the library is as expected.

ChuntaoLu · 2019-01-16T01:35:38Z

README.md

@@ -140,6 +140,23 @@ func (c *quux) Echo(s string) string { return s }
 ```
 Note the type of `deps` parameter passed to `NewClient` constructor function is generated by Zanzibar, as indicated by the import path. Zanzibar takes care of initializing and passing in the acutal `deps` argument, as mentioned in [Dependency Injection](#dependency-injection).

+###### Circuit Breaker
+For increasing overall system resiliency, zanzibar uses a [Circuit Breaker](https://msdn.microsoft.com/en-us/library/dn589784.aspx) to avoid calling clients when there is a increase in failure rate beyond a set 


typo: an increase

ChuntaoLu · 2019-01-16T01:41:49Z

README.md

@@ -140,6 +140,23 @@ func (c *quux) Echo(s string) string { return s }
 ```
 Note the type of `deps` parameter passed to `NewClient` constructor function is generated by Zanzibar, as indicated by the import path. Zanzibar takes care of initializing and passing in the acutal `deps` argument, as mentioned in [Dependency Injection](#dependency-injection).

+###### Circuit Breaker
+For increasing overall system resiliency, zanzibar uses a [Circuit Breaker](https://msdn.microsoft.com/en-us/library/dn589784.aspx) to avoid calling clients when there is a increase in failure rate beyond a set 
+threshold. It recovers it after elasped time to again fo in half-open and close state. 


It recovers it after elasped time to again fo in half-open and close state.

Can you rework this sentence? a few typos and the information it tries to convey doesn't seem clear to me.

ChuntaoLu · 2019-01-16T01:43:35Z

README.md

+```
+    "clients.<clientID>.errorPercentThreshold": 20
+```
+By default circuit breaker is enabled, if you want to disable it , set : 


redundant white space before last ,

ChuntaoLu · 2019-01-16T01:46:42Z

examples/example-gateway/config/production.yaml

 clients.google-now.ip: 127.0.0.1
 clients.google-now.port: 14120
 clients.google-now.timeout: 10000
+clients.google-now.maxConcurrentRequests: 1000


Let's put the disabled config here as well, to be consistent with the json config. (yaml is prefered, but zanzibar is backward compatible with json config)

ChuntaoLu · 2019-01-16T01:47:31Z

examples/example-gateway/config/test.yaml

@@ -3,27 +3,41 @@ clients.bar.defaultHeaders:
 clients.bar.ip: 127.0.0.1
 clients.bar.port: 4001
 clients.bar.timeout: 10000
+clients.bar.maxConcurrentRequests: 1000


same as above

ChuntaoLu · 2019-01-17T01:30:36Z

README.md

+For increasing overall system resiliency, zanzibar uses a [Circuit Breaker](https://msdn.microsoft.com/en-us/library/dn589784.aspx) which avoids calling client when there is an increase in failure rate beyond a set 
+threshold. After a sleepWindowInMilliseconds, client calls are attempted recovery by going in half-open and then close state. 
+
+circuitBreakerDisabled: Default false. Disables the circuit-breaker: 


Disables -> To disable

ChuntaoLu · 2019-01-17T01:30:58Z

README.md

+    "clients.<clientID>.circuitBreakerDisabled" : true
+```
+
+maxConcurrentRequests: Default 50. Sets how many requests can be run at the same time, beyond which requests are 


Sets -> To set

Same for the following 3 configurations

ChuntaoLu suggested changes Jan 9, 2019

View reviewed changes

ChuntaoLu requested a review from jacobgreenleaf January 9, 2019 04:40

abhishekparwal added 6 commits January 10, 2019 10:42

Added a circuit breaker for http and tchannel client calls for preven…

9a26a13

…ting downstream from failures/outages/high latency

generated code

7103f35

code review comments

99ac31b

code review comments

a1f55fb

generated code

8a496cd

code review

e98e2a5

abhishekparwal force-pushed the circuit_breaker branch from dfc9e9a to e98e2a5 Compare January 10, 2019 18:46

abhishekparwal added 2 commits January 10, 2019 10:57

review comments

dc8effc

review comments

ef057e9

ChuntaoLu reviewed Jan 11, 2019

View reviewed changes

review comments

05cae3f

ChuntaoLu reviewed Jan 16, 2019

View reviewed changes

abhishekparwal added 2 commits January 15, 2019 19:45

documentation and defaults

d784172

gen code

2f08eae

ChuntaoLu approved these changes Jan 17, 2019

View reviewed changes

grammar

fd04ba9

ChuntaoLu merged commit 9a4028e into master Jan 17, 2019

ChuntaoLu deleted the circuit_breaker branch January 17, 2019 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a circuit breaker for http and tchannel client calls #539

Added a circuit breaker for http and tchannel client calls #539

abhishekparwal commented Jan 8, 2019

CLAassistant commented Jan 8, 2019 •

edited

Loading

coveralls commented Jan 8, 2019 •

edited

Loading

ChuntaoLu left a comment

ChuntaoLu Jan 8, 2019

abhishekparwal Jan 9, 2019 •

edited

Loading

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 8, 2019

abhishekparwal Jan 9, 2019 •

edited

Loading

ChuntaoLu Jan 8, 2019

ChuntaoLu Jan 9, 2019

abhishekparwal Jan 9, 2019

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

abhishekparwal Jan 9, 2019

ChuntaoLu commented Jan 9, 2019

abhishekparwal commented Jan 10, 2019

ChuntaoLu left a comment

ChuntaoLu Jan 11, 2019

ChuntaoLu Jan 11, 2019

ChuntaoLu Jan 11, 2019

ChuntaoLu Jan 16, 2019

ChuntaoLu Jan 16, 2019

ChuntaoLu Jan 16, 2019

ChuntaoLu Jan 16, 2019

ChuntaoLu Jan 16, 2019

ChuntaoLu Jan 17, 2019

ChuntaoLu Jan 17, 2019

Added a circuit breaker for http and tchannel client calls #539

Added a circuit breaker for http and tchannel client calls #539

Conversation

abhishekparwal commented Jan 8, 2019

CLAassistant commented Jan 8, 2019 • edited Loading

coveralls commented Jan 8, 2019 • edited Loading

ChuntaoLu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekparwal Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekparwal Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuntaoLu commented Jan 9, 2019

abhishekparwal commented Jan 10, 2019

benchmark output circuit_breaker

benchmark output master

ChuntaoLu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jan 8, 2019 •

edited

Loading

coveralls commented Jan 8, 2019 •

edited

Loading

abhishekparwal Jan 9, 2019 •

edited

Loading

abhishekparwal Jan 9, 2019 •

edited

Loading