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 field to ratelimit descriptor #841

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

akondapuram
Copy link
Contributor

I created an issue with regard to adding detailed_metric support for dynamic ratelimiting using xds server a few weeks ago.
This is the first PR to address the same. Once this is merged, will add necessary changes to the ratelimit repo and create a new PR. Here is the draft PR.

@valerian-roche
Copy link
Contributor

Hey, can you clarify why this should be generated prior to the protobuf files being merged? It seems counter-intuitive to not allow users to regenerate based on the merged proto files

@ramaraochavali
Copy link

@phlax @kyessenov Can you please take a look?

@akondapuram
Copy link
Contributor Author

Hey, can you clarify why this should be generated prior to the protobuf files being merged? It seems counter-intuitive to not allow users to regenerate based on the merged proto files

@valerian-roche
Hi Valerian, thank you for looking into this PR.

From what I saw before from the conversation on the go-control-plane PR, the changes first were made on the go-control-plane repo. Please see the following two PRs.

  1. This PR was checked-in to go-control-plane on Jan 27th.
  2. The second PR to the ratelimit repo was then checked in on Feb 7th.
    Hence I created the first PR first (I generated it using the rls_conf.proto) and added the ratelimit PR draft as a reference in the this PR.

Also, the second PR can’t be merged in totality until the pb.go files are merged on the control plane side. Because there are several files (such as resource.go, config_xds.go) that depend on the rls_conf.pb.go file.

However, if you think the proto file has to be checked-in first on the ratelimit repo, I can split up this draft PR. I can first check-in the proto files to ratelimit repo, then check-in the generated file to go-control-plane repo and once this is merged, I can make the other changes in a separate PR. Please let me know your thoughts. Thank you!

@renuka-fernando Tagging you here for your thoughts/review as well, as you have contributed the dynamic ratelimiting feature to both the repos. Thanks!

@renuka-fernando
Copy link
Contributor

I guess this complexity is because we couldn't automate generating pb files as discussed in this issue #646.

@phlax, @jpeach, @dio, @arkodg could you please have a look?

@ramaraochavali
Copy link

@alecholmez Can you please review this PR?

@akondapuram
Copy link
Contributor Author

Thank you for your approval @valerian-roche Appreciate it.

@ramaraochavali
Copy link

@envoyproxy/go-control-plane Could you please help merge this?

@kyessenov kyessenov merged commit 841e293 into envoyproxy:main Jan 23, 2024
3 checks passed
@ramaraochavali
Copy link

@alecholmez Can you please help creating a release with this commit so that it can be used in gRLS service?

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.

5 participants