Support process level access log rate limiting#40992
Support process level access log rate limiting#40992kyessenov merged 70 commits intoenvoyproxy:mainfrom
Conversation
|
/assign-from @envoyproxy/first-pass-reviewers |
|
/assign @adisuissa |
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
Fix the lock issue please. Also make sure to add a test that recreates a logger with the same key, since it should have been caught by a test.
DONE |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
Added a couple of API comments.
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
Logic seems right, so it's style questions. @adisuissa for API review.
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
I've left a high-level comment.
Please also add integration tests as part of this work.
Thx! Integration test has been added. |
Signed-off-by: Xuyang Tao <taoxuy@google.com>
|
/retest |
|
Hi @wbpcode are you still the assigned reviewer, could you please take another look? Thx! |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
LGTM with only one nit comment. Thanks for this contribution.
|
Could you resolve the conflict also? Thanks. |
…ter.cc Co-authored-by: code <wbphub@gmail.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
|
/retest |
DONE. |
DONE. Please help merge the PR thx! |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks so much for your great contribution and all the patience for this works!!!
|
Hello, @kyessenov there is a request change from you, could you take a check to that and could we merge it? Thanks. |
|
|
||
| bool ProcessRateLimitFilter::evaluate(const Formatter::Context&, | ||
| const StreamInfo::StreamInfo&) const { | ||
| ENVOY_BUG(rate_limiter_->getLimiter() != nullptr, |
There was a problem hiding this comment.
Is there some metric that shows that access logs are dropped?
Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Xuyang Tao <taoxuy@google.com>
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
LG, one minor error - a duplicated line.
|
/retest |
1 similar comment
|
/retest |
…0992) Commit Message: support process level access log rate limiting Additional Description: this is used to rate limit access log emission on the process level. A typical usage is to rate limit logging to stdout or file when you have multiple access loggers. Fix envoyproxy#40103 Risk Level: NA as this is a new extension Testing: unit tests and integration test are added Docs Changes: updated Release Notes: updated Platform Specific Features:NA --------- Signed-off-by: Xuyang Tao <taoxuy@google.com> Co-authored-by: code <wbphub@gmail.com> Signed-off-by: Gustavo <grnmeira@gmail.com>
5 participants
Commit Message: support process level access log rate limiting
Additional Description: this is used to rate limit access log emission on the process level. A typical usage is to rate limit logging to stdout or file when you have multiple access loggers. Fix #40103
Risk Level: NA as this is a new extension
Testing: unit tests and integration test are added
Docs Changes: updated
Release Notes: updated
Platform Specific Features:NA