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

feat: support HTTP compression on stats/prometheus #3201

Merged

Conversation

zufardhiyaulhaq
Copy link
Contributor

What type of PR is this?
envoy gateway expose prometheus format metrics on /stats/prometheus. prometheus query this endpoints and there are network cost between prometheus & envoy gateway.

add HTTP compression support for gzip, brotli, zstd

What this PR does / why we need it:
add HTTP compression support on /stats/prometheus
reduce network cost for getting the gateway metrics

Which issue(s) this PR fixes:
#3200

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>
@zufardhiyaulhaq zufardhiyaulhaq requested a review from a team as a code owner April 15, 2024 10:40
@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2024

since this translation may get complex, probably makes sense to generate a go struct, marshal into YAML and append that into the bootstrap

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>
Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>
@zufardhiyaulhaq
Copy link
Contributor Author

is the test flaky?

@zufardhiyaulhaq
Copy link
Contributor Author

/retest

1 similar comment
@zufardhiyaulhaq
Copy link
Contributor Author

/retest

@zufardhiyaulhaq
Copy link
Contributor Author

@zirain let me know if we can merge this.
Thanks!

@zirain
Copy link
Contributor

zirain commented Apr 17, 2024

@arkodg PTAL when you're back.

@@ -79,4 +79,7 @@ type ProxyOpenTelemetrySink struct {
type ProxyPrometheusProvider struct {
// Disable the Prometheus endpoint.
Disable bool `json:"disable,omitempty"`
// Configure the compression on Prometheus endpoint. Compression is useful in situations when bandwidth is scarce and large payloads can be effectively compressed at the expense of higher CPU load.
// +optional
Compression *Compression `json:"compression,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

the BTP API instantiated this as a list

Compression []*Compression `json:"compression,omitempty"`
to support multiple compression settings, to support heterogenous clients (and compression settings), do we also need to make it a list here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could 1 envoy route support multiple compression libraries? AFAIK we need to select only 1 library right?

Copy link
Contributor

Choose a reason for hiding this comment

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

make it to list but limit maxitem to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesn't make sense IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I think envoy can support multiple compressor libraries per route, which is the argument for making it a list
but realistically the persona who creates the EnvoyProxy resource with this setting, also knows beforehand the compression supported by the peer, so a single scalar value is fine here

@zufardhiyaulhaq zufardhiyaulhaq requested a review from arkodg April 18, 2024 14:10
@arkodg arkodg merged commit 0552052 into envoyproxy:main Apr 18, 2024
20 checks passed
@zufardhiyaulhaq zufardhiyaulhaq deleted the support-compression-prometheus branch June 30, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants