Skip to content

configurable middleware and metrics & http middleware#9596

Merged
mattsse merged 1 commit into
paradigmxyz:mainfrom
smatthewenglish:middleware-metrics
Jul 18, 2024
Merged

configurable middleware and metrics & http middleware#9596
mattsse merged 1 commit into
paradigmxyz:mainfrom
smatthewenglish:middleware-metrics

Conversation

@smatthewenglish
Copy link
Copy Markdown
Contributor

@smatthewenglish smatthewenglish commented Jul 18, 2024

note: this compiles, but i don't think it would achieve the goal of allowing a builder to pass arbitrary middleware, as you can see here

another thing i tried was splitting the building of the server and the starting of the server into two distinct steps, but the trait bounds started to get very complicated and ultimately i wasn't able to get it working


this trait bound will get the server to start correctly

RpcMiddleware: Layer<RpcRequestMetricsService<RpcService>> + Send + 'static,

but i wish that it was more dynamic, i tried something like this

RpcMiddleware: Layer<Box<dyn RpcServiceT<RpcService>>> + Send + 'static,

but that didnt work.


i adjusted the visibility of RpcRequestMetricsService to avoid this error

type `RpcRequestMetricsService<RpcService>` is more private than the item `RpcServerConfig::<HttpMiddleware, RpcMiddleware>::start`
`#[warn(private_bounds)]` on by default

'cause it seems like a better solution than using #[warn(private_bounds)]

Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, but can we do this in two steps, if I read this correctly, then this does two things

will investigate the trait bounds

Comment thread crates/rpc/rpc-builder/src/lib.rs Outdated
Comment on lines +1147 to +1148
/// Configurable HTTP middleware
http_middleware: ServiceBuilder<HttpMiddleware>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could we first install the rpcmiddleware before we look at the HTTP middleware?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, sounds good. i just made that change

@smatthewenglish smatthewenglish force-pushed the middleware-metrics branch 3 times, most recently from fc48884 to 1619ae3 Compare July 18, 2024 13:13
@mattsse mattsse added this pull request to the merge queue Jul 18, 2024
Merged via the queue into paradigmxyz:main with commit 8c8702b Jul 18, 2024
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.

2 participants