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 Endpoint Stats #2824

Closed
arkodg opened this issue Mar 7, 2024 · 9 comments · Fixed by #3145
Closed

Support Endpoint Stats #2824

arkodg opened this issue Mar 7, 2024 · 9 comments · Fixed by #3145
Assignees
Labels
area/observability Observability related issues
Milestone

Comments

@arkodg
Copy link
Contributor

arkodg commented Mar 7, 2024

Description:

Describe the desired behavior, what scenario it enables and how it
would be used.

Add support for enabling per endpoint stats in envoy
envoyproxy/envoy#29709

https://gateway.envoyproxy.io/latest/api/extension_types/#proxymetrics

Will help with traffic splitting telemetry #2809

@arkodg arkodg added triage help wanted Extra attention is needed area/observability Observability related issues and removed triage labels Mar 7, 2024
@arkodg arkodg modified the milestones: v1.0.0, Backlog Mar 7, 2024
@ShyunnY
Copy link
Contributor

ShyunnY commented Mar 12, 2024

I want to try doing this, but I'm not sure if I have the right idea. Do we just need a bool as a switch to enable per endpoint stats on envoy? can you tell me more :)

@arkodg
Copy link
Contributor Author

arkodg commented Mar 12, 2024

yup bool pointer is the way to go here :)

@arkodg arkodg removed the help wanted Extra attention is needed label Mar 12, 2024
@ShyunnY
Copy link
Contributor

ShyunnY commented Mar 16, 2024

When implementing this feature: I will put EnableEndpointStats in ir.Metrics, since adding this feature will require setting the cluster in the buildXdsCluster function.

There is currently a problem: buildXdsCluster is called by different processXXXListenerXdsTranslation, and the parameters of these functions are not consistent. If we plan to support endpoint metrics for all protocols, this will change the parameters of many functions. Big changes to the code.

Currently we are considering whether to enable EnableEndpointStats only under HTTP, or whether to enable it under all protocols. I want to hear your opinions. Thanks :)

@ShyunnY
Copy link
Contributor

ShyunnY commented Mar 16, 2024

cc @zirain @Xunzhuo

@ShyunnY
Copy link
Contributor

ShyunnY commented Apr 8, 2024

@arkodg WDYT comment

@arkodg
Copy link
Contributor Author

arkodg commented Apr 8, 2024

not sure I exactly understand @ShyunnY, here's what Im thinking

@ShyunnY
Copy link
Contributor

ShyunnY commented Apr 8, 2024

Yes, you're absolutely right, that's exactly what I was thinking.

But another problem arises at this time:
The upper caller of buildXdsCluster is addXdsCluster, and addXdsCluster is called by different callers. We will find a phenomenon: *ir.Metrics seems to only appear in processHTTPListenerXdsTranslation:

func (t *Translator) processHTTPListenerXdsTranslation(
tCtx *types.ResourceVersionTable,
httpListeners []*ir.HTTPListener,
accessLog *ir.AccessLog,
tracing *ir.Tracing,
metrics *ir.Metrics,
) error {

Based on the information above, going back to the original comment, do we currently only support endpoint stats under HTTP? This question has made me hesitant about how to proceed with writing this PR, because I'm not sure.
@arkodg

@arkodg
Copy link
Contributor Author

arkodg commented Apr 8, 2024

we should support for all, the existing field in that IR (virtualHostStats) is related to HTTP, so the initial commit was only applied to HTTP

@zirain
Copy link
Member

zirain commented Apr 9, 2024

will this cause high cost of memory?

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