support stats compression #47997
support stats compression #47997
Conversation
istio-testing
added
the
size/M
istio-testing
added
size/XL
pkg/bootstrap/config.go
Outdated
| @@ -274,6 +274,12 @@ func getStatsOptions(meta *model.BootstrapNodeMetadata) []option.Instance { | |||
| } | |||
| } | |||
|
|
|||
| var compression string | |||
| // TODO: move annotation to api repo | |||
| if statsCompression, ok := meta.Annotations["sidecar.istio.io/statsCompression"]; ok { | |||
There was a problem hiding this comment.
If we are adding a new annotation, there should be a release note to highlight it
There was a problem hiding this comment.
Sanity check, do we also need a proxy version check here? Asking because I'm not sure if this is tied to any recent istio/proxy or recent commits in upstream envoyproxy repo.
There was a problem hiding this comment.
We should make it an environment variable. IMO this will not be a permanent API. We should:
- Turn off by default with opt in
- Turn on by default with opt-out
- Remove opt-out, if no one needs it
If we are really confident I am ok with skipping (1).
There was a problem hiding this comment.
as @kyessenov suggest, we can exend to support zst or brotli, so maybe it can be a permanent API.
pkg/bootstrap/config.go
Outdated
| @@ -274,6 +274,12 @@ func getStatsOptions(meta *model.BootstrapNodeMetadata) []option.Instance { | |||
| } | |||
| } | |||
|
|
|||
| var compression string | |||
| // TODO: move annotation to api repo | |||
| if statsCompression, ok := meta.Annotations["sidecar.istio.io/statsCompression"]; ok { | |||
There was a problem hiding this comment.
We should make it an environment variable. IMO this will not be a permanent API. We should:
- Turn off by default with opt in
- Turn on by default with opt-out
- Remove opt-out, if no one needs it
If we are really confident I am ok with skipping (1).
|
Can you add the commits from #47934 and/or make @kaiburjack a co-author on the commits? |
I don't really need/want to take credit for this in any way, since I wasn't the one coming up with the original solution anyways. If at all, it should be @svenwltr via #30987 (comment) |
|
Isn't it negotiated by the client (Prometheus)? Envoy can just accept any
of them and client can pick
…On Wed, Nov 22, 2023, 4:02 PM zirain ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/bootstrap/config.go
<#47997 (comment)>:
> @@ -274,6 +274,12 @@ func getStatsOptions(meta *model.BootstrapNodeMetadata) []option.Instance {
}
}
+ var compression string
+ // TODO: move annotation to api repo
+ if statsCompression, ok := meta.Annotations["sidecar.istio.io/statsCompression"]; ok {
as @kyessenov <https://github.com/kyessenov> suggest, we can exend to
support zst or brotli, so maybe it can be a permanent API.
—
Reply to this email directly, view it on GitHub
<#47997 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXOTIQZCM7ULKLGLVYTYF2HCZAVCNFSM6AAAAAA7WFS2VSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBVGU2DKOJSGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
istio-testing
added
size/XXL
|
FYI: I've done some testing with a simple Go HTTP proxy sidecar doing gzip compression for the istio-proxy sidecar's 15090 /stats/prometheus and declaring a new port 14090 to compress the response. |
|
It seems for agent metrics or for those using merged metrics via the agent at 15020 we are still at the mercy of the agent server supporting/not supporting compression. While kaiburjack's solution of defining a new listener (e.g. 0.0.0.0:14020) on an adjacent port and routing to the corresponding cluster (127.0.0.1:15020) will work as an intermediate solution perhaps we can get |
can you open another ticket? |
Opened: #49987 |
Please provide a description of this PR:
Alternative for #47934
xref: #30987