feat: add middleware #502
feat: add middleware #502
Conversation
Add middleware implementations for the async and synchronous Momento clients. Add aio and sync middleware to the configuration. Add example async and sync middleware implementations that match the node versions. These were used to test the middleware interceptors in conjunction with the integration tests and examples.
Combine the add middleware configuration functions to take either sync or async middleware. The get functions still have async and sync versions, so the different clients can get just the middleware they can use. Remove the example middleware to keep clutter out of the codebase. We can add an example in the actual examples later if desired.
Make the async middleware interceptor return a call even when it receives an error, since raising an error interferes with retries. Move the middleware interceptor to the end of the interceptors list to allow us to capture data about retries.
pgautier404
left a comment
pgautier404
left a comment
There was a problem hiding this comment.
LGTM! I was able to implement sync and async middleware both internal and external to the SDK, and the UX was nice.
malandis
left a comment
malandis
left a comment
There was a problem hiding this comment.
Looks great. My only suggestion is to use dataclasses where possible. Those can tidy up the record types (the middleware models and metadata). Dataclasses auto-generate equality, hash, str, and repr methods which really helps debugging. For dataclasses that you intend to be immutable, can pass in the frozen flag.
I'm approving now, recommend the dataclasses now or in a follow up PR
| """ | ||
| return self._middlewares.copy() | ||
|
|
||
| def get_aio_middlewares(self) -> List[momento.config.middleware.aio.Middleware]: |
There was a problem hiding this comment.
Would prefer get_async_middlewares to parallel the cache naming.
| from grpc.aio import Metadata | ||
|
|
||
|
|
||
| class MiddlewareMetadata: |
There was a problem hiding this comment.
Consider making this a dataclass. You'd have to lift grpc_metadata to be a class variable.
| def __init__(self, metadata: Optional[Metadata]): | ||
| self.grpc_metadata = metadata | ||
|
|
||
| def get_grpc_metadata(self) -> Optional[Metadata]: |
There was a problem hiding this comment.
Accessors like this are uncommon. Can make grpc_metadata private by prefixing with an underscore then make a property if you want to hide it. Or if you go with the dataclass route then you just leave it public.
| CONNECTION_ID_KEY = "connectionID" | ||
|
|
||
|
|
||
| class MiddlewareMessage: |
There was a problem hiding this comment.
Everything here could be a dataclass. The sole field of this one would be grpc_message, then you could make property-like accessors for message_length, constructor_name. Accessing those in the code would be:
m = MiddlewareMessage(message="asdf")
m.message_length
m.constructor_name
m.message
|
I'll prep a follow up pr to fix these issues. |
Add middleware implementations for the async and synchronous Momento clients.
Add aio and sync middleware to the configuration.
Add example async and sync middleware implementations that match the node versions. These were used to test the middleware interceptors in conjunction with the integration tests and examples.