output/cloudv2: Unlimited size for body payload

[codebien]

We have decided to drop the check for the body's payload. The remote service will take care of splitting the chunk more if it turns out they are too big to be handled.

[codecov-commenter]

Codecov Report

Merging #3120 (b76709b) into master (2e5631f) will decrease coverage by 0.03%.
The diff coverage is 83.92%.

❗ Current head b76709b differs from pull request most recent head bcd5532. Consider uploading reports for the commit bcd5532 to get more accurate results

@@            Coverage Diff             @@
##           master    #3120      +/-   ##
==========================================
- Coverage   73.86%   73.84%   -0.03%     
==========================================
  Files         243      243              
  Lines       18492    18502      +10     
==========================================
+ Hits        13659    13662       +3     
- Misses       3964     3969       +5     
- Partials      869      871       +2     

Flag	Coverage Δ
ubuntu	73.76% <83.92%> (-0.02%)	⬇️
windows	73.68% <83.92%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
output/cloud/output.go	74.14% <0.00%> (ø)
output/cloud/expv2/metrics_client.go	60.00% <75.00%> (+4.44%)	⬆️
output/cloud/expv2/output.go	85.21% <89.74%> (-1.63%)	⬇️
cloudapi/client.go	72.44% <100.00%> (-1.03%)	⬇️
output/cloud/expv2/flush.go	84.61% <100.00%> (ø)

... and 1 file with indirect coverage changes

[codebien]

We discussed it internally and decided to drop the check on the client side.

[imiric]

LGTM, but out of curiosity, why drop the check altogether instead of setting a reasonable limit?

There must be some limit in the backend for payload size. Couldn't this be exceeded in some cases? I suppose the backend would return HTTP 413, and the request would fail anyway. 🤔

[codebien]

Couldn't this be exceeded in some cases?

@imiric It could but it would be something deterministic that we could fix. For example, if we have a lot of aggregates per time series and we set on the backend a value that needs to be higher to support it. But it means we detect it, adjust the config, the limit and the chunk limit then it should not happen again because we are aggregating using a finite variable (the time - 3s for the aggregation period and 8s for the waiting period).

I suppose the backend would return HTTP 413, and the request would fail anyway

Yeah, it is the main reason for dropping it, we are delegating the control over the backend so we can tweak the limit without a direct dependency here that would increase the complexity.

I expect we and the backend will monitor our systems and we adjust things in case the metrics don't sound correct.

[olegbespalov]

Hey @codebien. Are there any blockers to merging this, or when we understand if we're going to merge this?

[codebien]

Hey @olegbespalov, the release was the blocker, it's now ready to be merged.