Skip to content
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

Reading Extensions value from Response wrapper #990

Closed
2 tasks
declanvk opened this issue Dec 5, 2023 · 6 comments
Closed
2 tasks

Reading Extensions value from Response wrapper #990

declanvk opened this issue Dec 5, 2023 · 6 comments
Labels
feature-request A feature should be added or improved.

Comments

@declanvk
Copy link
Contributor

declanvk commented Dec 5, 2023

Describe the feature

I would like to be able to access the extension field of the Response wrapper.

The issue is that in an interceptor hook, I cannot use the try_into_http02x method because it requires an owned value and the context will only return a shared reference to the Response. Additionally, Response does not implement clone, so I wouldn't be able to response.clone().try_into_http02x().

Use Case

I need this access so that I can read information like the HttpInfo as part of an Intercept implementation that uses the read_after_attempt hook.

Proposed Solution

I realize that directly exposing the http::Extension doesn't make sense for a wrapper type where you are attempting to hide the http dependency.

Other options might be:

  • Implement Clone for the Response wrapper in a way that preserves the extensions value
  • Implement your own Extensions type that is compatible with http's and then expose that type in a public interface

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

A note for the community

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue, please leave a comment
@declanvk declanvk added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 5, 2023
@jdisanti
Copy link
Contributor

jdisanti commented Dec 5, 2023

What problem are you trying to solve with the extensions? Maybe there's an alternative we can propose.

@rcoh
Copy link
Contributor

rcoh commented Dec 5, 2023

We actually capture http_info in a generic way (and we use this internally).

We write CaptureSmithyConnection into the property bag:

let captured_connection = cfg.load::<CaptureSmithyConnection>().cloned();

ConnectionMetadata currently only exposes a narrow amount of information, but we could consider exposing more fields of HttpInfo

@rcoh rcoh added response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 5, 2023
@declanvk
Copy link
Contributor Author

declanvk commented Dec 5, 2023

What problem are you trying to solve with the extensions? Maybe there's an alternative we can propose.

Mostly I need the HttpInfo data so that I can attempt to uniquely identify the TCP connection (using local + remote addr + port). I'd also like to have the remote addr information generally so that I can record number of unique hosts that I'm connecting to.


We actually capture http_info in a generic way (and we use this internally).

Thanks for the example! Apply that to my own case, would I also need to write a new CaptureSmithyConnection to both the request extensions and the interceptor cfg? Like in

let capture_smithy_connection = CaptureSmithyConnection::new();
context
.request_mut()
.add_extension(capture_smithy_connection.clone());
cfg.interceptor_state().store_put(capture_smithy_connection);

Or can I assume (with some tests) that a built-in smithy Intercept implementation will write this to the configuration for me?

If yes, then I would only need the local_addr information recorded into the ConnectionMetadata struct. Around

let smithy_connection = ConnectionMetadata::new(
conn.is_proxied(),
http_info.map(|info| info.remote_addr()),
move || match capture_conn.connection_metadata().as_ref() {
Some(conn) => conn.poison(),
None => tracing::trace!("no connection existed to poison"),
},
);
would need to pull out the local_addr from the HttpInfo.

@declanvk
Copy link
Contributor Author

declanvk commented Dec 5, 2023

I do think that you might still want to implement Clone for Response, otherwise the try_into_http02x will not be useful for any Intercept hooks. Maybe something similar to the Request::try_clone and keep the warning on it that says that it will clear the extensions field?

It wouldn't be useful for my case, but it might make the interface more flexible for someone else.

@jdisanti jdisanti removed the response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days. label Dec 5, 2023
github-merge-queue bot pushed a commit to smithy-lang/smithy-rs that referenced this issue Dec 6, 2023
## Motivation and Context
I want to use this field to uniquely identify TCP connection based on
their `local_addr` + `remote_addr`.

See awslabs/aws-sdk-rust#990 for additional
motivation for this change.

## Description
- Add a new optional `local_addr` field in the `ConnectionMetadata`
struct.
- Transfer the `local_addr` `SocketAddress` from the `hyper::HttpInfo`
to the `ConnectionMetadata` field.
- Add to the `trace-serialize` example program so that it will print out
the capture connection values.

## Testing
`cargo test` in `rust-runtime/aws-smithy-runtime-api` and
`aws-smithy-runtime`.

Also ran:
```
thedeck@c889f3b04fb0 examples % cargo run --example trace-serialize
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `/Users/thedeck/repos/github/declanvk/smithy-rs/target/debug/examples/trace-serialize`
2023-12-06T00:13:15.605555Z  INFO lazy_load_identity: aws_smithy_runtime::client::identity::cache::lazy: identity cache miss occurred; added new identity (took Ok(296µs))
2023-12-06T00:13:15.608344Z  INFO trace_serialize: Response received: response=Response { status: StatusCode(200), headers: Headers { headers: {"content-type": HeaderValue { _private: "application/json" }, "content-length": HeaderValue { _private: "17" }, "date": HeaderValue { _private: "Wed, 06 Dec 2023 00:13:15 GMT" }} }, body: SdkBody { inner: BoxBody, retryable: false }, extensions: Extensions }
2023-12-06T00:13:15.608388Z  INFO trace_serialize: Captured connection info remote_addr=Some(127.0.0.1:13734) local_addr=Some(127.0.0.1:50199)
2023-12-06T00:13:15.608511Z  INFO trace_serialize: Response received POKEMON_SERVICE_URL=http://localhost:13734 response=GetServerStatisticsOutput { calls_count: 0 }
```

You can see the log line with "Captured connection info" contains the
`remote_addr` and the `local_addr` fields.

## Checklist
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

Co-authored-by: Declan Kelly <thedeck@amazon.com>
@jdisanti
Copy link
Contributor

jdisanti commented Dec 6, 2023

@declanvk contributed a change to smithy-rs that will solve their problem in smithy-lang/smithy-rs#3286

Accessing http extensions is not something we would like to support unless there is a very compelling reason for it, so I'm going to close this for now.

@jdisanti jdisanti closed this as completed Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

3 participants