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

Support arbitrary statistic interval for flow control && stat parameters configurable #200

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

louyuting
Copy link
Collaborator

@louyuting louyuting commented Aug 4, 2020

Describe what this PR does / why we need it

Support arbitrary statistic interval for normal flow rule
stat parameters configurable

Does this pull request fix one issue?

Resolve
#129
#167

Describe how you did it

Support arbitrary statistic interval for normal flow rule

  1. If user doesn't set StatisticConfig, that means using default metric statistic.
    2.If user specifies StatisticConfig and meets the compliance of CheckValidityForReuseStatistic,
    will build readonly metric statistic based on resource's global statistic.
    3.If user specifies StatisticConfig and doesn't meet the compliance of CheckValidityForReuseStatistic,
    will generate independent token bucket statistic for this rule.

Describe how to verify it

TODO

  1. unit test
  2. integration test

Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2020

Codecov Report

Merging #200 into master will increase coverage by 1.17%.
The diff coverage is 57.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   41.89%   43.06%   +1.17%     
==========================================
  Files          77       79       +2     
  Lines        3817     3994     +177     
==========================================
+ Hits         1599     1720     +121     
- Misses       1977     2009      +32     
- Partials      241      265      +24     
Impacted Files Coverage Δ
core/config/config.go 21.50% <0.00%> (-1.23%) ⬇️
core/flow/tc_warm_up.go 0.00% <0.00%> (ø)
core/config/entity.go 51.51% <50.00%> (-0.28%) ⬇️
core/flow/rule.go 19.04% <50.00%> (+19.04%) ⬆️
core/flow/tc_throttling.go 62.96% <50.00%> (-1.04%) ⬇️
core/flow/rule_manager.go 61.06% <55.76%> (+0.07%) ⬆️
core/stat/base/sliding_window_metric.go 17.82% <60.00%> (-4.56%) ⬇️
core/flow/standalone_stat_slot.go 66.66% <66.66%> (ø)
core/base/stat.go 78.57% <78.57%> (ø)
core/flow/tc_default.go 68.75% <81.81%> (+46.52%) ⬆️
... and 9 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 7ddba92...cf49660. Read the comment docs.

@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from f28d2b9 to bd7baec Compare August 4, 2020 16:39
@sczyh30 sczyh30 added area/flow-control Issues or PRs related to flow control kind/feature Category issues or PRs related to feature request size/XXL Indicate a PR that changes 1000+ lines. labels Aug 5, 2020
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch 2 times, most recently from e9f2d77 to 77c9ad4 Compare August 6, 2020 14:29
@louyuting louyuting requested a review from sczyh30 August 6, 2020 14:32
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from 77c9ad4 to 5205051 Compare August 12, 2020 15:47
@louyuting louyuting added this to the 0.6.0 milestone Aug 14, 2020
@louyuting louyuting marked this pull request as ready for review August 14, 2020 15:26
@louyuting
Copy link
Collaborator Author

@sczyh30 Could you please review this large PR?

@sczyh30 sczyh30 self-assigned this Aug 17, 2020
@louyuting louyuting added the to-review PRs to review label Aug 17, 2020
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch 4 times, most recently from 85dc2ce to 875060e Compare August 23, 2020 04:44
core/flow/rule_manager.go Outdated Show resolved Hide resolved
core/flow/rule_manager.go Outdated Show resolved Hide resolved
core/flow/rule_manager.go Outdated Show resolved Hide resolved
core/flow/rule_manager.go Outdated Show resolved Hide resolved
core/base/stat.go Outdated Show resolved Hide resolved
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from 875060e to 444dfe2 Compare August 25, 2020 12:19
@louyuting louyuting removed this from the 0.6.0 milestone Aug 27, 2020
core/base/stat.go Outdated Show resolved Hide resolved
core/config/entity.go Show resolved Hide resolved
@louyuting louyuting added this to the 0.7.0 milestone Sep 2, 2020
@louyuting louyuting changed the title Support arbitrary statistic interval for normal flow rule/stat parameters configurable/break down ControlBehavior Support arbitrary statistic interval for flow control && stat parameters configurable Sep 8, 2020
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from ede8121 to 2d2bf98 Compare September 8, 2020 16:21
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from 2d2bf98 to 854ed30 Compare September 8, 2020 16:30
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from 854ed30 to 05e9d1f Compare September 9, 2020 16:14
@louyuting louyuting removed the size/XXL Indicate a PR that changes 1000+ lines. label Sep 10, 2020
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from 05e9d1f to 00ebf30 Compare September 20, 2020 09:38
@louyuting louyuting modified the milestones: 0.7.0, 1.0.0 Sep 20, 2020
@louyuting louyuting force-pushed the 20200801-statistci-configurable branch 2 times, most recently from 0b57285 to cc1d73c Compare September 22, 2020 14:50
@sczyh30 sczyh30 added the size/XXL Indicate a PR that changes 1000+ lines. label Sep 22, 2020
core/flow/rule_manager.go Outdated Show resolved Hide resolved
core/flow/rule_manager.go Outdated Show resolved Hide resolved
@sczyh30
Copy link
Member

sczyh30 commented Sep 22, 2020

And we may need to support the scenario when only one of sampleCount or intervalMs is provided (use the default value for another).

@louyuting louyuting force-pushed the 20200801-statistci-configurable branch from fcb4269 to 79a18bf Compare September 23, 2020 14:44
core/flow/tc_default.go Outdated Show resolved Hide resolved
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

@sczyh30 sczyh30 merged commit 4e8f5a0 into alibaba:master Sep 25, 2020
@sczyh30
Copy link
Member

sczyh30 commented Sep 25, 2020

Awesome. Thanks!

@sczyh30 sczyh30 removed the to-review PRs to review label Sep 25, 2020
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 size/XXL Indicate a PR that changes 1000+ lines.
Projects
None yet
5 participants