Circuit breaker refactor and enhance the scalability of stat module #143

louyuting · 2020-04-28T14:10:36Z

Describe what this PR does / why we need it

Circuit breaker refactor and enhance the scalability of stat module

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

codecov-io · 2020-04-28T16:48:36Z

Codecov Report

Merging #143 into master will decrease coverage by 1.16%.
The diff coverage is 40.68%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   43.48%   42.31%   -1.17%     
==========================================
  Files          67       68       +1     
  Lines        2863     3247     +384     
==========================================
+ Hits         1245     1374     +129     
- Misses       1479     1737     +258     
+ Partials      139      136       -3

Impacted Files	Coverage Δ
core/base/entry.go	`0.00% <0.00%> (ø)`
core/circuitbreaker/slot.go	`0.00% <0.00%> (ø)`
core/circuitbreaker/stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/metric_bucket.go	`62.96% <ø> (ø)`
core/stat/stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/sliding_window_metric.go	`22.87% <10.00%> (ø)`
core/circuitbreaker/circuit_breaker.go	`23.30% <23.30%> (ø)`
core/base/context.go	`40.54% <29.41%> (-11.85%)`	⬇️
core/stat/base/bucket_leap_array.go	`42.70% <50.00%> (ø)`
core/stat/base/leap_array.go	`57.14% <50.00%> (-7.23%)`	⬇️
... and 8 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 975debd...da72d05. Read the comment docs.

louyuting · 2020-04-29T12:23:00Z

Please ignore Conflicting.
It's not impact the implementation of this PR
I will fix it.

sczyh30 · 2020-05-06T11:14:11Z

core/base/context.go

 	Output *SentinelOutput
 }

+func (ctx *EntryContext) GetError() error {


Maybe just Error() or Err() is enough? See https://golang.org/doc/effective_go.html#Getters:

There's nothing wrong with providing getters and setters yourself, and it's often appropriate to do so, but it's neither idiomatic nor necessary to put Get into the getter's name. If you have a field called owner (lower case, unexported), the getter method should be called Owner (upper case, exported), not GetOwner.

Maybe Err() is better.
If EntryContext implement Error() function, will implement standard error interface implicitly.

sczyh30 · 2020-05-06T11:16:36Z

core/base/context.go

+	rtVal, existed := ctx.Output.Attachments[EntryContextRtStatKey]
+	if !existed {
+		rt := util.CurrentTimeMillis() - ctx.StartTime()
+		ctx.PutRtIfAbsent(rt)


Seems weird to bring the operation here. Any consideration?

My thinking may not be right, there is no concurrency scenario. I'm going to optimize here.

sczyh30 · 2020-05-06T11:18:04Z

core/base/context.go

+		ctx.PutRtIfAbsent(rt)
+		return rt
+	}
+	rt := rtVal.(uint64)


Is the performance OK if QPS is large?

The impact for performance is small.
I just thought about it again, maybe add a new field 'rt' in EntryContext is better.

sczyh30 · 2020-05-06T11:30:30Z

core/circuit_breaker/circuit_breaker.go

+----------------+                   +----------------+                 +----------------+
+*/
+const (
+	Closed Status = iota


It's better to be State (corresponding to state machine).

sczyh30 · 2020-05-06T11:32:32Z

core/circuit_breaker/circuit_breaker.go

+
+// StatusSwitchListener could listen on circuit breaker status switch
+type StatusSwitchListener interface {
+	onSwitchToClosed(prev Status, rule Rule)


Make it public?

sczyh30 · 2020-05-06T11:47:35Z

core/circuit_breaker/rule.go

 	// return the strategy type
 	BreakerStrategy() BreakerStrategy
 	// check whether the rule is valid and could be converted to corresponding circuit breaker
 	isApplicable() bool


Maybe it's better to return the concrete error here (and log it at caller). Example: https://github.com/alibaba/sentinel-golang/blob/master/core/system/rule_manager.go#L87

make sense.

sczyh30 · 2020-05-06T11:52:16Z

core/circuit_breaker/rule_manager.go

-	if !ok {
-		ret = make([]CircuitBreaker, 0)
+	ret := make([]CircuitBreaker, 0)
+	updateMux.Lock()


sczyh30 · 2020-05-06T11:57:17Z

core/circuit_breaker/rule_manager.go

+		return ret
 	}
+	ret = append(ret, resCBs...)
+	ret = append(ret, commonCBs...)


For default rules, it does not indicate shared circuit breaker for all resources (however it might be helpful in some scenarios). Actually it means shared circuit breaker generator on resource created.

sczyh30 · 2020-05-06T12:14:36Z

core/circuit_breaker/rule_manager.go

+				logger.Warnf("Circuit Breaker Generator for %+v is not existed.", r.BreakerStrategy())
+				continue
+			}
+			cb := generator(r)


Here we may improve the logic: if a rule remains unchanged (identical), use the former circuit breaker instance instead of re-creating a new one.

sczyh30 · 2020-05-06T12:15:08Z

core/circuit_breaker/rule_manager.go

 	logger.Info(sb.String())
 }
+
+func ResisterStatusSwitchListeners(listeners ...StatusSwitchListener) {


sczyh30 · 2020-05-06T12:30:17Z

core/circuit_breaker/circuit_breaker.go

+type StatusSwitchListener interface {
+	onSwitchToClosed(prev Status, rule Rule)
+
+	onSwitchToOpen(prev Status, rule Rule)


We could also carry the triggered "snapshot" value when the circuit breaker transforms to open.

sczyh30 · 2020-05-06T12:35:00Z

core/circuit_breaker/circuit_breaker.go

+// return true if succeed to update
+func (b *circuitBreakerBase) fromHalfOpenToOpen() bool {
+	if b.status.casStatus(HalfOpen, Open) {
+		for _, listener := range b.statusSwitchListeners {


updateNextRetryTimestamp is also needed here.

make sense.

sczyh30 · 2020-05-06T14:59:40Z

Please also add relevant slots to the default slot chain.

sczyh30 · 2020-05-06T15:02:30Z

core/circuit_breaker/rule_manager.go

+				retryTimeoutMs:        errRatioRule.RetryTimeout,
+				nextRetryTimestamp:    0,
+				status:                status,
+				statusSwitchListeners: statusSwitchListeners,


The new listeners will not be included if added after the circuit breaker is generated.

sczyh30 · 2020-05-06T15:03:01Z

core/circuit_breaker/stat_slot.go

+}
+
+func (c *MetricStatSlot) OnCompleted(ctx *base.EntryContext) {
+	res := ctx.Resource.String()


Resource.Name()

sczyh30 · 2020-05-06T15:05:35Z

core/circuit_breaker/circuit_breaker.go

+----------------+                   +----------------+                 +----------------+
+*/
+const (
+	Closed State = iota


It would be better if we could add String() method for it.

sczyh30 · 2020-05-11T11:46:26Z

core/base/context.go

+	}
 }

 // Reset init EntryContext,


The reset method should be updated for new fields.

sczyh30 · 2020-05-11T11:47:42Z

core/circuit_breaker/circuit_breaker.go

+// fromClosedToOpen update circuit breaker status machine from closed to open
+// Used for opening circuit breaker from closed when checking circuit breaker
+// return true if succeed to update
+func (b *circuitBreakerBase) fromClosedToOpen() bool {


Carry the snapshot value here?

sczyh30 · 2020-05-11T11:47:52Z

core/circuit_breaker/circuit_breaker.go

+// fromHalfOpenToOpen update circuit breaker status machine from half-open to open
+// Used for failing to probe
+// return true if succeed to update
+func (b *circuitBreakerBase) fromHalfOpenToOpen() bool {


Carry the snapshot value here?

sczyh30 · 2020-05-11T12:18:47Z

core/circuit_breaker/rule_manager.go

 	for _, rule := range rules {
-		if !rule.isApplicable() {
-			logger.Warnf("Ignoring invalid breaker rule when loading new rules, %+v.", rule)
+		if err := rule.IsApplicable(); err != nil {


Do we need to check nil here?

sczyh30 · 2020-05-11T12:19:39Z

core/circuit_breaker/rule_manager.go

-		if !rule.isApplicable() {
-			logger.Warnf("Ignoring invalid breaker rule when loading new rules, %+v.", rule)
+		if err := rule.IsApplicable(); err != nil {
+			logger.Warnf("Ignoring invalid breaker rule when loading new rules, err: %+resRules.", err)


Maybe it's better to print both err and the bad rule?

sczyh30 · 2020-05-19T14:40:53Z

core/circuitbreaker/stat_slot.go

+}
+
+func (c *MetricStatSlot) OnCompleted(ctx *base.EntryContext) {
+	res := ctx.Resource.Name()


We need to check whether this request has been blocked. If blocked, the request should not be recorded to the circuit breakers.

sczyh30 · 2020-05-19T14:42:20Z

core/circuitbreaker/circuit_breaker.go

+}
+
+func (b *circuitBreakerBase) retryTimeoutArrived() bool {
+	return util.CurrentTimeMillis() >= b.nextRetryTimestamp


We may need to make it thread-safe when accessing and modifying nextRetryTimestamp.

sczyh30 · 2020-05-19T14:42:43Z

core/circuitbreaker/circuit_breaker.go

+type CircuitBreaker interface {
+	BoundRule() Rule
+
+	BoundStat() interface{}


Can we make it private?

codecov-commenter · 2020-05-19T15:21:24Z

Codecov Report

Merging #143 into master will decrease coverage by 1.16%.
The diff coverage is 40.55%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   42.72%   41.55%   -1.17%     
==========================================
  Files          67       68       +1     
  Lines        2926     3314     +388     
==========================================
+ Hits         1250     1377     +127     
- Misses       1537     1801     +264     
+ Partials      139      136       -3

Impacted Files	Coverage Δ
core/base/entry.go	`0.00% <0.00%> (ø)`
core/circuitbreaker/slot.go	`0.00% <ø> (ø)`
core/circuitbreaker/stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/metric_bucket.go	`62.96% <ø> (ø)`
core/stat/stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/sliding_window_metric.go	`22.87% <10.00%> (ø)`
core/base/context.go	`21.62% <15.38%> (-3.38%)`	⬇️
core/circuitbreaker/circuit_breaker.go	`23.30% <23.30%> (ø)`
core/stat/base/bucket_leap_array.go	`42.70% <50.00%> (ø)`
core/stat/base/leap_array.go	`57.14% <50.00%> (-7.23%)`	⬇️
... and 8 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 b73ee5e...70ea8e9. Read the comment docs.

add example

sczyh30

LGTM

sczyh30 · 2020-05-21T12:51:44Z

Awesome work. Thanks!

louyuting added area/circuit-breaking Issues or PRs related to circuit breaking kind/enhancement Category issues or PRs related to enhancement to-review PRs to review labels Apr 28, 2020

louyuting requested a review from sczyh30 April 28, 2020 17:01

This was linked to issues Apr 29, 2020

Enhance Circuit Breaker Design #122

Closed

Make statistic metrics in MetricBucket extensible #123

Closed

sczyh30 reviewed May 6, 2020

View reviewed changes

louyuting force-pushed the circuit_breaker_refactor branch from 6afe67f to 45687b6 Compare May 6, 2020 13:43

sczyh30 requested changes May 6, 2020

View reviewed changes

sczyh30 reviewed May 6, 2020

View reviewed changes

sczyh30 requested changes May 11, 2020

View reviewed changes

sczyh30 reviewed May 11, 2020

View reviewed changes

louyuting force-pushed the circuit_breaker_refactor branch from f3b3d29 to 2d41d02 Compare May 18, 2020 13:36

sczyh30 requested changes May 19, 2020

View reviewed changes

louyuting added 2 commits May 20, 2020 17:35

circuit breaker refactor

ee63c25

fix bug: statslot ignore blocked request; data race in retryTimeout

7943e1c

add example

louyuting force-pushed the circuit_breaker_refactor branch from 1b796b1 to 7943e1c Compare May 21, 2020 09:16

fix state change snapshot when probing.

70ea8e9

louyuting force-pushed the circuit_breaker_refactor branch from ff4d6c1 to 70ea8e9 Compare May 21, 2020 12:20

sczyh30 approved these changes May 21, 2020

View reviewed changes

sczyh30 merged commit 112f169 into alibaba:master May 21, 2020

sczyh30 removed the to-review PRs to review label May 21, 2020

sczyh30 added this to the 0.3.0 milestone May 21, 2020

sczyh30 mentioned this pull request May 22, 2020

Polish api.Tracer with new error tracing mechanism #153

Merged

Circuit breaker refactor and enhance the scalability of stat module #143

Circuit breaker refactor and enhance the scalability of stat module #143

Uh oh!

Conversation

louyuting commented Apr 28, 2020

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Uh oh!

codecov-io commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

louyuting commented Apr 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louyuting May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sczyh30 commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Apr 28, 2020 •

edited

Loading

louyuting May 6, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading