Fix requestsTooLarge metric reporting when decompression buffer overflows on armeria in otel logs source#6633
Conversation
graytaylor0
requested review from
KarstenSchnitter,
dinujoh,
dlvenable,
engechas,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
…lows on armeria in otel logs source Signed-off-by: Taylor Gray <tylgry@amazon.com>
graytaylor0
force-pushed
the
MetricFix413
branch
from
a06e899 to
bdcc442
Compare
KarstenSchnitter
left a comment
KarstenSchnitter
left a comment
There was a problem hiding this comment.
Thanks for providing this change. I saw a test earlier. Is it possible to test this addition?
| final HttpStatus expectedStatus, | ||
| final Throwable throwable) { | ||
| assertThat("Http Status", response.status(), equalTo(expectedStatus)); | ||
| assertThat("Http Status", response.status(), equalTo(expectedStatus)); |
There was a problem hiding this comment.
Unecessary whitespace.
| assertThat("Http Status", response.status(), equalTo(expectedStatus)); | |
| assertThat("Http Status", response.status(), equalTo(expectedStatus)); |
There was a problem hiding this comment.
@KarstenSchnitter The test I had did not actually cover the new code. I have not been able to find a good way to test this code path with log access writer. I think there needs to be some actual integration tests that don't mock the OtelLogsSource server to cover this. If you see a way to add a unit test for this let me know.
There was a problem hiding this comment.
Unfortunately, I do not see an easy possibility. Furthermore, it would be more of a test of armeria.
Signed-off-by: Taylor Gray <tylgry@amazon.com>
| serverBuilder.http(oTelLogsSourceConfig.getPort()); | ||
| } | ||
|
|
||
| serverBuilder.accessLogWriter(log -> { |
There was a problem hiding this comment.
Are you sure this catches all cases?
When buffer.writeBytes(request.toByteArray(), null, bufferWriteTimeoutInMillis); throws an exception, we catch and throw new BufferWriteException(e.getMessage(), e);
There was a problem hiding this comment.
Isn't this exception already handled? This would indicate to me, that this PR closes a hole in the existing error handling.
There was a problem hiding this comment.
Yes this should already be handled. The problem was that armeria was swallowing the exception for decompression overflowing the max request size, and not running the exception handler code that increments this metric.
|
@graytaylor0 we should probably add the siimilar metric for OTEL traces and metrics as well, right? |
I agree. In my experience the issue is most likely with metrics.Note, that this will create a conflict with #6604. |
Yes we should. I will follow up with a similar change in those sources. |
Yes I will make a similar change in the other sources as a follow up. |
4 participants
Description
This change handles an edge case where the otel logs source does not report the "requestsTooLarge" metric despite throwing a 413 exception.
The edge case happens when the source is receiving compressed requests. If the decompressed request exceeds the max_request_length, the decompression buffer in armeria throws a 413 error, but it does not increment the metric "requestsTooLarge" because the exception handler does not run in this scenario.
Issues Resolved
Related to #5594
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.