Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[177] Add "warm-up" control behavior support #190

Merged
merged 13 commits into from
Aug 21, 2020

Conversation

binbin0325
Copy link
Collaborator

Describe what this PR does / why we need it

Add "warm-up" control behavior support

Does this pull request fix one issue?

#177

Describe how you did it

Describe how to verify it

run qps_warm_up_example.go

Special notes for reviews

core/config/constant.go Outdated Show resolved Hide resolved
core/flow/warm_up.go Outdated Show resolved Hide resolved
@sczyh30
Copy link
Member

sczyh30 commented Jul 27, 2020

Could you please also provide some unit tests (not just the example)?

@sczyh30 sczyh30 added area/flow-control Issues or PRs related to flow control to-review PRs to review labels Jul 27, 2020
@sczyh30 sczyh30 requested a review from louyuting July 27, 2020 03:35
@sczyh30 sczyh30 added this to the 0.6.0 milestone Jul 27, 2020
@sczyh30 sczyh30 linked an issue Jul 27, 2020 that may be closed by this pull request
@binbin0325
Copy link
Collaborator Author

Could you please also provide some unit tests (not just the example)?

Later I will try to add UT.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2020

Codecov Report

Merging #190 into master will decrease coverage by 0.54%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   43.99%   43.44%   -0.55%     
==========================================
  Files          80       81       +1     
  Lines        4453     4507      +54     
==========================================
- Hits         1959     1958       -1     
- Misses       2264     2319      +55     
  Partials      230      230              
Impacted Files Coverage Δ
core/flow/rule.go 0.00% <ø> (ø)
core/flow/rule_manager.go 54.61% <0.00%> (-0.86%) ⬇️
core/flow/tc_warm_up.go 0.00% <0.00%> (ø)
core/stat/base/bucket_leap_array.go 46.23% <ø> (+0.40%) ⬆️
core/stat/base/sliding_window_metric.go 22.58% <0.00%> (-0.30%) ⬇️
core/stat/base_node.go 25.00% <0.00%> (-1.48%) ⬇️

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 d0cf004...6fa4dc2. Read the comment docs.

core/base/stat_test.go Outdated Show resolved Hide resolved
core/flow/warm_up.go Outdated Show resolved Hide resolved
@sczyh30
Copy link
Member

sczyh30 commented Aug 11, 2020

Large amount of error occurred when I was running the warm-up example (printed in sentinel-record.log):

2020/08/11 10:18:14 logging.go:159: [default] [ERROR] Fail to get current(1597112293894) bucket, err: Provided time timeMillis=1597112293500 is already behind old.BucketStart=1597112303500.
github.com/alibaba/sentinel-golang/core/stat/base.(*LeapArray).currentBucketOfTime
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base/leap_array.go:194
github.com/alibaba/sentinel-golang/core/stat/base.(*BucketLeapArray).ValuesConditional
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base/bucket_leap_array.go:141
github.com/alibaba/sentinel-golang/core/stat/base.(*SlidingWindowMetric).getSumWithTime
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base/sliding_window_metric.go:93
github.com/alibaba/sentinel-golang/core/stat/base.(*SlidingWindowMetric).getQPSWithTime
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base/sliding_window_metric.go:108
github.com/alibaba/sentinel-golang/core/stat/base.(*SlidingWindowMetric).GetPreviousQPS
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base/sliding_window_metric.go:104
github.com/alibaba/sentinel-golang/core/stat.(*BaseStatNode).GetPreviousQPS
	/Users/sczyh30/temp/sentinel-go-fork/core/stat/base_node.go:41
github.com/alibaba/sentinel-golang/core/flow.(*WarmUpTrafficShapingCalculator).CalculateAllowedTokens
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/warm_up.go:49
github.com/alibaba/sentinel-golang/core/flow.(*TrafficShapingController).PerformChecking
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/traffic_shaping.go:44
github.com/alibaba/sentinel-golang/core/flow.checkInLocal
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/slot.go:70
github.com/alibaba/sentinel-golang/core/flow.canPassCheckWithFlag
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/slot.go:55
github.com/alibaba/sentinel-golang/core/flow.canPassCheck
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/slot.go:48
github.com/alibaba/sentinel-golang/core/flow.(*FlowSlot).Check
	/Users/sczyh30/temp/sentinel-go-fork/core/flow/slot.go:28
github.com/alibaba/sentinel-golang/core/base.(*SlotChain).Entry
	/Users/sczyh30/temp/sentinel-go-fork/core/base/slot_chain.go:153
github.com/alibaba/sentinel-golang/api.entry
	/Users/sczyh30/temp/sentinel-go-fork/api/api.go:143
github.com/alibaba/sentinel-golang/api.Entry
	/Users/sczyh30/temp/sentinel-go-fork/api/api.go:119
main.Task
	/Users/sczyh30/temp/sentinel-go-fork/example/warm_up/qps_warm_up_example.go:62
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1373.

core/flow/tc_warm_up.go Outdated Show resolved Hide resolved
@sczyh30
Copy link
Member

sczyh30 commented Aug 17, 2020

I've tested with the warm-up example and it works well 🤙

image

@sczyh30 sczyh30 added the kind/feature Category issues or PRs related to feature request label Aug 18, 2020
core/flow/tc_warm_up.go Outdated Show resolved Hide resolved
@@ -101,6 +101,10 @@ func (m *SlidingWindowMetric) GetQPS(event base.MetricEvent) float64 {
return m.getQPSWithTime(util.CurrentTimeMillis(), event)
}

func (m *SlidingWindowMetric) GetPreviousQPS(event base.MetricEvent) float64 {
return m.getQPSWithTime(util.CurrentTimeMillis()-uint64(m.bucketLengthInMs), event)
Copy link
Member

Choose a reason for hiding this comment

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

The initial design of the method is for getting the metric of last second (i.e. elapsed, "stable" second). Here we got the latest 1 sec (with current bucket, i.e. unstable bucket). We may discuss it later.

}

func NewWarmUpTrafficShapingCalculator(rule *FlowRule) *WarmUpTrafficShapingCalculator {
if rule.WarmUpColdFactor == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

The bad coldFactor value (<=0) should be filtered out when validating rules in flow rule manager (though checking is also needed here).

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM. We may improve the implementation later.

@sczyh30 sczyh30 merged commit ff74314 into alibaba:master Aug 21, 2020
@sczyh30 sczyh30 removed the to-review PRs to review label Aug 21, 2020
@sczyh30
Copy link
Member

sczyh30 commented Aug 21, 2020

Nice work. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flow-control Issues or PRs related to flow control kind/feature Category issues or PRs related to feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "warm-up" control behavior support
5 participants