distributor: Report correct size in "message too big" error #12799
Conversation
| } else if e.compressed > 0 { | ||
| msgSizeDesc = fmt.Sprintf(" of %d bytes (compressed)", e.compressed) | ||
| } | ||
| return globalerror.DistributorMaxOTLPRequestSize.MessageWithPerInstanceLimitConfig(fmt.Sprintf("the incoming OTLP request has been rejected because its message size%s is larger than the allowed limit of %d bytes", msgSizeDesc, e.limit), maxOTLPRequestSizeFlag) |
There was a problem hiding this comment.
Is it possible to not have either the compressed or actual size?
There was a problem hiding this comment.
Yes, because we only decompress requests until reaching the limit, so we don't exactly know by how much a compressed request is over the limit after decompression.
I added a specific constructor for this case at 8250a2f.
| responseContentLength: 292, | ||
| errMessage: "the incoming OTLP request has been rejected because its message size of 89 bytes is larger", | ||
| expectedLogs: []string{`level=warn user=test msg="detected an error while ingesting OTLP metrics request (the request may have been partially ingested)" httpCode=413 err="rpc error: code = Code(413) desc = the incoming OTLP request has been rejected because its message size of 89 bytes is larger than the allowed limit of 30 bytes (err-mimir-distributor-max-otlp-request-size). To adjust the related limit, configure -distributor.max-otlp-request-size, or contact your service administrator." insight=true`}, | ||
| responseContentLength: 307, |
There was a problem hiding this comment.
I see that this isn't new but this seems very fragile. Where does this number come from? Can we verify it some other way?
pkg/distributor/otel_test.go
Outdated
| expectedLogs: []string{`level=warn user=test msg="detected an error while ingesting OTLP metrics request (the request may have been partially ingested)" httpCode=413 err="rpc error: code = Code(413) desc = the incoming OTLP request has been rejected because its message size of 106 bytes is larger than the allowed limit of 30 bytes (err-mimir-distributor-max-otlp-request-size). To adjust the related limit, configure -distributor.max-otlp-request-size, or contact your service administrator." insight=true`}, | ||
| responseContentLength: 308, | ||
| errMessage: "the incoming OTLP request has been rejected because its message size of 106 bytes (decompressed) is larger", | ||
| expectedLogs: []string{`level=warn user=test msg="detected an error while ingesting OTLP metrics request (the request may have been partially ingested)" httpCode=413 err="rpc error: code = Code(413) desc = the incoming OTLP request has been rejected because its message size of 106 bytes (decompressed) is larger than the allowed limit of 30 bytes (err-mimir-distributor-max-otlp-request-size). To adjust the related limit, configure -distributor.max-otlp-request-size, or contact your service administrator." insight=true`}, |
There was a problem hiding this comment.
Personal preference but I really don't like testing for extremely specific log messages like this. It makes the tests fragile and is testing for implementation specific side-effects instead of the response for the method on question.
There was a problem hiding this comment.
I had the same thought (also about #12799 (comment)) while updating those tests, but I wanted to keep the PR focused.
I'll open a separate PR relaxing those tests.
What this PR does
In the Prometheus push endpoint, when the write request after compression exceeds the limit, the HTTP request's content length (this before compression) would be reported, so you could see an error like this:
With this change:
(compressed)or(uncompressed)to make it always clear which one it's talking about.Which issue(s) this PR fixes or relates to
—
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.