-
Notifications
You must be signed in to change notification settings - Fork 437
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
feat: expose stats tags in gateway-proxy stats config #8333
feat: expose stats tags in gateway-proxy stats config #8333
Conversation
Waiting for approval from someone in the solo-io org to start testing. |
90177b9
to
b5b9356
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.
Thanks for the contribution @jlourenc!
I like the idea here, there are just a few changes that we'll need to make:
- https://github.com/solo-io/gloo/blob/main/install/helm/gloo/generate/values.go is the source of truth for our helm docs. If you update the configuration you want to modify, and then run code-generation, the helm reference docs will be dynamically updated
- For any helm changes, we use https://github.com/solo-io/gloo/blob/main/install/test/helm_test.go to write a unit test to assert that the generated manifests match what we would expect. https://github.com/solo-io/gloo/blob/main/install/test/helm_test.go#L3607 is an example of a test that will be similar to yours (ie we apply some values, render the manifests, and assert the existence of the injected configuration)
- Have you explored https://docs.solo.io/gloo-edge/latest/reference/helm_chart_values/open_source_helm_chart_values/#helm-chart-kuberesourceoverrides? We expose a way to inject custom values to a lot of our templates, and perhaps you can use this to define the stats configuration generically (without being blocked by this helm change)
c86859d
to
d78cc8a
Compare
Hey @sam-heilbron , thanks for taking a look.
|
d78cc8a
to
cea84bb
Compare
/test |
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.
Looking great! Sorry for the delay in getting back to you
cea84bb
to
55dd295
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.
Looking good! I have a few suggestions to tighten up the test.
41633fa
to
83d7463
Compare
Allows configuring Envoy stats_config propety in Gateway Proxy.
83d7463
to
1f485f6
Compare
I have taken your suggestions onboard to update the test. Please take another look 🙏🏼 |
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, but please don't squash between reviews in the future! It takes away the ability to see just the changes since the review. Thanks for the feature!
/test run build-bot |
Co-authored-by: Rachael Graham <rachael.graham@solo.io>
/kick not sure what's hung up |
/test run build-bot |
/test rerun build bot with new changes |
* feat: expose Envoy statistics configuration in gwp Allows configuring Envoy stats_config propety in Gateway Proxy. * Update install/helm/gloo/generate/values.go Co-authored-by: Rachael Graham <rachael.graham@solo.io> * changelog and update docs * Update gwp-stats-config.yaml --------- Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com> Co-authored-by: Rachael Graham <rachael.graham@solo.io> Co-authored-by: sheidkamp <seth.heidkamp@gmail.com>
* feat: expose stats tags in gateway-proxy stats config (#8333) * feat: expose Envoy statistics configuration in gwp Allows configuring Envoy stats_config propety in Gateway Proxy. * Update install/helm/gloo/generate/values.go Co-authored-by: Rachael Graham <rachael.graham@solo.io> * changelog and update docs * Update gwp-stats-config.yaml --------- Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com> Co-authored-by: Rachael Graham <rachael.graham@solo.io> Co-authored-by: sheidkamp <seth.heidkamp@gmail.com> * move cl * Update gwp-stats-config.yaml * Update gwp-stats-config.yaml --------- Co-authored-by: Jérémy Lourenço <jeremy.lourenco@snyk.io> Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com> Co-authored-by: Rachael Graham <rachael.graham@solo.io> Co-authored-by: sheidkamp <seth.heidkamp@gmail.com>
Description
This new feature can be used to configure gateway proxies stats configuration with stats tags.
Checklist:
make -B install-go-tools generated-code
to ensure there will be no code diff