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

rpc: bump jsonrpsee v0.22 and fix race in rpc v2 chain_head #3230

Merged
merged 30 commits into from
Feb 14, 2024

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Feb 6, 2024

Close #2992

Breaking changes:

  • rpc server grafana metric substrate_rpc_requests_started is removed (not possible to implement anymore)
  • rpc server grafana metric substrate_rpc_requests_finished is removed (not possible to implement anymore)
  • rpc server ws ping/pong not ACK:ed within 30 seconds more than three times then the connection will be closed

Added

  • rpc server grafana metric substrate_rpc_sessions_time is added to get the duration for each websocket session

@niklasad1 niklasad1 added the T3-RPC_API This PR/Issue is related to RPC APIs. label Feb 6, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

rewrite rpc metrics as JSON-RPC middleware


//! JSON-RPC specific middleware.

pub mod metrics;
Copy link
Member Author

@niklasad1 niklasad1 Feb 6, 2024

Choose a reason for hiding this comment

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

I plan to add rate-limit middleware here as well but I'll do it in another PR and that's why I introduced a module for it

@niklasad1 niklasad1 added the R0-silent Changes should not be mentioned in any release notes label Feb 7, 2024
/// Number of Websocket sessions closed.
ws_sessions_closed: Option<Counter<U64>>,
/// Histogram over RPC websocket sessions.
ws_sessions_time: HistogramVec,
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because I think it's useful to know the connection duration.

@niklasad1 niklasad1 marked this pull request as ready for review February 7, 2024 16:56
@niklasad1 niklasad1 requested review from andresilva and a team as code owners February 7, 2024 16:56
@niklasad1 niklasad1 requested review from lexnv and removed request for a team and andresilva February 7, 2024 16:57
.enable_ws_ping(
PingConfig::new()
.ping_interval(Duration::from_secs(30))
.inactive_limit(Duration::from_secs(50)),
Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior before this release was: the server sends ping frames every 30 seconds and doesn't care about pong frames.
Now the behavior is: the server sends ping frames every 30 seconds and expects at least one pong frame (or event message -- correctly me if i'm wrong here) in at most 50 seconds.

Should we bump this to 60 seconds just to be sure? Not entirely convinced myself, since most RPC apis provide a response almost immediately

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add an API for folks to configure it themselves it in a follow-up PR but I added also that
the ping/pong must be missed three times.

I think that's a reasonable limit but better to write some breaking stuff in the CHANGELOG/prdoc for it

};

let make_service = make_service_fn(move |_conn: &AddrStream| {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, this will go away in the next release (when bringing hyper up to date); we could then think about how to tackle this best; just to simplify a bit this function

Copy link
Member Author

@niklasad1 niklasad1 Feb 8, 2024

Choose a reason for hiding this comment

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

it's not a big deal, it's just the we have launch the event loop and handle socket.accept() ourselves then upgrade it to a towerService

prdoc/pr_3230.prdoc Outdated Show resolved Hide resolved
prdoc/pr_3230.prdoc Outdated Show resolved Hide resolved
}
}

/// Response future for metrics.
Copy link
Member Author

Choose a reason for hiding this comment

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

Custom future to avoid boxing it.

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

LGTM

@niklasad1 niklasad1 added this pull request to the merge queue Feb 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2024
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5220628

@niklasad1 niklasad1 added this pull request to the merge queue Feb 14, 2024
Merged via the queue into master with commit c7c4fe0 Feb 14, 2024
127 of 128 checks passed
@niklasad1 niklasad1 deleted the na-bump-jsonrpsee-v0.22 branch February 14, 2024 22:42
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…tech#3230)

Close paritytech#2992 

Breaking changes:
- rpc server grafana metric `substrate_rpc_requests_started` is removed
(not possible to implement anymore)
- rpc server grafana metric `substrate_rpc_requests_finished` is removed
(not possible to implement anymore)
- rpc server ws ping/pong not ACK:ed within 30 seconds more than three
times then the connection will be closed

Added
- rpc server grafana metric `substrate_rpc_sessions_time` is added to
get the duration for each websocket session
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON-RPC messages arrive out of order
5 participants