ratelimit
ratelimit copied to clipboard
envoyproxy
Add detailed metrics
This was partially implemented in https://github.com/envoyproxy/ratelimit/pull/242
Pretty much tried to massage the missing parts from this PR https://github.com/envoyproxy/ratelimit/pull/237 into main.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
@jdamata I am interested in this functionality. Is this PR ready to be reviewed or are more changes required?
I think this is ready for review, was using the ratelimit with this PR for a few weeks in a load testing environment and it was working without issues.
I think some of the checks were failing because of linting but otherwise good to go
Ok sounds good thanks. @jdamata just to confirm, when the flag is enabled, stats are created as if the config had each value in it for each value matched?
Like lets say the config has descriptors: key: user rate_limit: ...
And user a and b both match, then the stats will be domain.user.a.total_hits and domain.user.b.total_hits, etc ?
@mattklein123 @ysawa0 could one of you take a look at this PR please (to rerun the tests since the logs have been deleted)?
@jdamata could you rebase from main please so the build checks pass? Thanks. Also, if you need help fixing the tests let me know.
@jdamata you might need to force push with "Signed-off-by: Joel Damata [email protected]" as the commit message since the last commit didn't have it making the DCO check fail.
@jdamata Just wanted to follow up on this. Could you force push to get DCO to pass? You'll have to also make the PR be from your feature branch to main, as right now it's the opposite. Thanks.
@jdamata looks good! Could check that failing test? I think it's formatting issues. Thanks.
@jdamata Thanks for updating this. Looks to be a very in-demand PR. Could you fix the DCO check? You'll have to force push with "Signed-off-by: Joel Damata [email protected]" as the commit message. Thanks.
@jdamata DCO seems to still be failing. The error message is:
Author: damatj, Committer: damatj; Expected "damatj [[email protected]]", but got "Joel Damata [[email protected]]"
You'll need to change the author and force push again with the new audit of "damatj". Thanks!
@jdamata hi, don't know if you saw but there's a failing test: ratelimit_test.go:207
Hi, @jdamata! Can you, please, give us an estimate on when this can be merged, so we can plan accordingly? We're depending on this to move forward with our project. Appreciate it!
@jdamata sorry to bother you, but many people are looking forward to your addition.
If you are not able to work on it, I can help if you want?
@jdamata sorry to bother you, but many people are looking forward to your addition.
If you are not able to work on it, I can help if you want?
No worries! Sorry yah I've been busy with work lately. I'd appreciate the help if you got the time.
I don't care for the git commit credit so if you want to branch out, close this pr and open a new one by all means go for it!
Ok sounds good, I'll do that. Just to confirm, you said this is working when you tested it for your purposes right?
Ok sounds good, I'll do that. Just to confirm, you said this is working when you tested it for your purposes right?
Yep, I had built my own docker image from this branch and had it working in a performance environment. Didn't notice any issues. Was able to get Prometheus to display metrics for header values and grafana graphs. I think that's what people are after