Record sizes for client-terminated whole-blob uploads - #3294
Open
jitheshtr wants to merge 4 commits into
Open
Conversation
When a client abandons an upload, nothing records how large the blob was meant to be, so it is not possible to test whether aborts concentrate on large uploads. The size that does reach the logs today is misleading: PutChunk builds its BlobProperties from chunkBlobSize, and BlobProperties.toString() prints that field as BlobSize, so the PutOperation$PutChunk line always reports the fixed 4 MiB chunk size rather than anything about the blob. The router cannot supply the real size on this path. PutOperation.getBlobSize() throws unless chunk filling completed successfully, and blobSize stays -1 until then, which by definition never happens on an abort. Renaming the field in the shared BlobProperties.toString() was rejected because it would change output everywhere for a diagnostic needed in one place. Record it in NettyMessageProcessor instead, where both useful quantities are already available when the abort is detected: the size the client declared, via the x-ambry-blob-size header or Content-Length, and the bytes that actually arrived. Together they show both whether large uploads are over-represented and how far clients got before giving up. Both go on the existing abort log lines next to the time in flight, so a single line answers the question without a join, and into two histograms fed from the one shot helper that already guards against channelInactive() and the idle timeout double counting the same abort. Use getBytesReceived() rather than getBlobBytesReceived(). NettyMultipartRequest sets its blob bytes only in processPart(), reached from prepare() once the whole request has been buffered, so an aborted multipart upload has no decoded blob part and reports zero. NettyMessageProcessor does construct that subclass, so the obvious getter would have silently recorded zeros for a whole class of uploads. testAbortedMultipartUploadRecordsBytesReceived pins this down and fails if the getter is swapped back. Record sizes only for POST and PUT, the methods that carry a body, since an aborted GET always contributes zero bytes and would swamp the distribution. Record a declared size only when the client gave one, so the -1 that represents a chunked upload with no declared size never enters the histogram; the log line renders that case as "unknown" instead, keeping one format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Describe the size histograms as following G1’s client-termination bucket rather than claiming every channelInactive event is proven client-caused. No behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
POST and PUT alone do not identify whole-object uploads: they also carry chunks, S3 multipart parts, stitches, dataset copies, and control requests. Mixing those request sizes makes the abort histograms misleading. Add a read-only classifier at the frontend routing boundary, delegate it through AsyncRequestResponseHandler, and capture the category in NettyMessageProcessor before the request enters the async queue. Only WHOLE_BLOB client terminations update the renamed histograms; idle timeouts remain excluded. For multipart/form-data, keep request bytes received but record a declared blob size only when x-ambry-blob-size is explicit, because Content-Length describes the MIME envelope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject malformed S3 PUT paths before they can be labeled as whole-object uploads. Strengthen tests so multipart envelope filtering, read-only classification, and invalid classifier fallbacks fail if regressed. Remove the redundant bodyless-request test now covered by the route and category tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3294 +/- ##
=============================================
- Coverage 64.24% 50.79% -13.46%
+ Complexity 10398 8717 -1681
=============================================
Files 840 939 +99
Lines 71755 80655 +8900
Branches 8611 9726 +1115
=============================================
- Hits 46099 40966 -5133
- Misses 23004 36286 +13282
- Partials 2652 3403 +751 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jitheshtr
marked this pull request as ready for review
August 22, 2026 00:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CASPIAN-5148 needs truthful size telemetry for client-aborted whole-object uploads. The existing router log reports a fixed chunk size, which cannot show whether large objects abort more often.
What changed
ClientTerminatedWholeBlobRequestBytesReceivedandClientTerminatedDeclaredWholeBlobSizeInBytes; chunks, S3 multipart parts, control requests, named stitches, dataset copies, invalid routes, and idle timeouts are excluded.Content-Lengthis not reported as a declared blob size unlessx-ambry-blob-sizesupplies the blob total.Suggested reading order: classifier contract, frontend classification, async delegation, then Netty metric recording; the remaining diff is metrics registration and tests.
Known gaps
Chunked and S3 multipart uploads are excluded because the final whole-object size is unavailable at this layer. The client-termination bucket can still include a server-initiated channel close, and disconnects first detected during response writes bypass it. Idle timeouts remain outside the size histograms but retain size details in the existing idle log line.
Risk Assessment
The classifier is read-only and defaults to
OTHERon failure, dropping a metric sample without changing request handling. No storage, write-path, callback, response-completion, or resource-lifecycle behavior changes. Durability risk: none.Testing Done
./gradlew :ambry-api:test :ambry-rest:test :ambry-frontend:test./gradlew :ambry-api:javadoc :ambry-rest:javadoc :ambry-frontend:javadocBUG=CASPIAN-5148
Tests generated with unit-tests plugin
🤖 Generated with GitHub Copilot CLI