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

[RFC] Server: add optional connection_guard in config #1301

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

links234
Copy link

While it is very nice to have a log which shows current number of connections, during prod outages and incident investigation it would be much better to have it exported as a metric (i.e. now you can plot it over time and correlate with other metrics easier).

I assume we want to leave the metrics service and setup to the user so we just have to expose this data.

I chose to allow the user to provide it's own ConnectionGuard so that he can keep a record for himself to poll later.

I reckon that this might not be the best way to do this (a more sound solution would be to pass a mpsc for tower to send connection events through), hence the request for comment with a simple solution.

@links234 links234 requested a review from a team as a code owner February 26, 2024 13:59
@@ -651,6 +658,12 @@ impl<HttpMiddleware, RpcMiddleware> Builder<HttpMiddleware, RpcMiddleware> {
self
}

/// Configure a custom [`ConnectionGuard`] for the server to use.
pub fn custom_connection_guard(mut self, guard: ConnectionGuard) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

From my perspective, this is similar to having a .max_connections() on the server builder.

If I understood the problem right, please correct me if I'm wrong, the only purpose of passing this object to the server is to later call ConnectionGuard::available_connections to check how many connections the server has active.

I think a much better approach for this would be writing a custom middleware, similar to the one we have for the logger layer:

pub struct RpcLogger<S> {
max: u32,
service: S,
}
impl<'a, S> RpcServiceT<'a> for RpcLogger<S>
where
S: RpcServiceT<'a>,
{
type Future = Instrumented<ResponseFuture<S::Future>>;
#[tracing::instrument(name = "method_call", skip_all, fields(method = request.method_name()), level = "trace")]
fn call(&self, request: Request<'a>) -> Self::Future {
rx_log_from_json(&request, self.max);
ResponseFuture { fut: self.service.call(request), max: self.max }.in_current_span()
}
}

If the number of connections is all you wish to monitor, a similar middleware was implemented for substrate chains:
https://github.com/paritytech/polkadot-sdk/blob/6c5a42a690f96d59dbdf94640188f5d5b5cc47e2/substrate/client/rpc-servers/src/middleware/metrics.rs#L61-L63

Copy link
Member

@niklasad1 niklasad1 Feb 26, 2024

Choose a reason for hiding this comment

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

Thanks for the PR.

I think it's an easy way to get the number connections on the server without writing some custom logic such as manual metrics, correct?

With this PR one could just to do:

let rpc_conns = ConnectionGuard::new(100);

tokio::spawn(async move  {
    let server = Server::builder().custom_connection_guard(rpc_conns.clone()).build(...).await.unwrap();
    server.start(...)
}

// some other task
do something with rpc_conns....

So, the idea now is that jsonrpsee now exposes "a low-level API" where the users can inject whatever data they want, have a look at this example: https://github.com/paritytech/jsonrpsee/blob/master/examples/examples/jsonrpsee_as_service.rs

Then it's possible to inject that data in specific JSON-RPC middleware (which @lexnv tried to explain) or somewhere else (up to each specific use-case).

However, utilizing the low-level API may be a little annoying for simple use-cases where one only wants a server and no custom stuff and then this PR could be useful I guess.

Thus, technically it's possible implement such thing already but much harder to do currently. Hmm, could be useful what does @lexnv @jsdw think?

Copy link
Author

Choose a reason for hiding this comment

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

If I understood the problem right, please correct me if I'm wrong, the only purpose of passing this object to the server is to later call ConnectionGuard::available_connections to check how many connections the server has active.

Yes, that is the reason.

If the number of connections is all you wish to monitor, a similar middleware was implemented for substrate chains:
https://github.com/paritytech/polkadot-sdk/blob/6c5a42a690f96d59dbdf94640188f5d5b5cc47e2/substrate/client/rpc-servers/src/middleware/metrics.rs#L61-L63

Thanks, I wasn't aware of this.

However, utilizing the low-level API may be a little annoying for simple use-cases where one only wants a server and no custom stuff and then this PR could be useful I guess.

I agree. While it might be easier to use the low-level api if you are just starting, it is annoying if you are already using the intended api.

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.

None yet

3 participants