Skip to content
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

Support for enabling Request Response Size statistics #4234

Closed
luvk1412 opened this issue Sep 13, 2024 · 4 comments · Fixed by #4314
Closed

Support for enabling Request Response Size statistics #4234

luvk1412 opened this issue Sep 13, 2024 · 4 comments · Fixed by #4314
Assignees
Labels
area/observability Observability related issues

Comments

@luvk1412
Copy link
Contributor

I want to have plots of Request Response Size statistics in my eg setup which requires me to enable request_response_sizes in config.cluster.v3.TrackClusterStats.

In my knowledge there is currently no api in eg to do so(please correct me if am wrong) nor it is practically feasible to enable this using EnvoyPatchPolicy as we have lots of clusters and there is no way to select them all based on regex or something else, and adding for each cluster one by one is not feasible.

Request to add support for this. BackendTrafficPolicy seems a good place to do so to me maybe ?

@arkodg
Copy link
Contributor

arkodg commented Sep 14, 2024

@luvk1412
Copy link
Contributor Author

luvk1412 commented Sep 14, 2024

somehow i have missed this api in eg as we are not using this api as well. Thanks @arkodg for pointing this out.
ProxyMetrics does seems like a good place to make this as opt in. When we add it is as field here, it would enable it for all clusters and users won't be able to turn it on for specific clusters right ? I am assuming same is the case for enablePerEndpointStats as of today as it is also part of config.cluster.v3.TrackClusterStats , and if mentioned true, it gets enabled for all clusters with no option to enable for specific clusters ? I am fine with this behaviour for at least my use case but just want to clarify.

Also I am a little confused by the ProxyMetrics api:

  • what does enableVirtualHostStats corresponds to in envoy proxy config and what stats it exactly enables ?
  • sinks are supposed to only have push based sinks ? was thinking why prometheus is not part of sinks but is outside at first level, hence this came to mind.
  • Currently all fields are marked required in the documentation, i am thinking that might not be the case. It would be helpful if you could clarify what all is optional and what is not.

[OFF TOPIC]
A side note suggestion for documentation if it seems helpful to you guys: It would be helpful if we start documenting what corresponding field in envoy proxy, does a specific field in eg api corresponds to. This will be really helpful for people who want to deep dive a little or understand it a bit more. I personally have been reffering to eg code to find this out. Most recent example was where i wanted understand what HTTPRoute's timeouts corresponds to as there are two. If this seems reasonable and worth talking about, i can open a separate issue for this as well.

@luvk1412
Copy link
Contributor Author

I can work on this, please assign to me.

Proposed solution:
Add enableRequestResponseSizesStats in ProxyMetrics, enabling which would enable request_response_sizes in config.cluster.v3.TrackClusterStats for all clusters. Please let me know if this looks good.

@zirain zirain added area/observability Observability related issues and removed triage labels Sep 23, 2024
@zirain
Copy link
Contributor

zirain commented Sep 23, 2024

@luvk1412 thanks for picking up this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/observability Observability related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants