-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement request ID access for SDK clients RFC #2129
Merged
Merged
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
60afad8
Add `RequestId` trait
jdisanti c761516
Implement `RequestId` for generated AWS client errors
jdisanti 1250e65
Move `RustWriter.implBlock` out of `StructureGenerator`
jdisanti ab14c6e
Create structure/builder customization hooks
jdisanti 1ce44f8
Customize `_request_id` into AWS outputs
jdisanti b133cc9
Set request ID on outputs
jdisanti d1468c5
Refactor SDK service decorators
jdisanti d3d201d
Refactor S3's extended request ID implementation
jdisanti bd9d166
Update changelog
jdisanti 3c21d82
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti fa82661
Fix `codegen-core` tests
jdisanti 8c09f45
Combine `Error` and `ErrorKind`
jdisanti 70547bf
Add test for service error conversion
jdisanti 65367e0
Fix request ID impl for service error
jdisanti 7eb87a7
Fix event stream marshall/unmarshall generators
jdisanti e9eee00
Fix `codegen-core` tests
jdisanti d3c9c80
Refactor error generation to fix server tests
jdisanti b6d1ebb
Move error generators into `codegen-client` and fix tests
jdisanti a1a426b
Re-export `ErrorMetadata`
jdisanti daf7a38
Update changelog
jdisanti 6438cc8
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti ef0614f
Incorporate feedback
jdisanti 5a7f1c1
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 2338074
Add request IDs to trace logs
jdisanti cbff721
Fix doc link
jdisanti b038943
Fix the canary build
jdisanti f11225f
Simplify some error trait handling
jdisanti 4ca4a92
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 87e0fef
Fix tracing statement
jdisanti b4680d2
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 6578643
Rename `ClientContextParamDecorator` to `ClientContextConfigCustomiza…
jdisanti f41fbae
Fix S3Control endpoint tests
jdisanti af52cd3
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 69f367d
temp: Check if Gradle caching is causing the server codegen issue
jdisanti 9e345fe
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 164b2f0
Revert "temp: Check if Gradle caching is causing the server codegen i…
jdisanti e667dda
Fix Python server errors
jdisanti 1bce442
Add deprecated alias to guide customers through upgrading
jdisanti 63ca8b0
Rename `_meta` to `meta`
jdisanti 44c1dd0
Rename the `ErrorMetadata` trait to `ProvideErrorMetadata`
jdisanti fe1ec5a
Rename `aws_smithy_types::Error` to `ErrorMetadata`
jdisanti 9a5b47b
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti afd7dde
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 13297be
Fix doc link
jdisanti 44ab428
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti dd7c7ee
Minor fixes
jdisanti fbf08d7
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 4d92c09
Fix test compile
jdisanti 1805cee
Fix another doc link
jdisanti ea72017
Incorporate feedback
jdisanti 41dff69
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti f5f31e5
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti 9d74fcf
Merge remote-tracking branch 'origin/main' into jdisanti-request-id-a…
jdisanti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,167 @@ message = "Adds jitter to `LazyCredentialsCache`. This allows credentials with t | |
references = ["smithy-rs#2335"] | ||
meta = { "breaking" = false, "tada" = false, "bug" = false } | ||
author = "ysaito1001" | ||
|
||
[[aws-sdk-rust]] | ||
message = """Request IDs can now be easily retrieved on successful responses. For example, with S3: | ||
```rust | ||
// Import the trait to get the `request_id` method on outputs | ||
use aws_sdk_s3::types::RequestId; | ||
let output = client.list_buckets().send().await?; | ||
println!("Request ID: {:?}", output.request_id()); | ||
``` | ||
""" | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false } | ||
author = "jdisanti" | ||
|
||
[[aws-sdk-rust]] | ||
message = """Retrieving a request ID from errors now requires importing the `RequestId` trait. For example, with S3: | ||
```rust | ||
use aws_sdk_s3::types::RequestId; | ||
println!("Request ID: {:?}", error.request_id()); | ||
``` | ||
""" | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false } | ||
author = "jdisanti" | ||
|
||
[[smithy-rs]] | ||
message = "Generic clients no longer expose a `request_id()` function on errors. To get request ID functionality, use the SDK code generator." | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"} | ||
author = "jdisanti" | ||
|
||
[[aws-sdk-rust]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably teach our changelog format a way so that an entry is meant to be duplicated for the AWS SDK. |
||
message = "The `message()` and `code()` methods on errors have been moved into `ProvideErrorMetadata` trait. This trait will need to be imported to continue calling these." | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false } | ||
author = "jdisanti" | ||
|
||
[[smithy-rs]] | ||
message = "The `message()` and `code()` methods on errors have been moved into `ProvideErrorMetadata` trait. This trait will need to be imported to continue calling these." | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"} | ||
author = "jdisanti" | ||
|
||
[[aws-sdk-rust]] | ||
message = """ | ||
The `*Error` and `*ErrorKind` types have been combined to make error matching simpler. | ||
<details> | ||
<summary>Example with S3</summary> | ||
**Before:** | ||
```rust | ||
let result = client | ||
.get_object() | ||
.bucket(BUCKET_NAME) | ||
.key("some-key") | ||
.send() | ||
.await; | ||
match result { | ||
Ok(_output) => { /* Do something with the output */ } | ||
Err(err) => match err.into_service_error() { | ||
GetObjectError { kind, .. } => match kind { | ||
GetObjectErrorKind::InvalidObjectState(value) => println!("invalid object state: {:?}", value), | ||
GetObjectErrorKind::NoSuchKey(_) => println!("object didn't exist"), | ||
} | ||
err @ GetObjectError { .. } if err.code() == Some("SomeUnmodeledError") => {} | ||
err @ _ => return Err(err.into()), | ||
}, | ||
} | ||
``` | ||
**After:** | ||
```rust | ||
// Needed to access the `.code()` function on the error type: | ||
use aws_sdk_s3::types::ProvideErrorMetadata; | ||
let result = client | ||
.get_object() | ||
.bucket(BUCKET_NAME) | ||
.key("some-key") | ||
.send() | ||
.await; | ||
match result { | ||
Ok(_output) => { /* Do something with the output */ } | ||
Err(err) => match err.into_service_error() { | ||
GetObjectError::InvalidObjectState(value) => { | ||
println!("invalid object state: {:?}", value); | ||
} | ||
GetObjectError::NoSuchKey(_) => { | ||
println!("object didn't exist"); | ||
} | ||
err if err.code() == Some("SomeUnmodeledError") => {} | ||
err @ _ => return Err(err.into()), | ||
}, | ||
} | ||
``` | ||
</details> | ||
""" | ||
references = ["smithy-rs#76", "smithy-rs#2129", "smithy-rs#2075"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false } | ||
author = "jdisanti" | ||
|
||
[[smithy-rs]] | ||
message = """ | ||
The `*Error` and `*ErrorKind` types have been combined to make error matching simpler. | ||
<details> | ||
<summary>Example with S3</summary> | ||
**Before:** | ||
```rust | ||
let result = client | ||
.get_object() | ||
.bucket(BUCKET_NAME) | ||
.key("some-key") | ||
.send() | ||
.await; | ||
match result { | ||
Ok(_output) => { /* Do something with the output */ } | ||
Err(err) => match err.into_service_error() { | ||
GetObjectError { kind, .. } => match kind { | ||
GetObjectErrorKind::InvalidObjectState(value) => println!("invalid object state: {:?}", value), | ||
GetObjectErrorKind::NoSuchKey(_) => println!("object didn't exist"), | ||
} | ||
err @ GetObjectError { .. } if err.code() == Some("SomeUnmodeledError") => {} | ||
err @ _ => return Err(err.into()), | ||
}, | ||
} | ||
``` | ||
**After:** | ||
```rust | ||
// Needed to access the `.code()` function on the error type: | ||
use aws_sdk_s3::types::ProvideErrorMetadata; | ||
let result = client | ||
.get_object() | ||
.bucket(BUCKET_NAME) | ||
.key("some-key") | ||
.send() | ||
.await; | ||
match result { | ||
Ok(_output) => { /* Do something with the output */ } | ||
Err(err) => match err.into_service_error() { | ||
GetObjectError::InvalidObjectState(value) => { | ||
println!("invalid object state: {:?}", value); | ||
} | ||
GetObjectError::NoSuchKey(_) => { | ||
println!("object didn't exist"); | ||
} | ||
err if err.code() == Some("SomeUnmodeledError") => {} | ||
err @ _ => return Err(err.into()), | ||
}, | ||
} | ||
``` | ||
</details> | ||
""" | ||
references = ["smithy-rs#76", "smithy-rs#2129", "smithy-rs#2075"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"} | ||
author = "jdisanti" | ||
|
||
[[smithy-rs]] | ||
message = "`aws_smithy_types::Error` has been renamed to `aws_smithy_types::error::ErrorMetadata`." | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"} | ||
author = "jdisanti" | ||
|
||
[[aws-sdk-rust]] | ||
message = "`aws_smithy_types::Error` has been renamed to `aws_smithy_types::error::ErrorMetadata`." | ||
references = ["smithy-rs#76", "smithy-rs#2129"] | ||
meta = { "breaking" = true, "tada" = false, "bug" = false } | ||
author = "jdisanti" |
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,182 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
use aws_smithy_http::http::HttpHeaders; | ||
use aws_smithy_http::operation; | ||
use aws_smithy_http::result::SdkError; | ||
use aws_smithy_types::error::metadata::{ | ||
Builder as ErrorMetadataBuilder, ErrorMetadata, ProvideErrorMetadata, | ||
}; | ||
use aws_smithy_types::error::Unhandled; | ||
use http::{HeaderMap, HeaderValue}; | ||
|
||
/// Constant for the [`ErrorMetadata`] extra field that contains the request ID | ||
const AWS_REQUEST_ID: &str = "aws_request_id"; | ||
|
||
/// Implementers add a function to return an AWS request ID | ||
pub trait RequestId { | ||
/// Returns the request ID, or `None` if the service could not be reached. | ||
fn request_id(&self) -> Option<&str>; | ||
} | ||
|
||
impl<E, R> RequestId for SdkError<E, R> | ||
where | ||
R: HttpHeaders, | ||
{ | ||
fn request_id(&self) -> Option<&str> { | ||
match self { | ||
Self::ResponseError(err) => extract_request_id(err.raw().http_headers()), | ||
Self::ServiceError(err) => extract_request_id(err.raw().http_headers()), | ||
jdisanti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_ => None, | ||
} | ||
} | ||
} | ||
|
||
impl RequestId for ErrorMetadata { | ||
fn request_id(&self) -> Option<&str> { | ||
self.extra(AWS_REQUEST_ID) | ||
} | ||
} | ||
|
||
impl RequestId for Unhandled { | ||
fn request_id(&self) -> Option<&str> { | ||
self.meta().request_id() | ||
} | ||
} | ||
|
||
impl RequestId for operation::Response { | ||
fn request_id(&self) -> Option<&str> { | ||
extract_request_id(self.http().headers()) | ||
} | ||
} | ||
|
||
impl<B> RequestId for http::Response<B> { | ||
fn request_id(&self) -> Option<&str> { | ||
extract_request_id(self.headers()) | ||
} | ||
} | ||
|
||
impl<O, E> RequestId for Result<O, E> | ||
where | ||
O: RequestId, | ||
E: RequestId, | ||
{ | ||
fn request_id(&self) -> Option<&str> { | ||
match self { | ||
Ok(ok) => ok.request_id(), | ||
Err(err) => err.request_id(), | ||
} | ||
} | ||
} | ||
|
||
/// Applies a request ID to a generic error builder | ||
#[doc(hidden)] | ||
pub fn apply_request_id( | ||
builder: ErrorMetadataBuilder, | ||
headers: &HeaderMap<HeaderValue>, | ||
) -> ErrorMetadataBuilder { | ||
if let Some(request_id) = extract_request_id(headers) { | ||
builder.custom(AWS_REQUEST_ID, request_id) | ||
} else { | ||
builder | ||
} | ||
} | ||
|
||
/// Extracts a request ID from HTTP response headers | ||
fn extract_request_id(headers: &HeaderMap<HeaderValue>) -> Option<&str> { | ||
headers | ||
.get("x-amzn-requestid") | ||
.or_else(|| headers.get("x-amz-request-id")) | ||
.and_then(|value| value.to_str().ok()) | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use aws_smithy_http::body::SdkBody; | ||
use http::Response; | ||
|
||
#[test] | ||
fn test_request_id_sdk_error() { | ||
let without_request_id = | ||
|| operation::Response::new(Response::builder().body(SdkBody::empty()).unwrap()); | ||
let with_request_id = || { | ||
operation::Response::new( | ||
Response::builder() | ||
.header( | ||
"x-amzn-requestid", | ||
HeaderValue::from_static("some-request-id"), | ||
) | ||
.body(SdkBody::empty()) | ||
.unwrap(), | ||
) | ||
}; | ||
assert_eq!( | ||
None, | ||
SdkError::<(), _>::response_error("test", without_request_id()).request_id() | ||
); | ||
assert_eq!( | ||
Some("some-request-id"), | ||
SdkError::<(), _>::response_error("test", with_request_id()).request_id() | ||
); | ||
assert_eq!( | ||
None, | ||
SdkError::service_error((), without_request_id()).request_id() | ||
); | ||
assert_eq!( | ||
Some("some-request-id"), | ||
SdkError::service_error((), with_request_id()).request_id() | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_extract_request_id() { | ||
let mut headers = HeaderMap::new(); | ||
assert_eq!(None, extract_request_id(&headers)); | ||
|
||
headers.append( | ||
"x-amzn-requestid", | ||
HeaderValue::from_static("some-request-id"), | ||
); | ||
assert_eq!(Some("some-request-id"), extract_request_id(&headers)); | ||
|
||
headers.append( | ||
"x-amz-request-id", | ||
HeaderValue::from_static("other-request-id"), | ||
); | ||
assert_eq!(Some("some-request-id"), extract_request_id(&headers)); | ||
|
||
headers.remove("x-amzn-requestid"); | ||
assert_eq!(Some("other-request-id"), extract_request_id(&headers)); | ||
} | ||
|
||
#[test] | ||
fn test_apply_request_id() { | ||
let mut headers = HeaderMap::new(); | ||
assert_eq!( | ||
ErrorMetadata::builder().build(), | ||
apply_request_id(ErrorMetadata::builder(), &headers).build(), | ||
); | ||
|
||
headers.append( | ||
"x-amzn-requestid", | ||
HeaderValue::from_static("some-request-id"), | ||
); | ||
assert_eq!( | ||
ErrorMetadata::builder() | ||
.custom(AWS_REQUEST_ID, "some-request-id") | ||
.build(), | ||
apply_request_id(ErrorMetadata::builder(), &headers).build(), | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_error_metadata_request_id_impl() { | ||
let err = ErrorMetadata::builder() | ||
.custom(AWS_REQUEST_ID, "some-request-id") | ||
.build(); | ||
assert_eq!(Some("some-request-id"), err.request_id()); | ||
} | ||
} |
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems too harsh. Whitelabel clients should also be able to easily opt into retrieving request IDs without having to onboard onto the full SDK code generator --- perhaps the decorator could be enabled via a codegen config key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, a whitelabel client would already be using other parts of
sdk-codegen
for things like credentials, right? So those would be able to also use request ID after these changes. If there is a use case for pure/generic Smithy clients (generated bycodegen-client
) to have request ID, then it really should be in the Smithy spec.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the terms whitelabel and generic interchangeably, to refer to clients produced by
codegen-client
, without the SDK decorators insdk-codegen
.There are use cases for generic clients to retrieve request IDs from responses, services return unmodeled request IDs frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add these kinds of features to
sdk-codegen
, but make them consumable in isolation via thesmithy-build.json
configuration. Essentially we want varying degrees of the AWS SDK.