alibaba / alibaba/Sentinel

System rule check didn't calculate with new acquire count

Open
#1,487 0 comments 0 reactions 0 assignees View on GitHub
kind/discussion
Dominant language
Java
Stars
23.1k
Forks
8.1k
PR merge metrics
No merged PRs in 30d

Description

### Issue Description

Result in `SystemGuardDemo` seems not as expected, may due to that System rule check didn't calculate with new acquire count, I find it when test setting SystemRule with RedisDataSourceDemo.

### Describe what happened (or what feature you want)

When set qps or max thread to 0 in `SystemRule`, there are still passed requests.

### Describe what you expected to happen

When set qps or max thread to 0 in `SystemRule`, there are no requests passed.

### How to reproduce it (as minimally and precisely as possible)

In `SystemGuardDemo`, modify `initSystemRule` method,

1.Only set `rule.setQps(0);` and other commented, result in console:
```
100, 1589615007900, total:3960, pass:100, block:3954
99, 1589615008900, total:5002, pass:100, block:4900
98, 1589615009899, total:5002, pass:100, block:4900
97, 1589615010900, total:5000, pass:100, block:4900
...
```

2.Only set `rule.setMaxThread(0);` and other commented, result in console:
```
100, 1589615116809, total:4051, pass:186, block:3961
99, 1589615117809, total:4996, pass:80, block:4920
98, 1589615118810, total:5000, pass:90, block:4910
97, 1589615119810, total:5000, pass:79, block:4916
96, 1589615120811, total:5000, pass:67, block:4933
95, 1589615121812, total:5000, pass:67, block:4914
94, 1589615122812, total:5000, pass:62, block:4938
93, 1589615123813, total:5000, pass:87, block:4913
92, 1589615124813, total:5000, pass:78, block:4922
...
```
In case1,2, since the qps or thread set to 0, there should be no requests passed.

3.Only set `rule.setQps(1);` and other commented, result in console:
```
100, 1589615286504, total:3978, pass:123, block:3955
99, 1589615287504, total:4970, pass:92, block:4877
98, 1589615288504, total:4995, pass:105, block:4888
97, 1589615289505, total:4997, pass:101, block:4899
96, 1589615290504, total:5000, pass:104, block:4896
95, 1589615291505, total:5000, pass:101, block:4899
94, 1589615292505, total:5000, pass:96, block:4899
...
```
Related to #1470 , now there are 100 threads, the pass qps is not accurate in in high concurrency.
In `SystemRuleManager#checkSystem` line 305:
`double currentQps = Constants.ENTRY_NODE == null ? 0.0 : Constants.ENTRY_NODE.successQps();`
modify `successQps()` to `passQps()`, result in console:
```
100, 1589616210408, total:4017, pass:100, block:4013
99, 1589616211408, total:5000, pass:10, block:4990
98, 1589616212409, total:5000, pass:11, block:4989
97, 1589616213410, total:4999, pass:4, block:4994
96, 1589616214409, total:5001, pass:5, block:4995
95, 1589616215411, total:5000, pass:13, block:4987
94, 1589616216411, total:4994, pass:8, block:4983
93, 1589616217412, total:5000, pass:4, block:4996
92, 1589616218412, total:4991, pass:11, block:4989
91, 1589616219414, total:5000, pass:9, block:4991
90, 1589616220414, total:5000, pass:2, block:4998
89, 1589616221415, total:5000, pass:3, block:4997
88, 1589616222415, total:5000, pass:3, block:4997
87, 1589616223416, total:5000, pass:3, block:4997
...
```
The result is better, less pass qps, not very accurate.

Above is in high concurrency, since in original demo: `private static final int threadCount = 100;`
Modify the threadCount to 1, and with `rule.setQps(1);`
result in cosole:
```
100, 1589615414932, total:41, pass:2, block:40
99, 1589615415933, total:50, pass:2, block:48
98, 1589615416933, total:50, pass:2, block:48
97, 1589615417934, total:50, pass:2, block:48
96, 1589615418934, total:50, pass:2, block:48
95, 1589615419934, total:50, pass:2, block:48
94, 1589615420935, total:50, pass:2, block:48
93, 1589615421935, total:50, pass:2, block:48
...
```
The qps of rule is 1, pass qps is 2 in result.

Go on and set `rule.setQps(2);` , result in console:
```
100, 1589615560453, total:42, pass:3, block:40
99, 1589615561453, total:50, pass:3, block:47
98, 1589615562454, total:50, pass:3, block:47
97, 1589615563455, total:49, pass:3, block:46
96, 1589615564455, total:50, pass:3, block:47
95, 1589615565456, total:50, pass:3, block:47
...
```
The pass qps 3 in is one more than qps set in system rule.

By debug and test, I find that `SystemRuleManager.checkSystem(resourceWrapper);` in `SystemSlot`
only use `resourceWrapper` as argument, without the new acquire count as `FlowRuleChecker` and `DegradeRuleManager` does.
Inside `SystemRuleManager#checkSystem`:
`if (currentQps > qps) `
`if (currentThread > maxThread)`
The new one acquire count is not calculated in juegement, this may have affected the result.

### Anything else we need to know?

Now the flow,paramFlow,authority rule has `XxxRuleChecker` class to do rule check logic, but degrade,system rule have not, maybe refactor and uniform them will be better.

Contributor guide

Open the contributing guide

Research direction

Start with SystemSlot and SystemRuleManager#checkSystem, then compare how FlowRuleChecker and DegradeRuleManager handle the new acquire count. Reproduce the cases in SystemGuardDemo with qps or maxThread set to 0, 1, and 2; done means the new acquire is included in system-rule checks and the observed pass counts match the configured limits.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.