feat(translator): Set statPrefix for HCM and TCPProxy

[aoledk]

What this PR does / why we need it:

Improve metric observability.

• set http-{port} for HTTP listener
• set https-{port} for HTTPS listener
• set tcp-{port} for TCP listener
• set tls-passthrough-{port} for TLS passthrough listener
• set tls-terminate-{port} for TLS terminate listener

Which issue(s) this PR fixes:

xref #3160

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.59%. Comparing base (f4c53f4) to head (6345734).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3728   +/-   ##
=======================================
  Coverage   67.59%   67.59%           
=======================================
  Files         183      183           
  Lines       22444    22446    +2     
=======================================
+ Hits        15171    15173    +2     
+ Misses       6194     6193    -1     
- Partials     1079     1080    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zirain]

I'm not sure this's a good idea or not, especially put hostname on stats, which means more memory cost.

[aoledk]

I put hostname in statPrefix to allow user to inspect metric of https or terminate listener on specific hostname. This will leading to more memory cost by envoy stats module and bandwidth cost between envoy and prometheus.

• contour use {http/https/tcp}-{port} as statPrefix ¹.
• emissary use {gatewayNamespace}-{gatewayName}-{listenerIndex} as statPrefix ².
• istio can use multiple ways to set statPrefix ³, without hostname.

Maybe we can do similar things as contour, remove {hostname} based on previous proposal.

Footnotes

1. https://github.com/projectcontour/contour/blob/20d2ed91fdd616e1122a64a6520bdd6e4c7016f5/internal/gatewayapi/listeners.go#L184 ↩

2. https://github.com/emissary-ingress/emissary/blob/5f7ac3008006082e44f602ac048610d298906ed1/pkg/gateway/gw_transforms.go#L31 ↩

3. https://github.com/istio/istio/blob/8f4506bf3d4526d7bee254b9fc390459ba555354/pilot/pkg/networking/core/listener_builder.go#L301 ↩

[zirain]

I put hostname in statPrefix to allow user to inspect metric of https or terminate listener on specific hostname. This will leading to more memory cost by envoy stats module and bandwidth cost between envoy and prometheus.

• contour use {http/https/tcp}-{port} as statPrefix 1.
• emissary use {gatewayNamespace}-{gatewayName}-{listenerIndex} as statPrefix 2.
• istio can use multiple ways to set statPrefix 3, without hostname.

Maybe we can do similar things as contour, remove {hostname} based on previous proposal.

Footnotes

1. https://github.com/projectcontour/contour/blob/20d2ed91fdd616e1122a64a6520bdd6e4c7016f5/internal/gatewayapi/listeners.go#L184 ↩
2. https://github.com/emissary-ingress/emissary/blob/5f7ac3008006082e44f602ac048610d298906ed1/pkg/gateway/gw_transforms.go#L31 ↩
3. https://github.com/istio/istio/blob/8f4506bf3d4526d7bee254b9fc390459ba555354/pilot/pkg/networking/core/listener_builder.go#L301 ↩

it should be as concise as possible or user input.

[aoledk]

Then I prefer to {http/https/tcp/passthrough/terminate}-{port}, usually port maybe configured on cloud provider's load balancer target group, what's your option ? @zirain

[zirain]

Then I prefer to {http/https/tcp/passthrough/terminate}-{port}, usually port maybe configured on cloud provider's load balancer target group, what's your option ? @zirain

looks better, what's the different between https/passthrough/terminate?

[aoledk]

http / tcp / https / passthrough / terminate are current statPrefix for HCM or TCPProxy:

• https for HTTPS listener with TLS certificates.
• passthrough for TLS listener without TLS certificates, just to inspect SNI for routing.
• terminate for TLS listener with TLS certificates.

[zirain]

http / tcp / https / passthrough / terminate are current statPrefix for HCM or TCPProxy:

• https for HTTPS listener with TLS certificates.
• passthrough for TLS listener without TLS certificates, just to inspect SNI for routing.
• terminate for TLS listener with TLS certificates.

what's the different between HTTPS and TLS?

[zhaohuabing]

http / tcp / https / passthrough / terminate are current statPrefix for HCM or TCPProxy:

• https for HTTPS listener with TLS certificates.
• passthrough for TLS listener without TLS certificates, just to inspect SNI for routing.
• terminate for TLS listener with TLS certificates.

what's the different between HTTPS and TLS?

I think the alpns are HTTP vs others. These terminologies are quite confusing, I need to think a moment every time I see them.

These names could be more explicit: (plain)http/https(-terminate)/(plain)tcp/tls-passthrought/tls-terminate

[aoledk]

https means it's a Gateway API listener whose protocol is HTTPS, will be HCM eventually. tls means it's a Gateway API listener whose protocol is TLS, will be TCPProxy eventually.

[aoledk]

http / tcp / https / passthrough / terminate are current statPrefix for HCM or TCPProxy:

• https for HTTPS listener with TLS certificates.

• passthrough for TLS listener without TLS certificates, just to inspect SNI for routing.

• terminate for TLS listener with TLS certificates.

what's the different between HTTPS and TLS?

I think the alpns are HTTP vs others. These terminologies are quite confusing, I need to think a moment every time I see them.

These names could be more explicit: (plain)http/https(-terminate)/(plain)tcp/tls-passthrought/tls-terminate

Leading with tls prefix for passthrough and terminate is a good idea.

[zhaohuabing]

@aoledk In the example of this issue #3160 , port is part of the envoy_listener_address lable of the metric. Do all the metrics have this label? If that's the case, we don't need to add the port to the stats prefix.

envoy_listener_http_downstream_rq_completed{envoy_http_conn_manager_prefix="https",envoy_listener_address="0.0.0.0_8080"} 0

[aoledk]

@aoledk In the example of this issue #3160 , port is part of the envoy_listener_address lable of the metric. Do all the metrics have this label? If that's the case, we don't need to add the port to the stats prefix.

envoy_listener_http_downstream_rq_completed{envoy_http_conn_manager_prefix="https",envoy_listener_address="0.0.0.0_8080"} 0

Only Per listener statistics of HCM will have envoy_listener_address tag and it just covers very small part of all metrics of HCM. TCPProxy doesn't have Per listener statistics like HCM.

So IMO port should be added to statPrefix.

[zhaohuabing]

LGTM. Thanks for adding the "tls-" prefix to the tls passthrough and tls terminate listeners, which makes the stat prefix easier to understand.

[guydc]

@aoledk - can you regenerate and update the PR description?

[aoledk]

@aoledk - can you regenerate and update the PR description?

Done.

[arkodg]

can you check why tests are failing @aoledk ?
also would be great if you can share what the metrics looked like now vs earlier

[aoledk]

can you check why tests are failing @aoledk ? also would be great if you can share what the metrics looked like now vs earlier

previous looks like

envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="admin"} 19
envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="eg-ready-http"} 45
envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="http-10080"} 0

now looks like

envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="admin"} 19
envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="eg-ready-http"} 45
envoy_http_downstream_rq_total{envoy_http_conn_manager_prefix="http"} 0

[arkodg]

thanks for fixing the e2e test @aoledk, does this change impact the grafana dashboards as well https://gateway.envoyproxy.io/docs/tasks/observability/grafana-integration/
cc @shawnh2

[zirain]

/retest

[aoledk]

/retest

[zirain]

LGTM, thanks for adding this.