Fix ApiCallDuration so that it measures the whole API call - #7338
Open
alextwoods wants to merge 4 commits into
Open
Fix ApiCallDuration so that it measures the whole API call#7338alextwoods wants to merge 4 commits into
ApiCallDuration so that it measures the whole API call#7338alextwoods wants to merge 4 commits into
Conversation
Contributor
|
With this change, the ApiCallDuration could be longer than configured apiCallTimeout since marshalling and endpoint resolution stuff does not count into the timeout. This could be confusing and worth mentioning that in somewhere, probably API_CALL_DURATION javadoc. |
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
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.
Fix
ApiCallDurationso it measures the whole API callMotivation and Context
CoreMetric.API_CALL_DURATIONunderstated latency, and did so differently on the sync and async clients.The measurement lived in a request pipeline stage. The pipeline's input is the already-marshalled request, so the window
opened after marshalling had finished — contradicting the formula in the metric's own javadoc, which lists a
MARSHALLING_DURATIONterm.The async client was worse.
AsyncApiCallMetricCollectionStagewas nested one builder deeper than its sync counterpart,so the window also excluded the entire request-mutation chain: endpoint resolution, auth scheme resolution, request compression, checksums, header and query merging and user agent.
The two clients also disagreed on the
afterExecutioninterceptors: inside the window on async, outside on sync.Modifications
ApiCallDurationis moved out of the pipeline entirely, up into the client handlers alongsideAPI_CALL_SUCCESSFUL— which was already scoped to the whole call, so the two metrics had been reporting on different windows from the same collector. No pipeline arrangement can enclose marshalling, so this was the only available position.afterExecutioninterceptors are now inside the window on both clients: an interceptor running as part of the call is part of the call.SERVICE_ENDPOINTcollection moved into the handlers'doExecute, the first point after the pipeline that still holds theExecutionContext.NoOpMetricCollector, the default when no publisher is configured, so the no-metrics path does not regress.Testing
New
ApiCallDurationWindowTest(test/codegen-generated-classes-test) injects a 300 ms delay into one phase at a time and asserts it appears inApiCallDuration, for both clients.Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License
One internal-API behaviour change: code driving
AmazonSyncHttpClientorAmazonAsyncHttpClientdirectly, bypassing theclient handlers, no longer receives
ApiCallDurationorSERVICE_ENDPOINT.Testing
New
ApiCallDurationWindowTest(test/codegen-generated-classes-test) injects a 300 ms delay into one phase at a timeand asserts it appears in
ApiCallDuration, for both clients. This is deliberately stronger than asserting theadditivity formula over real timings: the phases at issue cost microseconds against a millisecond-scale call, so an
inequality passes whether or not they are counted — an additivity-only check would not have caught the sync defect.
Verified against the unpatched SDK: 5 of the 7 cases fail, and the 2 that pass are exactly the two that were already
correct (sync endpoint resolution, async
afterExecution).Also added:
ApiCallDurationAssertions, applied fromCoreMetricsTestandBaseAsyncCoreMetricsTest, assertingApiCallDurationencloses each component metric on success, error and retry paths.