Record sizes for client-terminated whole-blob uploads

[jitheshtr]

Summary

Client-aborted whole-object uploads need truthful size telemetry. The existing router log reports a fixed chunk size, which cannot show whether large objects abort more often.

What changed

1. Review focus: centralized winning client-termination recording across channel-inactive and response-write paths, while server-initiated and shutdown closes remain excluded.
2. Reused the shared upload-routing predicate and one dataset-copy predicate for lazy classification after an idle or remote-client termination outcome wins.
3. Added ClientTerminatedWholeBlobRequestBytesReceived and ClientTerminatedDeclaredWholeBlobSizeInBytes; chunks, S3 multipart parts, control requests, named stitches, dataset copies, invalid routes, and idle timeouts are excluded.
4. Extended the existing abort log lines with request bytes, declared size, and classification without adding another log line.
5. Multipart-form aborts retain received request bytes, including MIME framing overhead, but envelope Content-Length is not reported as a declared blob size unless x-ambry-blob-size supplies 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. Idle timeouts remain outside the size histograms but retain size details in the existing idle log line.

Risk Assessment

The classifier runs only after termination, is read-only, and defaults to OTHER on failure, dropping a metric sample without changing request handling. No storage, write-path, response-completion, or resource-lifecycle behavior changes. Durability risk: none.

Testing Done

• Local code review completed

Command	Result
./gradlew :ambry-api:test :ambry-rest:test :ambry-frontend:test	526 passed, 6 skipped
./gradlew :ambry-api:javadoc :ambry-rest:javadoc :ambry-frontend:javadoc	Passed; pre-existing warnings only

---

Tests generated with unit-tests plugin

🤖 Generated with GitHub Copilot CLI

[codecov-commenter]

Codecov Report

❌ Patch coverage is 17.89474% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.71%. Comparing base (52ba813) to head (3d07367).
⚠️ Report is 417 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/com/github/ambry/rest/NettyMessageProcessor.java	10.86%	40 Missing and 1 partial ⚠️
...hub/ambry/frontend/FrontendRestRequestService.java	0.00%	25 Missing and 2 partials ⚠️
...va/com/github/ambry/rest/NettyResponseChannel.java	22.22%	6 Missing and 1 partial ⚠️
...github/ambry/rest/AsyncRequestResponseHandler.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3294       +/-   ##
=============================================
- Coverage     64.24%   50.71%   -13.54%     
+ Complexity    10398     8708     -1690     
=============================================
  Files           840      939       +99     
  Lines         71755    80732     +8977     
  Branches       8611     9740     +1129     
=============================================
- Hits          46099    40940     -5159     
- Misses        23004    36394    +13390     
- Partials       2652     3398      +746     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[beijxu]

Suggestion: defer classification to the termination path so it doesn't run per upload

This runs classifyRequestSize(request) eagerly for every POST/PUT, doing a full RequestPath.parse on the event loop — and routing parses the path again later, so uploads parse twice. But the result is only ever consumed on the termination path: requestSizeCategory is read in exactly three places — the channelInactive abort log, the userEventTriggered idle log, and the size recording in recordTermination. Nothing on the success path reads it.

The inputs are deterministic from immutable request fields (getRestMethod()/getPath()/getArgs(), all set at construction from client headers/query params, never mutated during handling), and the request object is still fully intact at both termination sites (they already read getUri()/getBytesReceived()/getSize() off it). So the parse can move off the hot path entirely — compute it lazily once inside recordTermination, under the existing terminationRecorded guard:

private long recordTermination(Histogram histogram, boolean recordClientTerminationSizes) {
  long timeInFlightMs = request.getMetricsTracker().getTimeSinceRequestReceivedInMs();
  if (!terminationRecorded) {
    terminationRecorded = true;
    requestSizeCategory = classifyRequestSize(request);   // was in handleRequest
    histogram.update(timeInFlightMs);
    if (recordClientTerminationSizes && requestSizeCategory == RequestSizeCategory.WHOLE_BLOB) {
      ...
    }
  }
  return timeInFlightMs;
}

Both termination sites call recordTermination(...) before the log line that reads requestSizeCategory, so ordering still works and both log lines keep printing the category. The only change is it now runs solely on the rare termination path instead of once per upload.

Two caveats:

1. testAbortedUploadRecordsSizes deliberately flips the mock classifier to OTHER after headers to prove capture-at-header-time; it would need updating to reflect the new capture point. For the real FrontendRestRequestService the result is identical since classification is deterministic.
2. Eager capture snapshots the classification while the request is provably pristine. Deferring assumes no future classifier becomes sensitive to args added during handling — the current one only reads client-supplied headers/query params, so it's safe today. Worth a one-line comment noting the assumption.

Trade-off clearly favors deferring: a twice-per-upload parse moves off the event-loop hot path to a rare path, at the cost of one test edit.

[jitheshtr]

Good point—addressed in ec56906. Classification now runs lazily inside the existing one-shot termination guard, so active and successful uploads avoid the extra path parse. The classifier contract requires stable pre-handling metadata, and the test now asserts zero classifications while the upload is active and exactly one after termination. The full ambry-api, ambry-rest, and ambry-frontend matrix passed: 502 tests passed, 6 skipped; all three Javadocs passed.