From f46c0f9e63c7388000e93d3d942bd0c4d00ec559 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Tue, 17 Dec 2024 13:49:23 +0100 Subject: [PATCH] inject span data using tower http --- crates/bin/pd/Cargo.toml | 2 +- crates/bin/pd/src/main.rs | 25 ++++++++++++++++++++++--- crates/core/app/src/rpc.rs | 23 ----------------------- crates/util/tower-trace/src/lib.rs | 2 +- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/crates/bin/pd/Cargo.toml b/crates/bin/pd/Cargo.toml index e168982aa7..8831af8da6 100644 --- a/crates/bin/pd/Cargo.toml +++ b/crates/bin/pd/Cargo.toml @@ -110,7 +110,7 @@ tonic-web = { workspace = true } tower = { workspace = true, features = ["full"] } tower-abci = "0.18" tower-actor = "0.1.0" -tower-http = { workspace = true, features = ["cors"] } +tower-http = { workspace = true, features = ["cors", "trace"] } tower-service = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter", "ansi"] } diff --git a/crates/bin/pd/src/main.rs b/crates/bin/pd/src/main.rs index e18ca1f781..be7540df1e 100644 --- a/crates/bin/pd/src/main.rs +++ b/crates/bin/pd/src/main.rs @@ -21,10 +21,13 @@ use pd::{ }; use penumbra_app::app_version::check_and_update_app_version; use penumbra_app::{APP_VERSION, SUBSTORE_PREFIXES}; +use penumbra_tower_trace::remote_addr; use rand::Rng; use rand_core::OsRng; use tendermint_config::net::Address as TendermintAddress; +use tower::ServiceBuilder; use tower_http::cors::CorsLayer; +use tower_http::trace::TraceLayer; use tracing::Instrument as _; use tracing_subscriber::{prelude::*, EnvFilter}; use url::Url; @@ -130,8 +133,18 @@ async fn main() -> anyhow::Result<()> { let tm_proxy = penumbra_tendermint_proxy::TendermintProxy::new(cometbft_addr); - let grpc_routes = penumbra_app::rpc::routes(&storage, tm_proxy, enable_expensive_rpc)?; - // let grpc_server = penumbra_app::rpc::router(&storage, tm_proxy, enable_expensive_rpc)?; + let grpc_routes = penumbra_app::rpc::routes(&storage, tm_proxy, enable_expensive_rpc)? + .into_axum_router() + .layer( + ServiceBuilder::new().layer(TraceLayer::new_for_grpc().make_span_with( + |req: &http::Request<_>| match remote_addr(req) { + Some(remote_addr) => { + tracing::error_span!("grpc", ?remote_addr) + } + None => tracing::error_span!("grpc"), + }, + )), + ); // Create Axum routes for the frontend app. let frontend = pd::zipserve::router("/app/", pd::MINIFRONT_ARCHIVE_BYTES); @@ -139,7 +152,6 @@ async fn main() -> anyhow::Result<()> { // Now we drop down a layer of abstraction, from tonic to axum, and merge handlers. let router = grpc_routes - .into_axum_router() .merge(frontend) .merge(node_status) // Set rather permissive CORS headers for pd's gRPC: the service @@ -152,6 +164,13 @@ async fn main() -> anyhow::Result<()> { // Now start the GRPC server, initializing an ACME client to use as a certificate // resolver if auto-https has been enabled. if auto-https is not enabled, we will // instead spawn a future that will never return. + + // TODO(janis): Is `axum_server::bind` sufficient to accept http1 (for grpc-web) and + // http2 (for grpc) requests? + // + // See also this (about axum::serve, suggesting that it just works out of the box; does that + // apply to axum_server::bind as well?) + // https://github.com/tokio-rs/axum/blob/c596deafe48ed608775e312eef7d12ddbb0fd424/examples/websockets-http2/src/main.rs#L57-L59 let grpc_server = axum_server::bind(grpc_bind); let (grpc_server, acme_worker) = match grpc_auto_https { Some(domain) => { diff --git a/crates/core/app/src/rpc.rs b/crates/core/app/src/rpc.rs index 310390f3dc..ee90110f4c 100644 --- a/crates/core/app/src/rpc.rs +++ b/crates/core/app/src/rpc.rs @@ -65,29 +65,6 @@ pub fn routes( ) -> anyhow::Result { let ibc = penumbra_ibc::component::rpc::IbcQuery::::new(storage.clone()); - // TODO(janis): upgrading to tonic 0.12 meant that the batteries included server could no - // longer be used. So we now need to configure this on the axum/hyper server directly. - // What can be done? Instead of `trace_fn` one could make use of tower-http's - // TraceLayer. - // - // HTTP1 and HTTP2 now need to be configured at a higher level - but if we make use - // of `axum::serve`, then they seem to both be activated? - // See also this: - // https://github.com/tokio-rs/axum/blob/c596deafe48ed608775e312eef7d12ddbb0fd424/examples/websockets-http2/src/main.rs#L57-L59 - - // let grpc_server = tonic::transport::server::Server::builder() - // .trace_fn(|req| match remote_addr(req) { - // Some(remote_addr) => { - // tracing::error_span!("grpc", ?remote_addr) - // } - // None => tracing::error_span!("grpc"), - // }) - // // Allow HTTP/1, which will be used by grpc-web connections. - // // This is particularly important when running locally, as gRPC - // // typically uses HTTP/2, which requires HTTPS. Accepting HTTP/2 - // // allows local applications such as web browsers to talk to pd. - // .accept_http1(true) - let mut builder = Routes::builder(); builder // As part of #2932, we are disabling all timeouts until we circle back to our diff --git a/crates/util/tower-trace/src/lib.rs b/crates/util/tower-trace/src/lib.rs index 5e60baf249..0992a0f28b 100644 --- a/crates/util/tower-trace/src/lib.rs +++ b/crates/util/tower-trace/src/lib.rs @@ -11,7 +11,7 @@ use std::net::SocketAddr; // request level, but the hook available to do that gives us an http::Request // rather than a tonic::Request, so the tonic::Request::remote_addr method isn't // available. -pub fn remote_addr(req: &http::Request<()>) -> Option { +pub fn remote_addr(req: &http::Request) -> Option { use tonic::transport::server::TcpConnectInfo; // NOTE: needs to also check TlsConnectInfo if we use TLS req.extensions()