-
Notifications
You must be signed in to change notification settings - Fork 72
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
chore: add active users counter per rate limiter duration #3150
chore: add active users counter per rate limiter duration #3150
Conversation
🚨 Memory Leak Detected 🚨A potential memory leak has been detected in the test titled Details📊 Memory Leak Detection Report 📊 GC Type: MarkSweepCompact Heap Statistics (before vs after executing the test):
Heap Space Statistics (before vs after executing the test):
RecommendationsPlease investigate the memory allocations in this test, focusing on objects that are not being properly deallocated. |
Test Results 19 files + 2 272 suites +42 34m 0s ⏱️ + 4m 59s For more details on these failures, see this check. Results for commit 127fc31. ± Comparison against base commit 264d2cb. ♻️ This comment has been updated with latest results. |
c40af15
to
9c52deb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good change but not sure it's at the right level and might have side effects
540fdca
to
9ae5db5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to have this managed by the metrics service?
602c959
to
d8364e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've got to an agreement on the metric choice now
359ac49
to
07cdacf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! LGTM but left some questions and a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! One thing can be addressed in follow PR.
Great work! 🙌🏼
b880663
07cdacf
to
b880663
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2afbe1d
4d16990
to
720e403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 1 question
Signed-off-by: Nadezhda Popova <[email protected]>
Signed-off-by: Nadezhda Popova <[email protected]>
Signed-off-by: Nadezhda Popova <[email protected]>
Signed-off-by: Nadezhda Popova <[email protected]>
127fc31
720e403
to
127fc31
Compare
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3150 +/- ##
==========================================
- Coverage 77.85% 77.83% -0.03%
==========================================
Files 66 66
Lines 4453 4457 +4
Branches 993 998 +5
==========================================
+ Hits 3467 3469 +2
Misses 613 613
- Partials 373 375 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Description:
Add counter for active users based on the rate limiter duration period.
Related issue(s):
Fixes #3151
Notes for reviewer:
Checklist