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

Add design doc for configuring SDK client for the orchestrator #2527

Closed
wants to merge 11 commits into from

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Mar 31, 2023

Motivation and Context

This doc describes how to configure an SDK client for the orchestrator, with a focus on the following:

  • Support for operation-level configuration
  • Support for runtime components required by the orchestrator

Rendered


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

@ysaito1001 ysaito1001 requested review from a team as code owners March 31, 2023 23:30
@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Just a couple comments.

Co-authored-by: John DiSanti <jdisanti@amazon.com>
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

This commit converts RFC to an evolving design document. The document
leaves out too many implementation details to be called an RFC. They
will be discovered over time and the document will be updated
accordingly rather than a snapshot of the decision record.
@ysaito1001 ysaito1001 changed the title Add RFC for client configuration for the orchestrator Add design doc for client configuration for the orchestrator Apr 6, 2023
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

design/src/client/configuration.md Outdated Show resolved Hide resolved
design/src/client/configuration.md Show resolved Hide resolved
design/src/client/configuration.md Show resolved Hide resolved
design/src/client/configuration.md Show resolved Hide resolved
design/src/client/configuration.md Outdated Show resolved Hide resolved
design/src/client/configuration.md Show resolved Hide resolved
design/src/client/configuration.md Show resolved Hide resolved
design/src/client/configuration.md Outdated Show resolved Hide resolved
design/src/client/configuration.md Outdated Show resolved Hide resolved
design/src/client/configuration.md Outdated Show resolved Hide resolved

The proposed methods generally take a type that implements [the corresponding trait from RFC 34](https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0034_smithy_orchestrator.md#traits). However, for backward compatibility reasons, we mark "No change" for `EndpointResolver`, `HttpClients`, and `IdentityProviders`:
- `EndpointResolver` - we [deprecated the previous the .endpoint_resolver method](https://github.com/awslabs/smithy-rs/pull/2464). Introducing a new `.endpoint_resolver` might run counter to Endpoints 2.0 migration.
- `HttpClients` - users have been able to specify custom connections using the existing method, and introducing a new method like `.connection(&self, impl Connection + 'static)` might require non-trivial upgrades.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should note how this interacts with future APIs that aren't HTTP-based.

design/src/client/configuration.md Outdated Show resolved Hide resolved
design/src/client/configuration.md Outdated Show resolved Hide resolved
Co-authored-by: Zelda Hessler <zhessler@amazon.com>
@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as draft April 13, 2023 22:53
@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

ysaito1001 added a commit that referenced this pull request Apr 18, 2023
This commit adds skeleton code for supporting operation level config. The
approach is described in #2527,
where fluent builders now have a new method, `config_override`, that
takes a service config builder
ysaito1001 added a commit that referenced this pull request Apr 19, 2023
## Motivation and Context
This PR adds skeleton code for supporting operation level config behind
`enableNewSmithyRuntime`.

## Description
The approach is described in
#2527, where fluent builders
now have a new method, `config_override`, that takes a service config
builder. The builder implements the `RuntimePlugin` trait and once
`TODO(RuntimePlugins)` is implemented, allows itself to put field values
into a config bag within `send_v2`.

## Testing
Added a line of code to an ignored test in `sra_test`, which, however,
does check compilation.

----

_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: Yuki Saito <awsaito@amazon.com>
Co-authored-by: John DiSanti <jdisanti@amazon.com>
@Velfi
Copy link
Contributor

Velfi commented Apr 20, 2023

I took the review tag off of this since it's back in draft. When it's ready for review again, re-tag it

unexge pushed a commit that referenced this pull request Apr 24, 2023
## Motivation and Context
This PR adds skeleton code for supporting operation level config behind
`enableNewSmithyRuntime`.

## Description
The approach is described in
#2527, where fluent builders
now have a new method, `config_override`, that takes a service config
builder. The builder implements the `RuntimePlugin` trait and once
`TODO(RuntimePlugins)` is implemented, allows itself to put field values
into a config bag within `send_v2`.

## Testing
Added a line of code to an ignored test in `sra_test`, which, however,
does check compilation.

----

_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: Yuki Saito <awsaito@amazon.com>
Co-authored-by: John DiSanti <jdisanti@amazon.com>
rcoh pushed a commit that referenced this pull request Apr 24, 2023
## Motivation and Context
This PR adds skeleton code for supporting operation level config behind
`enableNewSmithyRuntime`.

## Description
The approach is described in
#2527, where fluent builders
now have a new method, `config_override`, that takes a service config
builder. The builder implements the `RuntimePlugin` trait and once
`TODO(RuntimePlugins)` is implemented, allows itself to put field values
into a config bag within `send_v2`.

## Testing
Added a line of code to an ignored test in `sra_test`, which, however,
does check compilation.

----

_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: Yuki Saito <awsaito@amazon.com>
Co-authored-by: John DiSanti <jdisanti@amazon.com>
aws-sdk-rust-ci pushed a commit to awslabs/aws-sdk-rust that referenced this pull request May 1, 2023
## Motivation and Context
This PR adds skeleton code for supporting operation level config behind
`enableNewSmithyRuntime`.

## Description
The approach is described in
smithy-lang/smithy-rs#2527, where fluent builders
now have a new method, `config_override`, that takes a service config
builder. The builder implements the `RuntimePlugin` trait and once
`TODO(RuntimePlugins)` is implemented, allows itself to put field values
into a config bag within `send_v2`.

## Testing
Added a line of code to an ignored test in `sra_test`, which, however,
does check compilation.

----

_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: Yuki Saito <awsaito@amazon.com>
Co-authored-by: John DiSanti <jdisanti@amazon.com>
@ysaito1001 ysaito1001 changed the title Add design doc for client configuration for the orchestrator Add design doc for configuring SDK client for the orchestrator May 10, 2023
@ysaito1001 ysaito1001 marked this pull request as ready for review May 10, 2023 19:27
@ysaito1001 ysaito1001 requested review from jdisanti and Velfi May 10, 2023 19:28
Copy link
Contributor

@Velfi Velfi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great doc Yuki. I left some comments for wording, feel free to ignore any of them that you disagree with.


Terminology
-----------
- Component: An interface coupled with its default implementations to enable an SDK client functionality.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT - possible alternative wording:

Suggested change
- Component: An interface coupled with its default implementations to enable an SDK client functionality.
- Component: An interface which enables some aspect of SDK client functionality. All components have a default implementation.

- `Checksum Algorithms`: Configures how an SDK calculates request and response checksums.
- `Interceptors`: Configures specific stages of the request execution pipeline.

From an interface perspective, the aim of client configuration for the SDK is to provide public APIs for customizing these components as needed. From an implementation perspective, the goal of client configuration is to store the components in a typed configuration map for later use by the orchestrator during execution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT - I don't think "as needed" is necessary.

Suggested change
From an interface perspective, the aim of client configuration for the SDK is to provide public APIs for customizing these components as needed. From an implementation perspective, the goal of client configuration is to store the components in a typed configuration map for later use by the orchestrator during execution.
From an interface perspective, the aim of client configuration for the SDK is to provide public APIs for customizing these components. From an implementation perspective, the goal of client configuration is to store the components in a typed configuration map for later use by the orchestrator during execution.

- Operation: A high-level abstraction representing an interaction between an SDK Client and a remote service.
- Orchestrator: The code within an SDK client that handles the process of making requests and receiving responses from remote services.
- Remote Service: A remote API that a user wants to use. Communication with a remote service usually happens over HTTP. The remote service is usually, but not necessarily, an AWS service.
- SDK Client: A client generated for the AWS SDK, allowing users to make requests to remote services.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

Suggested change
- SDK Client: A client generated for the AWS SDK, allowing users to make requests to remote services.
- SDK Client: A client generated for the AWS SDK, allowing users to make requests to AWS remote services.


`ConfigLoader`, `SdkConfig`, and service configs allow users to configure the necessary runtime components for today's [`Tower`-based infrastructure](https://github.com/awslabs/smithy-rs/blob/35f2f27a8380a1310c264a386e162cd9f2180137/rust-runtime/aws-smithy-client/src/lib.rs#L155-L245). The degree to which these components can be configured in the orchestrator will remain largely unchanged.

The following table shows for each runtime component (the left column), what method on `ConfigLoader`, `sdk_config::Builder`, and service config builder (e.g. `aws_sdk_s3::config::Builder`) are currently available (the middle column) and what new method will be available on those types as proposed by the design (the right column).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT - When listing things, I think we should actually use bulleted/numbered lists:

Suggested change
The following table shows for each runtime component (the left column), what method on `ConfigLoader`, `sdk_config::Builder`, and service config builder (e.g. `aws_sdk_s3::config::Builder`) are currently available (the middle column) and what new method will be available on those types as proposed by the design (the right column).
The following table shows:
- _(the left column)_ The runtime components.
- _(the middle column)_ Current methods of `ConfigLoader`, `sdk_config::Builder`, and service config builder (e.g. `aws_sdk_s3::config::Builder`) for setting those components.
- _(the right column)_ The proposed changes to those methods, if any.

Also, I changed the wording here, please update things if I didn't communicate your intent accurately.

| Checksum Algorithms | Not configurable on SDK client | No change |
| Interceptors | None | `.interceptor(&self, impl Interceptor + 'static)` |

If components can be configured today, their builder methods continue to exist in the orchestrator. There are no plans to support `TraceProbes` prior to GA because users can still set up `tracing::Subscriber` to specify where metrics are published. For `IdentityProviders`, in addition to `.credentials_provider`, we will introduce `.token_provider` as part of [this PR](https://github.com/awslabs/smithy-rs/pull/2627). `HTTPAuthSchemes`, `AuthSchemeResolver`s, and `Checksum Algorithms` will only be configurable on the generic client, but not on the SDK client. Finally, customers will be able to configure interceptors to inject logic into specific stages of the request execution pipeline, which will be enabled by [this PR](https://github.com/awslabs/smithy-rs/pull/2669).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT - possible alternative wording:

Suggested change
If components can be configured today, their builder methods continue to exist in the orchestrator. There are no plans to support `TraceProbes` prior to GA because users can still set up `tracing::Subscriber` to specify where metrics are published. For `IdentityProviders`, in addition to `.credentials_provider`, we will introduce `.token_provider` as part of [this PR](https://github.com/awslabs/smithy-rs/pull/2627). `HTTPAuthSchemes`, `AuthSchemeResolver`s, and `Checksum Algorithms` will only be configurable on the generic client, but not on the SDK client. Finally, customers will be able to configure interceptors to inject logic into specific stages of the request execution pipeline, which will be enabled by [this PR](https://github.com/awslabs/smithy-rs/pull/2669).
If a component is configurable today, its builder method(s) will continue to be supported by the orchestrator. There are no plans to support `TraceProbes` prior to GA because users can still set up `tracing::Subscriber` to specify where metrics are published. For `IdentityProviders`, in addition to `.credentials_provider`, we will introduce `.token_provider` as part of [this PR](https://github.com/awslabs/smithy-rs/pull/2627). `HTTPAuthSchemes`, `AuthSchemeResolver`s, and `Checksum Algorithms` will be configurable on the generic client, but **not** on the SDK client. Finally, customers will be able to configure interceptors to inject logic into specific stages of the request execution pipeline, which will be enabled by [this PR](https://github.com/awslabs/smithy-rs/pull/2669).


### Operation-level configuration

Currently, users are able to customize runtime configuration at multiple levels. `SdkConfig` is used to configure settings for all services, while a service config (e.g., `aws_sdk_s3::config::Config`) is used to configure settings for a specific service client. However, there has been no support for configuring settings for a single operation invocation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within our team, we refer often to different "levels" of configuration. I'm worried we don't explain this way of looking at things to outsiders. Would you mind linking to a footnote that briefly names the different levels and how they relate to one another? If you don't want to write a footnote, we may have an RFC or something you can link to instead.

You already explain it a bit here. You could take what you have and expand it, but I think moving stuff into a footnote is better because the explanation will otherwise "slow things down" for readers that understand the layered config.


The main benefit of this approach is simplicity for users. The only change required is to call the `config_override` method on an operation input fluent builder. If users do not wish to specify the operation level config, their workflow will remain unaffected.

How the design has been implemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT - you could probably just call this:

Suggested change
How the design has been implemented
Implementation

or "The Implementation"

- [Implement the UserAgentInterceptor for the SDK](https://github.com/awslabs/smithy-rs/pull/2550)
- [Add DefaultEndpointResolver to the orchestrator](https://github.com/awslabs/smithy-rs/pull/2577)

While each PR tackles different part of runtime components, some of them share a common porting strategy, which involves moving logic out of `make_operation` on an operation input struct and logic out of the associated Tower layer, and placing them in a struct that implements the relevant trait defined in [aws_smithy_runtime_api::client::orchestrator](https://github.com/awslabs/smithy-rs/blob/2b165037fd785ce122c993c1a59d3c8d5a3e522c/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs#L81-L130).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
While each PR tackles different part of runtime components, some of them share a common porting strategy, which involves moving logic out of `make_operation` on an operation input struct and logic out of the associated Tower layer, and placing them in a struct that implements the relevant trait defined in [aws_smithy_runtime_api::client::orchestrator](https://github.com/awslabs/smithy-rs/blob/2b165037fd785ce122c993c1a59d3c8d5a3e522c/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs#L81-L130).
While each PR tackles a different part of runtime components, some of them share a common porting strategy, which involves moving logic out of `make_operation` on an operation input struct and logic out of the associated Tower layer, and placing them in a struct that implements the relevant trait defined in [aws_smithy_runtime_api::client::orchestrator](https://github.com/awslabs/smithy-rs/blob/2b165037fd785ce122c993c1a59d3c8d5a3e522c/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs#L81-L130).

@Velfi
Copy link
Contributor

Velfi commented Jul 10, 2023

@ysaito1001 Can you give this a quick look and see if it can be merged? If something's missing from it, just create a TODO

@rcoh rcoh requested review from a team as code owners November 14, 2023 02:21
@ysaito1001
Copy link
Contributor Author

Closing this stale design doc in favor of #2887

@ysaito1001 ysaito1001 closed this Apr 12, 2024
@ysaito1001 ysaito1001 deleted the ysaito/rfc-client-config-for-orchestrator branch August 1, 2024 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants