feat: Add retry information headers #692

sichanyoo · 2024-04-05T21:01:56Z

Companion PR: awslabs/aws-sdk-swift#1432

Issue #

awslabs/aws-sdk-swift#1108

Description of changes

Adds retry information headers according to SEP: --/seps/accepted/shared/retry-header.md
Adds tests for verifying header contents in inputs.

Misc:

Organize test files and directories to mirror source structure

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…y structure.

…imeout & estimatedSkew).

…o middleware context.

…tring & one that calculates TTL by adding estimated skew and socket timeout to current time according to local machine clock.

…ed HTTP response's Date header value.

…rt to string without any adjustments.

…t existing RetryIntegrationTests to check retry information headers in inputs.

sichanyoo

Comments to help with PR reviews.

sichanyoo · 2024-04-05T22:51:21Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

@@ -16,23 +22,36 @@ public struct RetryMiddleware<Strategy: RetryStrategy,
    public var id: String { "Retry" }
    public var strategy: Strategy

+    // The UUID string used to uniquely identify an API call and all of its subsequent retries.
+    private let invocationID = UUID().uuidString.lowercased()


This invocation ID the unique string that identifies a single SDK operation invocation. In other words, the initial request & possible subsequent retried requests sent on behalf of it all share this unique ID.

sichanyoo · 2024-04-05T23:00:22Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

@@ -5,6 +5,12 @@
 // SPDX-License-Identifier: Apache-2.0


Make RetryMiddleware add amz-sdk-invocation-id header and amz-sdk-request header to the request.

amz-sdk-invocation-id: just a UUID value generated for the given operation invocation. Shared between origianl request and all subsequent retried requests.

amz-sdk-request: has three values. ttl=, attempt=, and max=. For the first, original rquest, TTL is not included. Starting from first retry request, TTL is calculated by adding socket timeout & estimated skew values to current time. Both socketTimeout and estimatedSkew are taken from context. socketTimeout is saved to context at context creation upstream, in operation method body. estimatedSkew is calculated then saved to context by DeserializeMiddleware when HTTP response is first received and deserialized.

sichanyoo · 2024-04-05T23:01:46Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

+    // The UUID string used to uniquely identify an API call and all of its subsequent retries.
+    private let invocationID = UUID().uuidString.lowercased()
+    // Max number of retries configured for retry strategy.
+    private var maxRetries: Int


This is the value used for max= value in amz-sdk-request header.

sichanyoo · 2024-04-05T23:02:26Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

        let partitionID = try getPartitionID(context: context, input: input)
        let token = try await strategy.acquireInitialRetryToken(tokenScope: partitionID)
-        return try await sendRequest(token: token, context: context, input: input, next: next)
+        input.headers.add(name: "amz-sdk-request", value: "attempt=1; max=\(maxRetries)")


Add amz-sdk-request header with TTL excluded for the first, original request.

sichanyoo · 2024-04-05T23:03:04Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

+            input.headers.update(
+                name: "amz-sdk-request",
+                value: "ttl=\(ttlDateUTCString); attempt=\(attemptNumber + 1); max=\(maxRetries)"
+            )


Update the amz-sdk-request header. Update attempt number & TTL.

sichanyoo · 2024-04-05T23:04:01Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

+// Calculates & returns TTL datetime in strftime format `YYYYmmddTHHMMSSZ`.
+func getTTL(now: Date, estimatedSkew: TimeInterval, socketTimeout: TimeInterval) -> String {
+    let dateFormatter = DateFormatter()
+    dateFormatter.dateFormat = "yyyyMMdd'T'HHmmss'Z'"
+    dateFormatter.locale = Locale(identifier: "en_US_POSIX")
+    dateFormatter.timeZone = TimeZone(abbreviation: "GMT")
+    let ttlDate = now.addingTimeInterval(estimatedSkew + socketTimeout)
+    return dateFormatter.string(from: ttlDate)
+}


Utility method for adding socketTimeout and estimatedSkew to given Date. Returns the sum as a string in strftime format.

sichanyoo · 2024-04-05T23:04:46Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

+// Calculates & returns estimated skew.
+func getEstimatedSkew(now: Date, responseDateString: String) -> TimeInterval {
+    let dateFormatter = DateFormatter()
+    dateFormatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss z"
+    dateFormatter.locale = Locale(identifier: "en_US_POSIX")
+    dateFormatter.timeZone = TimeZone(abbreviation: "GMT")
+    let responseDate: Date = dateFormatter.date(from: responseDateString) ?? now
+    // (Estimated skew) = (Date header from HTTP response) - (client's current time)).
+    return responseDate.timeIntervalSince(now)
+}


Utility method for subtracting given Date from responseDateString. Returns the difference in seconds (TimeInterval). This method is used in DeserializeMiddleware but placed here to group TTL related things in one place.

sichanyoo · 2024-04-05T23:06:57Z

Sources/ClientRuntime/Networking/Http/Middlewares/DeserializeMiddleware.swift

+            if let responseDateString = response.httpResponse.headers.value(for: "Date") {
+                let estimatedSkew = getEstimatedSkew(now: Date(), responseDateString: responseDateString)
+                context.attributes.set(key: AttributeKeys.estimatedSkew, value: estimatedSkew)
+            }


Calculate estimatedSkew by getting difference between server's timestamp on HTTP response returned via the Date header and current client time. Save to context for use in RetryMiddleware.

What happens when the response body takes a long time to transmit due to size or bad network? Does that time affect this number?

Bad network would increase estimated skew because network latency is included in the estimated skew.

As for response body taking a long time to transmit, that would increase estimated skew as well since that would delay when the response is deserialized which would subsequently delay when the estimated skew gets calculated.

jbelkins · 2024-04-11T15:16:26Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

@@ -45,7 +64,24 @@ public struct RetryMiddleware<Strategy: RetryStrategy,
                // TODO: log token error here
                throw operationError
            }
-            return try await sendRequest(token: token, context: context, input: input, next: next)
+            guard let estimatedSkew: TimeInterval = context.attributes.get(key: AttributeKeys.estimatedSkew) else {
+                throw ClientError.dataNotFound("Estimated skew not found; failed to calculate TTL for retry.")


Do we want to fail the entire request for lack of a parameter for this header?
Not sure what could cause this, but I don't think we want to fail customer requests for lack of this header.

Changed to logging .info level message with logger & using default values.

jbelkins · 2024-04-11T15:18:09Z

Sources/ClientRuntime/Middleware/RetryMiddleware.swift

+    let dateFormatter = DateFormatter()
+    dateFormatter.dateFormat = "yyyyMMdd'T'HHmmss'Z'"
+    dateFormatter.locale = Locale(identifier: "en_US_POSIX")
+    dateFormatter.timeZone = TimeZone(abbreviation: "GMT")


nit: use TZ identifier "UTC" for server communications

Changed to UTC.

jbelkins · 2024-04-11T15:19:25Z

Sources/ClientRuntime/Networking/Http/HttpClientConfiguration.swift

@@ -45,7 +45,7 @@ public class HttpClientConfiguration {
    ///   - protocolType: The HTTP scheme (`http` or `https`) to be used for API requests.  Defaults to the operation's standard configuration.
    public init(
        connectTimeout: TimeInterval? = nil,
-        socketTimeout: TimeInterval? = nil,
+        socketTimeout: TimeInterval? = 60.0,


If we are providing a non-nil default, should the param be non-optional?

Changed as suggested.

jbelkins · 2024-04-11T15:22:18Z

Sources/ClientRuntime/Networking/Http/Middlewares/DeserializeMiddleware.swift

+            if let responseDateString = response.httpResponse.headers.value(for: "Date") {
+                let estimatedSkew = getEstimatedSkew(now: Date(), responseDateString: responseDateString)
+                context.attributes.set(key: AttributeKeys.estimatedSkew, value: estimatedSkew)
+            }


What happens when the response body takes a long time to transmit due to size or bad network? Does that time affect this number?

jbelkins · 2024-04-11T15:23:11Z

Tests/ClientRuntimeTests/Retry/RetryIntegrationTests.swift

+        let responseDateStringPlusTen = "Mon, 15 Jul 2024 01:24:22 GMT"
+        let estimatedSkew = getEstimatedSkew(now: responseDate, responseDateString: responseDateStringPlusTen)
+
+        XCTAssert(estimatedSkew == 10.0)


nit: Use XCTAssertEqual to compare two values in a test assertion

Changed as suggested.

…hrowing an error.

SmithyRetriesTests test target was accidentally removed from Package.swift in smithy-lang#692, so this commit adds it back. The test was also not working properly, because Context.partitionID's attribute key name used by RetryMiddleware was different from the one being set in the test. This caused the test to exit early when partitionID wasn't found.

Sichan Yoo added 10 commits April 5, 2024 09:15

Organize test files and directories to mirror code files and director…

38a4614

…y structure.

Add withSocket utility method & AttributeKeys related to TTL (socketT…

3a8240e

…imeout & estimatedSkew).

Make socketTimeout to default to 60 seconds.

f7c37bf

Add codegen that saves socketTimeout from HttpClientConfiguration int…

bd13c10

…o middleware context.

Add utility functions; one that calculates estimated skew from date s…

65a3c97

…tring & one that calculates TTL by adding estimated skew and socket timeout to current time according to local machine clock.

Make DeserializeMiddleware save estimated skew calculated from return…

32fcae8

…ed HTTP response's Date header value.

Make RetryMiddleware add retry information headers as defined in SEP.

7cd8aee

Fix dateFormatter in getTTL utility method to take raw date and conve…

5c71413

…rt to string without any adjustments.

Add tests for the 2 utility methods getTTL & getEstimatedSkew. Augmen…

d8325cc

…t existing RetryIntegrationTests to check retry information headers in inputs.

Update codegen test to include socketTimeout addition.

efecdb9

sichanyoo mentioned this pull request Apr 5, 2024

feat: Add retry information headers awslabs/aws-sdk-swift#1432

Merged

sichanyoo commented Apr 5, 2024

View reviewed changes

Add dummy values needed for context used by retry middleware tests.

5b111cc

jbelkins requested changes Apr 11, 2024

View reviewed changes

Sichan Yoo added 4 commits April 11, 2024 11:33

Change a couple XCTAssert to XCTAssertEqual for better log message.

54a86da

Make socketTimeout non-optional given default value is being set now.

c46c143

Log .info level message then proceed with default values instead of t…

93cf091

…hrowing an error.

Merge branch 'main' into feat/retry-information-headers

e345b32

sichanyoo requested a review from jbelkins April 11, 2024 18:38

Sichan Yoo added 2 commits April 11, 2024 11:51

Fix socket timeout related errors.

7dbc435

Fix syntax error.

db853ef

jbelkins approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'main' into feat/retry-information-headers

2289812

sichanyoo merged commit 7be2bd8 into main Apr 16, 2024
12 checks passed

sichanyoo deleted the feat/retry-information-headers branch April 16, 2024 17:22

milesziemer mentioned this pull request Jul 12, 2024

fix: run retry integration tests #780

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add retry information headers #692

feat: Add retry information headers #692

sichanyoo commented Apr 5, 2024 •

edited

Loading

sichanyoo left a comment

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

sichanyoo Apr 5, 2024

jbelkins Apr 11, 2024

sichanyoo Apr 11, 2024

jbelkins Apr 11, 2024

sichanyoo Apr 11, 2024

jbelkins Apr 11, 2024

sichanyoo Apr 11, 2024

jbelkins Apr 11, 2024

sichanyoo Apr 11, 2024

jbelkins Apr 11, 2024

jbelkins Apr 11, 2024

sichanyoo Apr 11, 2024

feat: Add retry information headers #692

feat: Add retry information headers #692

Conversation

sichanyoo commented Apr 5, 2024 • edited Loading

Issue #

Description of changes

Scope

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sichanyoo commented Apr 5, 2024 •

edited

Loading