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.

Sign up for GitHub

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

Jump to bottom

filter: add ratelimit log #17902

Merged
merged 1 commit into from
Sep 24, 2021
Merged

filter: add ratelimit log #17902

merged 1 commit into from
Sep 24, 2021

Conversation

zirain
Copy link
Member

@zirain zirain commented Aug 28, 2021

Commit Message: add ratelimt log
Additional Description:
add some log to help debug ratelimt filter, in following cases specially:

  • ratelimt Status Error due when timeout was set to small, refer to istio PR
  • call grpc server onFailure

Risk Level: Low
Testing: manual testing
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@zirain zirain requested a review from mattklein123 as a code owner August 28, 2021 13:16
@repokitteh-read-only
Copy link

Hi @zirain, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17902 was opened by zirain.

see: more, trace.

@wbpcode
Copy link
Member

wbpcode commented Aug 29, 2021

Please add signature for every commit by git commit -s.

wbpcode
wbpcode requested changes Aug 29, 2021
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks. We do need some logs to help us to debug rate limit filter.

I add some initial comments to help us to complete this.

source/extensions/filters/http/ratelimit/ratelimit.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/ratelimit/ratelimit.cc Outdated Show resolved Hide resolved
davinci26
davinci26 suggested changes Sep 7, 2021
View reviewed changes
Copy link
Member

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

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

thanks! Got a small question. Please take a look at the format and DCO

source/extensions/filters/common/ratelimit/ratelimit_impl.cc Outdated Show resolved Hide resolved
@davinci26 davinci26 self-assigned this Sep 7, 2021
@davinci26
Copy link
Member

/wait

@zirain
Copy link
Member Author

zirain commented Sep 10, 2021

/wait

do we still wait something else?

@davinci26
Copy link
Member

Is the CI is passing the DCO and the format check?

@davinci26
Copy link
Member

You will need to apply the DCO on the first commit (follow the instructions here) which would require rewritting the git history of your PR.

Also take a look at https://dev.azure.com/cncf/envoy/_build/results?buildId=87580&view=logs&j=55bc2ab9-36c1-5ab8-58a6-8c258728eeff&t=33b57bf9-ef13-58c8-bc2b-f804aeb16d39 which indicates that you have some format errors

@wbpcode
Copy link
Member

wbpcode commented Sep 10, 2021

Please add signature for every commit by git commit -s.

And we need to format this code by tools/code_format/check_format.py fix.

These are the remaining problems.

@zirain zirain changed the title filter: add ratelimt log filter: add ratelimit log Sep 11, 2021
@zirain
Copy link
Member Author

zirain commented Sep 11, 2021

Please add signature for every commit by git commit -s.

And we need to format this code by tools/code_format/check_format.py fix.

These are the remaining problems.

All checks have passed.

davinci26
davinci26 previously approved these changes Sep 12, 2021
View reviewed changes
Copy link
Member

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM @envoyproxy/senior-maintainers for a second review

@zirain zirain requested a review from wbpcode September 13, 2021 01:03
wbpcode
wbpcode previously approved these changes Sep 13, 2021
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks. And waiting senior-maintainers to merge it.

mattklein123
mattklein123 requested changes Sep 13, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, small comment.

/wait

source/extensions/filters/common/ratelimit/ratelimit_impl.cc Outdated
Comment on lines 116 to 117
ENVOY_LOG_TO_LOGGER(Logger::Registry::getLog(Logger::Id::filter), warn,
"rate limit fail, status={} msg={}", status, msg);
Copy link
Member

Choose a reason for hiding this comment

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

This will log spam at high volume if the upstream RL server is having issues which I don't think is what we want. I would switch this to debug. There should already be a failure stat. If not we can add one?

Copy link
Member Author

Choose a reason for hiding this comment

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

there's nothing record right now when the upstream RL server is having issues, I think 'warn' is suitable in this scenario

Copy link
Member

Choose a reason for hiding this comment

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

As I said it's not suitable for log spamming reasons. If there is no other stat, you can also potentially use ENVOY_LOG_EVERY_POW_2 or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

switch it to debug

@mattklein123 mattklein123 self-assigned this Sep 13, 2021
@zirain
add ratelimit log
f3f3c15 
Signed-off-by: zirain <[email protected]>
@zirain zirain dismissed stale reviews from wbpcode and davinci26 via f3f3c15 September 15, 2021 01:58
@zirain
Copy link
Member Author

zirain commented Sep 24, 2021

Thanks, small comment.

/wait

@mattklein123 please check the code again

mattklein123
mattklein123 approved these changes Sep 24, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit ae85c31 into envoyproxy:main Sep 24, 2021
@zirain zirain deleted the ratelimit-tracelog branch December 12, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@davinci26 davinci26 Awaiting requested review from davinci26

@wbpcode wbpcode Awaiting requested review from wbpcode

Assignees

@mattklein123 mattklein123

@wbpcode wbpcode

@davinci26 davinci26

Labels
waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zirain @wbpcode @davinci26 @mattklein123