Expose the `CustomizableSend` type in each SDK trait to allow treating operations generically

declanvk commented 11 months ago

Describe the feature

I would like to expose the CustomizableSend trait that is used in the bounds of the CustomizableOperation::send call as public.

Use Case

The reason that I would like to have this trait public, but not implementable is so that I can generically call CustomizableOperation::send call without needing to know the concrete operation types.

For example:

async fn send_with_interceptor<T, E, B>(
    operation: CustomizableOperation<T, E, B>,
) -> (Result<T, SdkError<E, Response<SdkBody>>>, Option<RequestExecution>)
where
    E: Error + Send + Sync + 'static,
    B: aws_sdk_dynamodb::client::customize::internal::CustomizableSend<T, E>,
    // fails with "error[E0603]: module `internal` is private"
{
    operation
        .interceptor(SomeInterceptor)
        .send()
        .await
}

Overall, I would like to be able to do some things with the generic operations, like:

Apply an interceptor to the custom operation
Call send() and .await it

Proposed Solution

I propose that this trait be made public. However, I wouldn't want this trait to be implemented by external crates, so it would be necessary to use a private Seal marker trait as a supertrait to prevent that.

Acknowledgements

[x] I may be able to implement this feature request (maybe)
[ ] 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

rcoh commented 11 months ago

yeah seems like a reasonable thing to do. I think we'll need to do an RFC process to consider any implications. In the meantime, I'd suggest using a macro. (approximate, I didn't test this)

macro_rules! send_intercepted {
  ($builder: expr, $interceptor: expr) => { $builder.customize().interceptor($interceptor).send().await }
}

jmklix commented 5 months ago

Can you provide a high level description of a use case for this that isn't currently possible with this sdk?

declanvk commented 5 months ago

I want to add a custom interceptor to all DynamoDB calls made in my program so that I can extract some specific timing information, info about number of attempts, request IDs, etc. To make sure that all calls get this interceptor attached, I've created a wrapper DDB client that delegates to the real DDB client underneath.

I'd like to be able to abstract away some of the wrapper client code that is shared amongst all function, like I showed in the initial example. Specifically, I wanted a way to pass in a generic operation and attach an interceptor to that generic operation.

Currently I'm using a macro to reduce the copy-paste pain, but I would prefer some trait-using solution here so that I can get better error messages and not have to use a macro. Or at least reduce the amount of code which is in the macro.

rcoh commented 5 months ago

Is there a reason you can't add the interceptor on the Client itself? You can configure interceptors at both the Client::Config and SdkConfig level.

On Fri, May 10, 2024 at 8:18 PM Declan Kelly @.***> wrote:

I want to add a custom interceptor to all DynamoDB calls made in my program so that I can extract some specific timing information, info about number of attempts, request IDs, etc. To make sure that all calls get this interceptor attached, I've created a wrapper DDB client that delegates to the real DDB client underneath.

I'd like to be able to abstract away some of the wrapper client code that is shared amongst all function, like I showed in the initial example. Specifically, I wanted a way to pass in a generic operation and attach an interceptor to that generic operation.

Currently I'm using a macro to reduce the copy-paste pain, but I would prefer some trait-using solution here so that I can get better error messages and not have to use a macro. Or at least reduce the amount of code which is in the macro.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-sdk-rust/issues/987#issuecomment-2105402277, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADYKZ2L5DZQEUABFSRBB5TZBVPT3AVCNFSM6AAAAABADSO2H2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBVGQYDEMRXG4 . You are receiving this because you commented.Message ID: @.***>

declanvk commented 5 months ago

Is there a reason you can't add the interceptor on the Client itself?

Couple reasons, mostly because the interceptor is created with state that is available at the time the user is making the call, so I create the interceptor at that point. I might be able to refactor some of that state to be derived from things returned by the interceptor hook.

Also, I'm a little unsure, but I believe that the interceptor state at the client level would be shared amongst all client calls, which I think would be a disadvantage in this scenario. I don't want my interceptor to have to contain a Mutex<HashMap<CallId, State>> entry that would be contended among by all threads making an API call.

awslabs / aws-sdk-rust