Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Trace response payload in default jsonrpsee middleware #12886

Merged
paritytech-processbot merged 2 commits into from
Dec 9, 2022
Merged

Trace response payload in default jsonrpsee middleware #12886

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9fc7dd
Trace result in default `jsonrpsee` middleware
tgmichel Dec 9, 2022
d837a54
`rpc_metrics::extra`
tgmichel Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Trace result in default jsonrpsee middleware
  • Loading branch information
@tgmichel
tgmichel committed Dec 9, 2022
commit f9fc7dd57bd8e33bb9b083734c8c5f6f548fc26d
3 changes: 2 additions & 1 deletion client/rpc-servers/src/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,9 @@ impl RpcMiddleware {
}

/// Called once the JSON-RPC request is finished and response is sent to the output buffer.
fn on_response(&self, _result: &str, started_at: std::time::Instant) {
fn on_response(&self, result: &str, started_at: std::time::Instant) {
log::trace!(target: "rpc_metrics", "[{}] on_response started_at={:?}", self.transport_label, started_at);
log::trace!(target: "extra_rpc_metrics", "[{}] result={:?}", self.transport_label, result);
Copy link
Member

@bkchr bkchr Dec 9, 2022

Choose a reason for hiding this comment

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

Not sure about the naming, can we not also just name it rpc_metrics?

Copy link
Contributor Author

@tgmichel tgmichel Dec 9, 2022

Choose a reason for hiding this comment

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

It's just to not interfere with the current setups. Node operators probably enable rpc target to get stats on rpc method call usage but likely don't care about the response payload in most cases, which can get quite big in size (and expensive to store) and is mostly useful in development environments to just debug. Separating both targets one can choose the granularity.

Copy link
Contributor

@niklasad1 niklasad1 Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

you can already get this with RUST_LOG="jsonrpsee=trace" (except the transport label) that log is super-noisy but shouldn't matter for development environments.

in addition jsonrpsee actually has separate tracing spans for each RPC method call to filter out stuff but it doesn't play well with the logger in substrate, I should probably fix that at some point.

however we truncate logs bigger 4096 characters currently, so I guess it could be useful

Copy link
Member

@bkchr bkchr Dec 9, 2022

Choose a reason for hiding this comment

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

It's just to not interfere with the current setups. Node operators probably enable rpc target to get stats on rpc method call usage but likely don't care about the response payload in most cases, which can get quite big in size (and expensive to store) and is mostly useful in development environments to just debug. Separating both targets one can choose the granularity.

People should not get metrics by parsing logs. They should use prometheus. Let's change the target name. If people are requiring better metrics, they should open issues and say what they need.

self.metrics.requests_finished.with_label_values(&[self.transport_label]).inc();
}
}
Expand Down