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

Add detailed_metric support for xds-config #465

Merged
merged 7 commits into from
Feb 7, 2024

Conversation

akondapuram
Copy link
Contributor

@akondapuram akondapuram commented Dec 21, 2023

[Please NOTE] This PR has to be merged only after receiving approval for the PR on the go-control-plane.

  1. This PR adds support for detailed_metric field to dynamic ratelimiting in response to the issue I created a few weeks ago.
  2. The reason service RateLimitConfigDiscoveryService has been removed from this particular rls_conf.proto is because protoc would not compile it correctly. I think this was added after this file (rls_conf.pb.go) was generated and checked-in to go-control-plane repo.
  3. Please see commit for when rls_conf_ds.proto was initially added, but it was later removed as part of another commit.
  4. In the final commit rls_conf.proto was added along with service, but the corresponding go-control-plane's files were not updated. Hence, removing it to reduce confusion here. (Another possible solution is to separate it out to another file (rls_conf_ds.proto) like before.

@dmi3zkm
Copy link
Contributor

dmi3zkm commented Jan 11, 2024

Hey @envoyproxy/ratelimit-maintainers
Could you please review this PR?

@birdayz
Copy link
Contributor

birdayz commented Jan 29, 2024

bump :)
it would be really nice if we could merge this. the API part is merged into envoy's go-control-plane already.

@akondapuram
Copy link
Contributor Author

@birdayz Though the PR is merged, currently awaiting the release of the new version of the go-control-plane repo. The latest version is from 4 weeks ago and doesn't have my changes yet. https://pkg.go.dev/github.com/envoyproxy/go-control-plane/ratelimit/config/ratelimit/v3

@birdayz
Copy link
Contributor

birdayz commented Jan 29, 2024

Thanks for the quick response.
We could also depend on the commit 841e293a220b4d5abcc84ec0df41fd10a259ae42 directly (instead of using the tag), until the release of go-control-plane has happened. But i don't know if that is against some policy of the ratelimit project.

@akondapuram akondapuram marked this pull request as ready for review January 29, 2024 23:32
@akondapuram
Copy link
Contributor Author

@birdayz Thank you for the suggestion. I used commit instead of tag now and converted this PR from draft to review.
@renuka-fernando Could you please review when you get some time? Thank you in advance.

Signed-off-by: alekhya.kondapuram <[email protected]>
Signed-off-by: alekhya.kondapuram <[email protected]>
Signed-off-by: alekhya.kondapuram <[email protected]>
@renuka-fernando
Copy link
Contributor

Other changes LGTM.

@akondapuram
Copy link
Contributor Author

@renuka-fernando Thank you for your review and approval.

@akondapuram
Copy link
Contributor Author

@envoyproxy/ratelimit-maintainers Could you please help merge this PR? Thank you.

@ramaraochavali
Copy link

@mattklein123 @ysawa0 Can you please help merge this?

@mattklein123 mattklein123 self-assigned this Feb 5, 2024
mattklein123
mattklein123 previously approved these changes Feb 6, 2024
@mattklein123
Copy link
Member

Please check CI.

Also, is this a breaking change? It's a little unclear to me.

@akondapuram
Copy link
Contributor Author

@mattklein123 Thank you for looking into this. It's not a breaking change, but this has dependency on the go-control-plane repo as we rely on rls_conf.pb.go file. This was merged a couple of weeks ago. Hence I kept this PR in draft until that was merged. Converted it to review after the other PR was merged.

Regarding the CI, Looks like there was a pre-commit end-of-file failure for rls_conf_ds.proto. Pushed a new commit. Could you please try to re-run the checks when you have some bandwidth? Thank you.

@akondapuram
Copy link
Contributor Author

@mattklein123 Looks like all the checks are successful, could you please help merge this when you get some time? Thank you.

@mattklein123 mattklein123 merged commit 9901a9b into envoyproxy:main Feb 7, 2024
6 checks passed
@akondapuram
Copy link
Contributor Author

@mattklein123 Thank you for your review and approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants