NET 6409 #3158
NET 6409 #3158
Conversation
absolutelightning
added
pr/no-backport
|
Thanks @absolutelightning Will provide feedback later this week, also pinging SMEs for feedback. |
There was a problem hiding this comment.
Ashesh, excellent work. I think this will be very useful for many of our practitioners. I know I will use it for debugging.
I left some requested changes that bring this work in line with the existing patterns and practices that we have for this CLI. We want the user interface for the CLI to be as consistent as possible.
Please reach out if you have any questions about my feedback. I would love to help you get this PR merged as I think the functionality is great!
Co-authored-by: Thomas Eckert <[email protected]>
Thanks for review 🙏 I have updated the PR please check again. |
There was a problem hiding this comment.
Great! Thank you so much for adding this functionality. I added some comments on things that I would like you to change (they are small), but I went ahead and approved so that you can merge when you get the issues fixed up.
The most important comment that needs to be fixed is grabbing the correct namespace. You'll need to grab it from the Helm settings instead of the deployment configuration. This is because we want it to reflect the configuration in the users Kubectl. The namespace of the deployment is not that and is not generally where users will want to look for Envoy deployments (unless they are looking for gateway configurations).
|
I think this is great, thank you for doing this. I can't tell from the PR, but does the CLI provide a new line right after the command is issued? I think that is necessary to at least provide some separation between CLI output and the CLI command itself. To close this out we should also provide docs for this feature here https://developer.hashicorp.com/consul/docs/k8s/k8s-cli#proxy and add it after the proxy log command. We should be explicit to also note that this is cluster stats we are outputting as opposed to XDS Server stats: https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/statistics There may be additional feedback in the future but I think this is a great start and will tremendously help troubleshooting. |
Yes there is a new line after the command. It looks like this Also I have created the PR in consul to update the docs. - hashicorp/consul#19515 |
david-yu
added
backport/1.3.x
and removed
pr/no-backport
|
Could we backport to 1.3.x, 1.2.x and 1.1.x? |
david-yu
added
backport/1.1.x
|
This can be merged without waiting for patch releases, only the docs need to wait for the releases. |
|
@t-eckert - Not able to merge. Tests are failing which are not related to my change. |
* init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]>
* init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]>
* init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]>
* backport of commit de72bfd * NET 6409 (#3158) * init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]> * removed envoy stats --------- Co-authored-by: absolutelightning <[email protected]> Co-authored-by: Ashesh Vidyut <[email protected]> Co-authored-by: Thomas Eckert <[email protected]>
* backport of commit de72bfd * backport of commit 1f649bf * backport of commit 3f9597f * backport of commit 20615bf * backport of commit 1c673af * backport of commit 7abbbfa * backport of commit c5fb109 * backport of commit 4218e93 * backport of commit c78acd3 * NET 6409 (#3158) * init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]> * removed envoy stats * removed envoy stats --------- Co-authored-by: absolutelightning <[email protected]> Co-authored-by: Ashesh Vidyut <[email protected]> Co-authored-by: Thomas Eckert <[email protected]>
* init * fix help and synopsis * added some tests * change log * some fixes * rename var name * tests for get envoy stats * fix tests * Update cli/cmd/envoy-stats/envoy_stats.go Co-authored-by: Thomas Eckert <[email protected]> * proxy stats command * fix command options * pr comment resolved * fix globaloptions * fix lint --------- Co-authored-by: Thomas Eckert <[email protected]>
Changes proposed in this PR:
Fixes - #2433
consul-k8s CLI: Display envoy cluster stats using consul-k8s CLI
How I've tested this PR:
make cli-dev./consul-k8s proxy stats -pod nginx-6d7469694f-rdmwp -kubeconfig /Users/asheshvidyut/.kube/config -namespace default > nginxcat nginxHow I expect reviewers to test this PR:
Checklist: