[Python] Allow to run pure Python request middlewares inside a Tower service #1734

crisidev · 2022-09-14T22:13:43Z

Motivation and Context

Customers want to be able to implement simple middleware directly in Python. This PR aims to add the initial support for it.
Improve the idiomatic experience of logging by exposing a handler compatible with Python's stardard library logging module.

Description

Middleware

A middleware is defined as a sync or async Python function that can return multiple values, following these rules:

Middleware not returning will let the execution continue without changing the original request.
Middleware returning a modified Request will update the original request before continuing the execution.
Middleware returning a Response will immediately terminate the request handling and return the response constructed from Python.
Middleware raising MiddlewareException will immediately terminate the request handling and return a protocol specific error, with the option of setting the HTTP return code.
Middleware raising any other exception will immediately terminate the request handling and return a protocol specific error, with HTTP status code 500.

Middlewares are registered into the Python application and executed in order of registration.

Example:

from sdk import App
from sdk.middleware import Request, MiddlewareException

app = App()

@app.request_middleware
def inject_header(request: Request):
    request.set_header("x-amzn-answer", "42")
    return request

@app.request_middleare
def check_header(request: Request):
    if request.get_header("x-amzn-answer") != "42":
        raise MiddlewareException("Wrong answer", 404)

@app.request_middleware
def dump_headers(request: Request):
    logging.debug(f"Request headers after middlewares: {request.headers()}")

NOTE: this PR only adds support for request middlewares, which are executed before the operation handler. Response middlewares, executed after the operation are tracked here: #1754.

Logging

To improve the idiomatic experience, now logging need to be configured from the Python side by using the standard logging module. This allows customers to opt-out of our tracing based logging implementation and use their own and logging level is now driven by Python.

import logging
from sdk.logging import TracingHandler

logging.basicConfig(level=logging.INFO, handlers=[TracingHandler.handle()])

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or 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.

github-actions · 2022-09-14T22:26:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
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 · 2022-09-16T02:17:54Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

Signed-off-by: Bigo <[email protected]>

github-actions · 2022-09-16T06:16:26Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

Signed-off-by: Bigo <[email protected]>

github-actions · 2022-09-16T17:36:09Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-09-16T19:34:18Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-16T20:48:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-16T22:17:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-17T02:46:02Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-09-17T03:36:22Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-17T22:20:22Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

crisidev · 2022-09-20T17:13:03Z

rust-runtime/aws-smithy-http-server-python/src/server.rs

@@ -73,17 +77,15 @@ pub trait PyApp: Clone + pyo3::IntoPy<PyObject> {
                    .getattr(py, "pid")
                    .map(|pid| pid.extract(py).unwrap_or(-1))
                    .unwrap_or(-1);
-                tracing::debug!("Terminating worker {idx}, PID: {pid}");
+                println!("Terminating worker {idx}, PID: {pid}");


These tracing statements have been moved to println! since at this stage we do not have the tracing subscriber initialized.

We could use with_default with a fallback/default subscriber here? It's probably more effort than it's worth though.

I have actually fixed this issue in a new commit that I'll push in a bit!

crisidev · 2022-09-20T17:15:54Z

...smithy/rust/codegen/server/python/smithy/generators/PythonServerOperationHandlerGenerator.kt

+                #{pyo3}::Python::with_gil(|py| {
+                    let pyhandler: &#{pyo3}::types::PyFunction = handler.extract(py)?;
+                    let output = if handler.args == 1 {
+                        pyhandler.call1((input,))?
+                    } else {
+                        pyhandler.call1((input, state.0))?
+                    };
+                    output.extract::<$output>()


The reason for removing tokio::task::block_in_place is that we are still bound to the mutex on the GIL that is taken inside the Python::with_gil closure.

No matter what we do, this synchronous mutex will not allow tokio to continue the execution.

Removing block_in_place yields a significant performance improvement since it removes the overhead of scheduling new tasks.

Other frameworks using the same approach as Smithy-rs Python are also doing the same: sparckles/Robyn@4ef01e6

github-actions · 2022-09-20T17:36:08Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-20T19:59:21Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

...re/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerModuleGenerator.kt

rust-runtime/aws-smithy-http-server-python/src/logging.rs

rust-runtime/aws-smithy-http-server-python/src/middleware/request.rs

rust-runtime/aws-smithy-http-server-python/src/middleware/layer.rs

Co-authored-by: Burak <[email protected]>

hlbarber

Is it the case that this approach will be less expressive than a general tower::Layer, even after the response side middleware is also implemented?

A general tower::Layer, or more concretely the struct Middleware<S> { inner: S, ... } it applies around a S, has the ability to hold onto state from the request side to the response side.

For example, I might pop a request header, hold onto it while the S does some handling, and then insert it as a response header. Is this a class of behavior we want to support?

You can see this kind of middleware in Django too
https://docs.djangoproject.com/en/4.1/topics/http/middleware/#writing-your-own-middleware

If this were possible to support then request/response middleware would then become a subclass of this more general middleware.

rust-runtime/aws-smithy-http-server-python/src/middleware/layer.rs

hlbarber · 2022-09-21T14:33:45Z

...smithy/rust/codegen/server/python/smithy/generators/PythonServerOperationHandlerGenerator.kt

+                        filename = #{tracing}::field::Empty,
+                        lineno = #{tracing}::field::Empty
+                    );
+                    let guard = span.enter();


This method is handy if you prefer this style https://docs.rs/tracing/latest/tracing/struct.Span.html#method.entered

...smithy/rust/codegen/server/python/smithy/generators/PythonServerOperationHandlerGenerator.kt

hlbarber · 2022-09-21T14:58:31Z

rust-runtime/aws-smithy-http-server-python/src/server.rs

@@ -73,17 +77,15 @@ pub trait PyApp: Clone + pyo3::IntoPy<PyObject> {
                    .getattr(py, "pid")
                    .map(|pid| pid.extract(py).unwrap_or(-1))
                    .unwrap_or(-1);
-                tracing::debug!("Terminating worker {idx}, PID: {pid}");
+                println!("Terminating worker {idx}, PID: {pid}");


We could use with_default with a fallback/default subscriber here? It's probably more effort than it's worth though.

crisidev · 2022-09-21T17:06:48Z

Is it the case that this approach will be less expressive than a general tower::Layer, even after the response side middleware is also implemented?

A general tower::Layer, or more concretely the struct Middleware<S> { inner: S, ... } it applies around a S, has the ability to hold onto state from the request side to the response side.

This is an initial implementation of a subset of the features I would like to see available in middleware handling, but there are certain features that will be either extremely hard or not possible to do with PyO3: for example there is no way to store generics inside a Python compatible structure. I will handoff this to @unexge after this initial support for middleware is done. I think after it we can sit down together and discuss possible improvement and a more general implementation.

For example, I might pop a request header, hold onto it while the S does some handling, and then insert it as a response header. Is this a class of behavior we want to support?

You can see this kind of middleware in Django too https://docs.djangoproject.com/en/4.1/topics/http/middleware/#writing-your-own-middleware

We already discussed about using a class to express middleware instead of just functions, which will end up with something very similar to what Django does.

If this were possible to support then request/response middleware would then become a subclass of this more general middleware.

FYI, I would like to see most if not all the middleware we vend to customers to be implemented directly in Rust for performance and safety reasons, but I think it would be nice for customers to be able to implement simple middleware directly in Python.

github-actions · 2022-09-21T17:29:48Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-21T18:07:27Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-22T13:58:30Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-22T14:45:48Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-22T16:26:02Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-22T17:06:10Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-09-22T17:26:20Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

crisidev added 2 commits September 14, 2022 15:09

Initial runtime support for middlewares

d1d74fe

Cleanup

da9e052

crisidev mentioned this pull request Sep 14, 2022

[WIP] Generate server implementation for pure Python business logic #1367

Open

34 tasks

Codegenerate middleware support

aeeaa89

Support responses, errors and header updating in middleware chain

e20563e

Signed-off-by: Bigo <[email protected]>

Refactor and make errors consistently working

583c967

Signed-off-by: Bigo <[email protected]>

crisidev added 3 commits September 16, 2022 12:17

Fix the ability of changing the request between middlewares

047401c

Merge branch 'main' into crisidev/oxipy-middleware

88bbd11

Remove unused errors

aa2dc0a

Refactor

5e784f6

Remove trait

03c660b

Add testing of middleware handlers

28e3c09

crisidev added 5 commits September 16, 2022 20:17

Add end to end test of the service

f3e21de

Add end to end test of the layer

84cc06b

Merge branch 'main' into crisidev/oxipy-middleware

78a6b6f

Remove useless dependency

38d971c

Remove another useless dependency

cf7521d

crisidev added 2 commits September 17, 2022 15:07

Idiomatic logging refactoring

80cdbfc

Enable back logging tests

e79797f

Make clippy happy

b08d331

crisidev commented Sep 20, 2022

View reviewed changes

Update changelog

fb58bdf

crisidev requested a review from a team as a code owner September 20, 2022 17:24

Merge branch 'main' into crisidev/oxipy-middleware

e06d7d7

hlbarber self-requested a review September 21, 2022 11:13

unexge reviewed Sep 21, 2022

View reviewed changes

Apply suggestions from code review

8f98141

Co-authored-by: Burak <[email protected]>

hlbarber reviewed Sep 21, 2022

View reviewed changes

Merge branch 'main' into crisidev/oxipy-middleware

964a470

Refactor logging for a more idiomatic experience

4ee2743

crisidev added 2 commits September 22, 2022 14:47

Merge branch 'main' into crisidev/oxipy-middleware

b9a8e3b

Remove useless dependency

433c162

Merge branch 'main' into crisidev/oxipy-middleware

747b6c0

unexge approved these changes Sep 22, 2022

View reviewed changes

Fix documentation

da065a4

crisidev enabled auto-merge (squash) September 22, 2022 16:56

Merge branch 'main' into crisidev/oxipy-middleware

6c1f9ab

crisidev merged commit e5c8cf3 into main Sep 22, 2022

crisidev deleted the crisidev/oxipy-middleware branch September 22, 2022 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Allow to run pure Python request middlewares inside a Tower service #1734

[Python] Allow to run pure Python request middlewares inside a Tower service #1734

crisidev commented Sep 14, 2022 •

edited

Loading

github-actions bot commented Sep 14, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 17, 2022

crisidev Sep 20, 2022

hlbarber Sep 21, 2022

crisidev Sep 21, 2022

crisidev Sep 20, 2022

crisidev Sep 20, 2022

github-actions bot commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

hlbarber left a comment •

edited

Loading

hlbarber Sep 21, 2022

hlbarber Sep 21, 2022

crisidev commented Sep 21, 2022 •

edited

Loading

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

[Python] Allow to run pure Python request middlewares inside a Tower service #1734

[Python] Allow to run pure Python request middlewares inside a Tower service #1734

Conversation

crisidev commented Sep 14, 2022 • edited Loading

Motivation and Context

Description

Middleware

Logging

Checklist

github-actions bot commented Sep 14, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 17, 2022

crisidev Sep 20, 2022

Choose a reason for hiding this comment

hlbarber Sep 21, 2022

Choose a reason for hiding this comment

crisidev Sep 21, 2022

Choose a reason for hiding this comment

crisidev Sep 20, 2022

Choose a reason for hiding this comment

crisidev Sep 20, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

hlbarber left a comment • edited Loading

Choose a reason for hiding this comment

hlbarber Sep 21, 2022

Choose a reason for hiding this comment

hlbarber Sep 21, 2022

Choose a reason for hiding this comment

crisidev commented Sep 21, 2022 • edited Loading

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

crisidev commented Sep 14, 2022 •

edited

Loading

hlbarber left a comment •

edited

Loading

crisidev commented Sep 21, 2022 •

edited

Loading