-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 viz stat-inbound and viz stat-outbound commands #12994
Conversation
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
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.
I'm a big fan of this change. UX vastly improved and the code reads very well too!
I think most of my comments below for stat-inbound also apply to stat-outbound.
} | ||
for quantile, resultsChan := range results { | ||
go func(quantile string) { | ||
defer close(resultsChan) |
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.
Don't you need to pin resultsChan?
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.
Apparently not since all the quantile channels do get populated with this code. I think because channels are always passed by reference, the contents of the loop variable are already a reference (instead of being a reference to the loop variable).
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.
hmm I think that being a reference doesn't matter for how closures use those vars, but we likely are just benefiting from go 1.22's fix around this type of issue, so nothing to see here :-)
* cronjobs | ||
* daemonsets | ||
* deployments | ||
* namespaces | ||
* jobs | ||
* pods | ||
* replicasets | ||
* replicationcontrollers | ||
* statefulsets`, |
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.
Authority is still supported, I think it's worth mentioning it here.
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.
Hmm... technically this works but it kind of breaks the mental model of centering around concrete workloads. I'm tempted to explicitly make this not work. Is there a compelling use case for it?
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.
Not that I know of...
Signed-off-by: Alex Leong <alex@buoyant.io>
We add two new commands to the linkerd viz extension:
linkerd viz stat-inbound
andlinkerd viz stat-outbound
. These commands are meant as replacements for thelinkerd viz stat
. Thelinkerd viz stat
command provides stats when ServiceProfiles are used whereas the new commands provide stats when xRoute resources are used. Either command can be used when no xRoute or ServiceProfile is used but the new commands include several improvements:linkerd viz stat-outbound
to easily see the effects of retries, timeouts, and traffic splittingUnlike the
linkerd viz stat
command, these commands query prometheus directly rather than going through the intermediary of the metrics-api. If prometheus is enabled in linkerd-viz, these commands will use a port-forward to connect to that prometheus instance. If an external prometheus is configured, these commands will attempt to use that prometheus URL; however note that the prometheus URL must be reachable from where the CLI is executed for this to work. This can be overridden by a--prometheusURL
flag.Json and table output are both supported.