[RFC] Server: add optional connection_guard in config

[links234]

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.

[lexnv]

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:

jsonrpsee/server/src/middleware/rpc/layer/logger.rs

Lines 66 to 83 in 6da4608

	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

[niklasad1]

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?

[links234]

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.

[niklasad1]

Hey @links234 sorry to late response but we just merged #1443 which exposes the ConnectionGuard as an extension which mean that you can access it in every rpc call which should be similar to this PR with the exception that can't poll it when you want unless you clone the ConnectionGuard and send it some other task.

Do you think it's sufficient your use-case? I would be happy not add this API