Updating IngesterClient MaxCallRecvMsgSize to be configurable vs fixed size #712
Conversation
…he ingester client.
…ter client config
khaines
changed the title
pkg/distributor/distributor.go
Outdated
| } | ||
|
|
||
| client, err := d.cfg.ingesterClientFactory(ingester.Addr, d.cfg.CompressToIngester) | ||
| client, err := d.cfg.ingesterClientFactory(ingester.Addr, d.cfg.CompressToIngester, d.cfg.IngesterClientConfig) |
There was a problem hiding this comment.
For bonus points you could move the CompressToIngester flags into the IngesterClientConfig.
There was a problem hiding this comment.
Agreed. I will make the adjustment.
|
Other than the minor nit, LGTM. |
|
Any insight into what drives these massive data sizes? Could we do some summarization before sending? |
pkg/ingester/client/client.go
Outdated
| // We have seen 20MB returns from queries - add a bit of headroom | ||
| f.IntVar(&cfg.MaxRecvMsgSize, "ingester.client.max-recv-message-size", 64*1024*1024, "Maximum message size, in bytes, this client will receive.") | ||
| // moved from distributor pkg, but flag prefix left as-is so existing users do not break. | ||
| flag.BoolVar(&cfg.CompressToIngester, "distributor.compress-to-ingester", false, "Compress data in calls to ingesters.") |
There was a problem hiding this comment.
Rather than the confusing name, I think I'd rather have two flags and copy from one to the other to allow backwards-compatibility.
There was a problem hiding this comment.
Certainly. I've added this in the latest updates to the PR.
|
BTW, thanks for the contribution! Great to see a broader community. |
…ingester.client namespace, but also leaving a fallback flag under distributor for back compat
|
I'm glad to be contributing :). This will be the first of many for this excellent platform |
|
As for insight to what is causing the large data size returns, at least in our case it appears to be purely based on the number of timeseries found during the index lookup phase of querying. We have some fun & extreme cases where there are tens of thousands of timeseries matches to a query. This is even during a recording rule job cycle, so it is over a very small time window. I haven't looked too deeply at that api or flow yet, but I in principle agree with a comment Tom made on slack that perhaps a future revision here would be to make it streaming as all the results have to come back in 1 giant message blob currently. |
|
Couple of lint and test failures - looks like you need to fix up the tests to match changes in the code. |
|
Thanks for catching that @bboreham. Our current build system here wasn't running those tests (gasp), so they got missed but thought to be passing. Corrections have been made |
3 participants
We have some data scenarios where the amount of data exceeds the 64MB fixed size that the grpc client was set to for a max message size (added in #660). This update changes that to a configurable command arg flag, with the default being set to 64MB.