-
Notifications
You must be signed in to change notification settings - Fork 576
add stat prefix for routes #2405
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 stat prefix for routes #2405
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali can you add a release note for this? approval is pending on adding that :) |
Sure. I was thinking of adding it when the feature is implemented in Istio. I can add it if you prefer to add it here also. |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
@ericvn @hzxuzhonghu PTAL |
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
paging @douglas-reid |
How is this intended to function with the Telemetry API ? |
@louiscryan I think this is beyond the current scope of the Telemetry API. As I understand it, this is adding prefixes to envoy-generated stats, which has generally been distinct from the domain of the Telemetry API (which has focused on controlling Istio generation). Mostly, the envoy-level interactions for stats tend to be at the bootstrap level, which the Telemetry API has not tried to cover. The use case is not fully fleshed out here, but I imagine that @ramaraochavali desires to include some envoy-level metrics specific to endpoints. @ramaraochavali is that correct? The Telemetry API-based equivalent, iiuc, would be adding new labels to the istio metrics for path -- and using attribute-gen (or equivalent) to manage scoping of that generation for specific "high-value" destinations. That pattern is a bit complex, and involves bits outside the Telemetry API as well (existing docs). I think at a minimum we should be clearer in the documentation provided that this prefix is only for proxy-level stats ( |
Yes. This is to set the stat prefix to envoy route config so that it can generate route level stats.
I will update the docs. |
// The human readable prefix to use when emitting statistics for this route. | ||
// The statistics are generated with prefix route.<stat_prefix>. | ||
// This should be set for highly critical routes that one wishes to get “per-route” statistics on. | ||
string stat_prefix = 14; |
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.
can we please document that this is for the unstable envoy metrics, not the core Istio ones? thanks?
and maybe link to envoy docs for them
// The human readable prefix to use when emitting statistics for this route. | ||
// The statistics are generated with prefix route.<stat_prefix>. | ||
// This should be set for highly critical routes that one wishes to get “per-route” statistics on. | ||
string stat_prefix = 14; |
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 think this should not be in HTTPMatchRequest, which contains all matching conditions
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.
For each match we create a route - In Envoy it is exposed at route. I did not get why it can not be at Match? This is for path based routing. For each match, we want stat to be generated.
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 think stat_prefix
is to specify a behavior for a route after matching conditions, however this pr mess it in the match conditions.
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.
It is per match in Istio (translates to route in Envoy) to get path level stats.
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 can see it is aligned with envoy api
envoyproxy/envoy#21302 added support for route level stats in Envoy. This PR enables that in Istio