Repository navigation
Separate successful-response TTFB from HTTP error latency - #3307
abha-mutalik merged 4 commits into
Conversation
Track completed 2xx and 304 separately from delivered 4xx and 5xx, with sample-matched rates and delivery diagnostics. Preserve legacy behavior and cover inline, delayed, interim, and failed response paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3307 +/- ##
=============================================
- Coverage 64.24% 50.68% -13.57%
+ Complexity 10398 8711 -1687
=============================================
Files 840 939 +99
Lines 71755 80922 +9167
Branches 8611 9777 +1166
=============================================
- Hits 46099 41013 -5086
- Misses 23004 36483 +13479
- Partials 2652 3426 +774 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Keep 2xx and 304 separate from 4xx and 5xx without testing delivery completeness. Remove the new delivery-state instrumentation and update regressions for incomplete bodies, aborts and subsequent internal errors. Existing error metrics remain unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove four redundant delivery-focused tests and combine zero, invalid, and missing-start timing cases. Preserve seven core TTFB tests, existing test extensions, and all production behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep this change focused on status-specific latency and its missing-timing diagnostic. Remove ClientErrorCount registration, recording and related assertions while preserving existing error metrics and the seven core TTFB tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jitheshtr
left a comment
jitheshtr
left a comment
There was a problem hiding this comment.
Approved. The final-header-only classification and capture before write submission preserve the intended semantics while keeping legacy error accounting intact. Three non-blocking suggestions inline.
Generated with GitHub Copilot CLI
| assertEquals(1, metrics.nioTimeToFirstByteInMs.getCount()); | ||
| assertEquals(0, metrics.nioTimeToFirstByteInMs.getSnapshot().getMax()); | ||
| if (valid) { | ||
| assertArrayEquals(new long[]{0}, metrics.successfulResponseNioTimeToFirstByteInMs.getSnapshot().getValues()); |
There was a problem hiding this comment.
Non-blocking: could we also assert a nonzero measured TTFB value and its millisecond units? The implementation is correct, but in a focused mutation run all 11 tracker tests still passed both when the nanoseconds-to-milliseconds conversion was removed and when every valid duration was replaced with zero. Using the existing reflection pattern to install a known past monotonic start, then checking lower and upper millisecond bounds, would reject both regressions without sleeps or a new production clock abstraction.
| } | ||
|
|
||
| @Test | ||
| public void testResponseTtfbUsesFinalHeadersAndPreservesErrorCounts() { |
There was a problem hiding this comment.
Non-blocking CI follow-up: the current workflow compiles/packages the API tests through dependencies but never invokes :ambry-api:test. I checked the complete logs for all ten executed jobs at this head: these new tracker tests do not execute, although the four new REST tests do. Could we add ambry-api to an appropriate existing unit-test job's module list? This is a pre-existing workflow omission, so a small follow-up would also be reasonable.
| * size sample even though the interim response channel has already completed. | ||
| */ | ||
| @Test | ||
| public void testResponseTtfbIgnoresDelayedContinue() throws Exception { |
There was a problem hiding this comment.
Non-blocking nit: the Javadoc immediately above describes failed 100-Continue client-abort accounting, which belongs to testContinueResponseWriteFailureRecordsClientAbortOnce, not this delayed-Continue TTFB test. Please move that block back above its original method.
Summary
Separate TTFB by final response-header status only: 2xx/304 (including 206/HEAD), 4xx, and 5xx, with sample-matched rates and a missing-timing diagnostic.
A submitted 200 remains in the 2xx/304 group even if its body is incomplete or later fails. These are status-latency metrics, not successful-download or availability metrics. Preserve legacy error/abort metrics, HTTP behavior, resource handling, and blob durability. No live-alert changes.
Testing Done
./gradlew --no-build-cache: targeted:ambry-api:test(15),:ambry-rest:test(66), and:ambry-frontend:test(60) passed; no retries/skips.Generated with GitHub Copilot CLI.