frequency parameters traffic control #119

louyuting · 2020-04-06T06:47:50Z

Describe what this PR does / why we need it

Frequency parameters traffic control implementation.

Does this pull request fix one issue?

Describe how you did it

Supporting reject directly strategy and throttling strategy.
Using token counter to record statistics。

Describe how to verify it

Special notes for reviews

codecov-io · 2020-04-06T06:52:37Z

Codecov Report

Merging #119 into master will decrease coverage by 1.74%.
The diff coverage is 34.00%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage   43.48%   41.74%   -1.75%     
==========================================
  Files          67       74       +7     
  Lines        2863     3507     +644     
==========================================
+ Hits         1245     1464     +219     
- Misses       1479     1865     +386     
- Partials      139      178      +39

Impacted Files	Coverage Δ
api/api.go	`32.78% <0.00%> (-1.70%)`	⬇️
core/base/result.go	`38.33% <ø> (ø)`
core/config/config.go	`25.00% <0.00%> (ø)`
core/freq_params_traffic/concurrency_stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/leap_array.go	`64.36% <0.00%> (ø)`
util/atomic.go	`58.33% <0.00%> (-25.67%)`	⬇️
core/freq_params_traffic/rule_manager.go	`0.72% <0.72%> (ø)`
core/freq_params_traffic/rule.go	`25.75% <25.75%> (ø)`
core/freq_params_traffic/slot.go	`34.11% <34.11%> (ø)`
core/freq_params_traffic/traffic_shaping.go	`43.58% <43.58%> (ø)`
... and 11 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...9f00b0f. Read the comment docs.

sczyh30 · 2020-04-18T09:46:45Z

core/stat/stat_slot_callback.go

+	callbackUpdateMux = new(sync.RWMutex)
+)
+
+type StatSlotEntryCallback interface {


Maybe the stat slot callback is not needed? Just make it as another StatSlot and add it to slot chain is okay :)

Adding a new StatSlot to record the concurrency statistic seems make more sense. I will reimplement related logic.

sczyh30 · 2020-05-07T07:13:44Z

core/freq_params_traffic/params_metric.go

+	// ruleTokenCounter record the number of token of value
+	ruleTokenCounter cache.ConcurrentCounterCache
+	// goroutineCounter record the number of goroutine of value
+	goroutineCounter cache.ConcurrentCounterCache


How about concurrencyCounter or something else (as the counter indicates concurrency actually)?

sczyh30 · 2020-05-07T07:15:00Z

core/freq_params_traffic/rule.go

+type ControlStrategy int8
+
+const (
+	RejectFaster ControlStrategy = iota


Maybe just Reject is enough?

I want to express the meaning of quick and direct fail.
”Reject“ may be enough.

sczyh30 · 2020-05-07T07:16:54Z

core/freq_params_traffic/rule.go

+
+import "strconv"
+
+type ControlStrategy int8


Keep the name consistent with that in FlowRule?

That's ok. Keep the name convention.

sczyh30 · 2020-05-07T07:19:08Z

core/freq_params_traffic/rule.go

+
+type SpecificValue struct {
+	ValType  Kind
+	ValueStr string


Keep the naming consistent? (both val or value)

sczyh30 · 2020-05-07T07:20:58Z

core/freq_params_traffic/rule.go

+	QPS
+)
+
+type Kind int


The term "kind" has different meanings in type system. Maybe we could use ParamType or other concrete name?

Here i refer to the data type definition of go standard reflect package.
https://github.com/golang/go/blob/98d20fb23551a7ab900fcfe9d25fd9cb6a98a07f/src/reflect/type.go#L230
I think keep name consistent with standard package is ok.

That's okay, but just Kind may be too generic. Maybe ParamKind or other concrete?

ParamKind is Okay.

sczyh30 · 2020-05-07T07:32:59Z

core/freq_params_traffic/slot.go

+
+	for _, tc := range tcs {
+		if tc == nil {
+			logger.Warnf("Nil traffic controller was found, res: %s", res)


It may yield a large number of logs. As this is not expected as usual, maybe we could make it "debug" level?

Make sense.

sczyh30 · 2020-05-07T07:33:34Z

core/freq_params_traffic/slot.go

+			logger.Warnf("The TrafficShapingController's paramIndex(%d) less than 0.", idx)
+		}
+		if len(args) <= idx {
+			logger.Warnf("The index of rule is not existed in args. The TrafficShapingController's paramIndex is %d, args: %+v, tc: %+v", idx, args, tc)


Ditto for "debug" level

sczyh30 · 2020-05-07T07:36:47Z

core/freq_params_traffic/slot.go

+			continue
+		}
+		idx := tc.getParamIndex()
+		if idx < 0 {


Maybe we could support reversed index here. For example, -1 means the last one. This could be useful when the length of parameter list is not fixed.

sczyh30 · 2020-05-07T08:11:44Z

core/freq_params_traffic/traffic_shaping.go

+				awaitTime := expectedTime - currentTimeInMs
+				if awaitTime > 0 {
+					atomic.StoreInt64(lastPassTimePtr, expectedTime)
+					time.Sleep(time.Duration(awaitTime) * time.Millisecond)


Maybe it's better to use NewTokenResultShouldWait instead?

sczyh30 · 2020-05-07T09:13:04Z

core/freq_params_traffic/concurrency_stat_slot.go

+			logger.Warnf("The TrafficShapingController's paramIndex(%d) less than 0.", idx)
+		}
+		if len(args) <= idx {
+			logger.Warnf("The index of rule is not existed in args. The TrafficShapingController's paramIndex is %d, args: %+v, tc: %+v", idx, args, tc)


Ditto for logging level. Also typo: is not existed -> does not exist

sczyh30 · 2020-05-11T13:12:51Z

core/freq_params_traffic/slot.go

+		return nil
+	}
+	switch arg.(type) {
+	case int:


Also support bool/float32/float64 here?

codecov-commenter · 2020-05-21T13:58:26Z

Codecov Report

Merging #119 into master will increase coverage by 2.12%.
The diff coverage is 53.18%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   40.77%   42.90%   +2.12%     
==========================================
  Files          68       75       +7     
  Lines        3279     3951     +672     
==========================================
+ Hits         1337     1695     +358     
- Misses       1810     2065     +255     
- Partials      132      191      +59

Impacted Files	Coverage Δ
api/api.go	`30.50% <0.00%> (-1.64%)`	⬇️
core/base/result.go	`29.35% <ø> (ø)`
core/config/config.go	`25.00% <0.00%> (ø)`
core/freq_params_traffic/concurrency_stat_slot.go	`0.00% <0.00%> (ø)`
core/stat/base/leap_array.go	`57.14% <0.00%> (ø)`
util/atomic.go	`58.33% <0.00%> (-25.67%)`	⬇️
core/freq_params_traffic/slot.go	`32.95% <32.95%> (ø)`
core/freq_params_traffic/rule_manager.go	`53.12% <53.12%> (ø)`
core/freq_params_traffic/cache/lru.go	`53.77% <53.77%> (ø)`
core/freq_params_traffic/rule.go	`58.75% <58.75%> (ø)`
... and 11 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 46b892a...43968c8. Read the comment docs.

sczyh30 · 2020-05-26T11:18:18Z

core/freq_params_traffic/traffic_shaping.go

+}
+
+func newBaseTrafficShapingControllerWithMetric(r *Rule, metric *ParamsMetric) *baseTrafficShapingController {
+	size := int(math.Min(float64(ParamsMaxCapacity), float64(ParamsCapacityBase*r.DurationInSec)))


How about making the cache size as an attribute of the param flow rule?

Maybe an optional attribute make sense.

sczyh30 · 2020-06-03T07:41:29Z

I've been testing with the demo and found the pass count flaky like this (see the attachment):

sentinel-go-demo-local-metrics.log

Rule:

{
	Resource:          "test",
	MetricType:        freq_params_traffic.QPS,
	Behavior:          freq_params_traffic.Reject,
	ParamIndex:        0,
	Threshold:         100,
	MaxQueueingTimeMs: 0,
	ParamsMaxCapacity: 2,
	DurationInSec:     1,
	SpecificItems:     make(map[freq_params_traffic.SpecificValue]int64),
}

Demo:

// Usually, the total pass QPS is expected to be 200 (two parameters, 100 respectively).
for i := 0; i < 8; i++ {
	go func() {
		for {
			e, b := sentinel.Entry(Resource, sentinel.WithSlotChain(sc), sentinel.WithArgs(true, rand.Int()%3000, uuid.New().String(), uuid.New().String()))
			if b != nil {
				// Blocked. We could get the block reason from the BlockError.
				time.Sleep(time.Duration(rand.Uint64()%10) * time.Millisecond)
			} else {
				// Passed, wrap the logic here.
				time.Sleep(time.Duration(rand.Uint64()%10) * time.Millisecond)
				// Be sure the entry is exited finally.
				e.Exit()
			}
		}
	}()
}

for {
	e, b := sentinel.Entry(Resource, sentinel.WithSlotChain(sc), sentinel.WithArgs(false, rand.Int()%3000, uuid.New().String(), uuid.New().String()))
	if b != nil {
		time.Sleep(time.Duration(rand.Uint64()%10) * time.Millisecond)
	} else {
		// Passed, wrap the logic here.
		time.Sleep(time.Duration(rand.Uint64()%10) * time.Millisecond)

		// Be sure the entry is exited finally.
		e.Exit()
	}
}

Any ideas about it?

sczyh30

LGTM, we may improve this in upcoming pull requests

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

Fabulous work. Thanks!

louyuting linked an issue Apr 6, 2020 that may be closed by this pull request

"Frequent" parameter flow control #103

Closed

sczyh30 added kind/feature Category issues or PRs related to feature request to-review PRs to review labels Apr 7, 2020

louyuting force-pushed the frequent-parameters-flow-control-implementation branch 2 times, most recently from 9b64632 to 14a48a7 Compare April 17, 2020 11:47

louyuting requested a review from sczyh30 April 17, 2020 11:48

sczyh30 reviewed Apr 18, 2020

View reviewed changes

sczyh30 reviewed May 7, 2020

View reviewed changes

louyuting force-pushed the frequent-parameters-flow-control-implementation branch from 3bdd464 to 5fab2ef Compare May 8, 2020 17:07

sczyh30 reviewed May 11, 2020

View reviewed changes

core/freq_params_traffic/slot.go

return nil

}

switch arg.(type) {

case int:

Copy link

Member

sczyh30 May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also support bool/float32/float64 here?

louyuting force-pushed the frequent-parameters-flow-control-implementation branch from 86ee5dc to 1974b3e Compare May 21, 2020 13:46

frequency parameters flow control implementation

aa358bc

louyuting force-pushed the frequent-parameters-flow-control-implementation branch from 1974b3e to aa358bc Compare May 23, 2020 16:02

louyuting requested a review from sczyh30 May 23, 2020 16:08

sczyh30 reviewed May 26, 2020

View reviewed changes

louyuting added 3 commits May 26, 2020 23:49

optimize the TokenResult using

904e06a

fix unit test

b1d50d7

Make statistic cache size configurable in Rule

43968c8

louyuting requested a review from sczyh30 May 27, 2020 14:16

sczyh30 approved these changes Jun 5, 2020

View reviewed changes

sczyh30 removed the to-review PRs to review label Jun 5, 2020

sczyh30 merged commit 03d6c0d into alibaba:master Jun 5, 2020

sczyh30 added the size/XXL Indicate a PR that changes 1000+ lines. label Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frequency parameters traffic control #119

frequency parameters traffic control #119

louyuting commented Apr 6, 2020

codecov-io commented Apr 6, 2020 •

edited

Loading

sczyh30 Apr 18, 2020

louyuting Apr 20, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 8, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

sczyh30 May 7, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 7, 2020

louyuting May 8, 2020

sczyh30 May 11, 2020

codecov-commenter commented May 21, 2020 •

edited

Loading

sczyh30 May 26, 2020

louyuting May 27, 2020

sczyh30 commented Jun 3, 2020

sczyh30 left a comment

sczyh30 commented Jun 5, 2020

frequency parameters traffic control #119

frequency parameters traffic control #119

Conversation

louyuting commented Apr 6, 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

codecov-io commented Apr 6, 2020 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

codecov-commenter commented May 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sczyh30 commented Jun 3, 2020

sczyh30 left a comment

Choose a reason for hiding this comment

sczyh30 commented Jun 5, 2020

codecov-io commented Apr 6, 2020 •

edited

Loading

codecov-commenter commented May 21, 2020 •

edited

Loading