Skip to content

Conversation

ramaraochavali
Copy link
Contributor

envoyproxy/envoy#21302 added support for route level stats in Envoy. This PR enables that in Istio

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 2, 2022
@linsun
Copy link
Member

linsun commented Jul 6, 2022

@ramaraochavali can you add a release note for this? approval is pending on adding that :)

@ramaraochavali
Copy link
Contributor Author

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>
@istio-policy-bot
Copy link

🤔 🐛 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.

@ramaraochavali
Copy link
Contributor Author

@ericvn @hzxuzhonghu PTAL

Copy link

@ericvn ericvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@louiscryan
Copy link
Contributor

paging @douglas-reid

@louiscryan
Copy link
Contributor

How is this intended to function with the Telemetry API ?

@douglas-reid
Copy link
Contributor

@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 (envoy_*) and not service-level (istio_*) stats. It may also be worth noting that using this prefix may require bootstrap changes to stats_matcher configuration, etc., to properly use (although @ramaraochavali may solve that in the control plane implementation?).

@ramaraochavali
Copy link
Contributor Author

I imagine that @ramaraochavali desires to include some envoy-level metrics specific to endpoints. @ramaraochavali is that correct?

Yes. This is to set the stat prefix to envoy route config so that it can generate route level stats.

I think at a minimum we should be clearer in the documentation provided that this prefix is only for proxy-level stats (envoy_) and not service-level (istio_) stats. It may also be worth noting that using this prefix may require bootstrap changes to stats_matcher configuration, etc., to properly use (although @ramaraochavali may solve that in the control plane implementation?).

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;
Copy link
Member

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;
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants