Skip to content

Conversation

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 11, 2022

I happened across this when trying to implement my own middleware that looked a lot like AwsMiddleware. It seems some IDE type hints were copied over along with the code itself, so I removes those and fixed a s/,/;/, and the example code works again.

The reason these weren't caused by CI/tests btw is that this code block is marked as ignored, presumably because it'd require a bunch of dependencies to compile.

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

@jonhoo jonhoo requested a review from a team as a code owner April 11, 2022 23:49
Copy link
Contributor

@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.

Thanks for fixing this! It looks like the CI failure is the intermittent failure I am fixing in #1311. I'll retry the failing steps as needed.

@github-actions
Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -13.25% 68645.02 79125.61
Total requests -13.23% 6181720 7124395
Total errors NaN% 0 0
Total successes -13.23% 6181720 7124395
Average latency ms -54.10% 0.56 1.22
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -30.06% 16.73 23.92
Stdev latency ms -65.73% 0.73 2.13
Transfer Mb -13.23% 642.59 740.58
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 12, 2022

It actually looks like the default stack has changed a bit since this was last written. Should it be updated to something more like

https://github.com/awslabs/smithy-rs/blob/9708aa830730956a5a5a1500c37a6f95fffc9bfb/aws/rust-runtime/aws-inlineable/src/middleware.rs#L21-L33

@jdisanti
Copy link
Contributor

The real AWS middleware in `aws-inlineable` has changed, but the example
hasn't changed to follow suit.
@jonhoo jonhoo force-pushed the fix-aws-stack-example branch from 71e0eda to afd0313 Compare April 12, 2022 18:04
@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 12, 2022

Thinking some more about this, is there a reason the AWS middleware definition is in aws-inlineable rather than being a public type in one of the smithy crates? That would certainly make it easier to re-use.

@github-actions
Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

@jdisanti
Copy link
Contributor

I think there are two reasons the default middleware is currently in aws-inlineable:

  1. Both aws-http and aws-sig-auth declare middleware layers for the stack, and the generated service crate is the only thing that depends on both of these and can consume all the middleware layers. To have default middleware in the runtime crates would require introducing another crate since aws-config is an optional dependency for customers.
  2. While most services use default middleware, some services customize the middleware stack for their specific use-cases. This is kind of a weaker reason though since they can just not use the default.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 12, 2022

I think there are two reasons the default middleware is currently in aws-inlineable:

1. Both `aws-http` and `aws-sig-auth` declare middleware layers for the stack, and the generated service crate is the only thing that depends on both of these and can consume all the middleware layers. To have default middleware in the runtime crates would require introducing another crate since `aws-config` is an optional dependency for customers.

Hmm, yeah, I suppose it would need to be its own crate. That said, there are also a fair number of aws-* crates, so is having one more actually a problem? It's rare that users will actually take dependencies on these manually, but it would give them the option. There may also be other things from aws-inlineable that could be moved to such a crate, I'm not sure?

@jdisanti
Copy link
Contributor

I'm not opposed to creating a new crate, but I think it probably deserves a RFC to lay out what it should be named and what should go into it. @rcoh probably has some strong opinions in this area.

@github-actions
Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 11.73% 80204.01 71784.54
Total requests 11.78% 7226077 6464550
Total errors NaN% 0 0
Total successes 11.78% 7226077 6464550
Average latency ms 114.29% 1.2 0.56
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -14.11% 24.05 28
Stdev latency ms 159.26% 2.1 0.81
Transfer Mb 11.78% 751.15 671.99
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 0.00% 88610.24 88609.56
Total requests 0.05% 7983517 7979735
Total errors NaN% 0 0
Total successes 0.05% 7983517 7979735
Average latency ms -1.00% 0.99 1
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 2.65% 24.38 23.75
Stdev latency ms 0.00% 1.87 1.87
Transfer Mb 0.05% 829.89 829.5
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@rcoh
Copy link
Collaborator

rcoh commented Apr 13, 2022

it isn't public because it's really a property of an individual service—having a public stack that we claim is "the stack" is a little misleading. There might be a "probably right" stack, but ultimately I expect that as things evolve different services may have different requirements

@rcoh
Copy link
Collaborator

rcoh commented Apr 13, 2022

that said, I'm not opposed to a service builder we expose in aws-http or something like that that does wire up the "normal" stuff

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 3.50% 41182.8 39790.3
Total requests 3.51% 3710461 3584734
Total errors NaN% 0 0
Total successes 3.51% 3710461 3584734
Average latency ms -10.89% 0.9 1.01
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 2.69% 71.49 69.62
Stdev latency ms -35.27% 1.45 2.24
Transfer Mb 3.51% 385.7 372.63
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@jdisanti jdisanti merged commit 572448b into main Apr 22, 2022
@jdisanti jdisanti deleted the fix-aws-stack-example branch April 22, 2022 18:21
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.

3 participants