chore: make grpc compression configurable for store gateway and alertmanager client

[yeya24]

Signed-off-by: Ben Ye [email protected]

What this PR does:

Enable snappy compression for store gateway and alertmanager gRPC client.
Now I just make it a default, but we can go with flag, too.

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[alanprot]

I think this should be a config as in the other clients , no?

[yeya24]

// We prefer sane defaults instead of exposing further config options.

This seems like an old comment. I am okay with exposing this flag. @alvinlin123 @alanprot WDYT?

[alanprot]

I think all the other clients expose this as a config..

It weird just this one hard code to snappy. I know in theory snappy should not use lots of resources but who knows...

[yeya24]

@alanprot If I am going to expose the compressor as a flag then I might need to expose other fields like max send receive size, etc. If I am going to use https://github.com/cortexproject/cortex/blob/master/pkg/util/grpcclient/grpcclient.go#L40, then seems the default value of grpc-max-recv-msg-size and grpc-max-send-msg-size for alertmanager client got changed. Are we okay with this? But for the store gateway client, that remain the same.

[yeya24]

I updated the code and exposed all grpc configurations as flags.
This also introduces a compatiblity issue for the flags.
The store gateway TLS enabled flag changes from querier.store-gateway-client.tls_enabled to querier.store-gateway-client.grpc-client-config.tls_enabled.
The AM client TLS enabled flag changes from alertmanager.alertmanager-client.tls_enabled to alertmanager.alertmanager-client.grpc-client-config.tls_enabled.

[friedrichg]

the TLS configuration is not part of the guarantees.
https://cortexmetrics.io/docs/configuration/v1guarantees/
We can change them as needed.

This looks good but needs a CHANGELOG entry and running make doc.

[alanprot]

@yeya24 Thanks...

Can you fix the lint?

[yeya24]

PR updated. PTAL again @alanprot @friedrichg

[alanprot]

Thanks!!

I have 2 comments though.

• Can we avoid this change to be a breaking change? Otherwise people will need to change their config when updating cortex.
• Do we need all those extra configs? If for now we just wanna make the compression configurable can we add just that field? We are trying to make the cortex configuration less confusing and i guess adding lots of configs that customer would never use it not going in that direction.

WDYT?

[yeya24]

@alanprot @friedrichg
Based on Alan's suggestion, I expose the flags for grpc compression only for now. PTAL