diff --git a/libs/utils/src/http/endpoint.rs b/libs/utils/src/http/endpoint.rs index 4a78f16cfb..db3642b507 100644 --- a/libs/utils/src/http/endpoint.rs +++ b/libs/utils/src/http/endpoint.rs @@ -33,8 +33,10 @@ struct RequestId(String); /// Adds a tracing info_span! instrumentation around the handler events, /// logs the request start and end events for non-GET requests and non-200 responses. /// +/// Usage: Replace `my_handler` with `|r| request_span(r, my_handler)` +/// /// Use this to distinguish between logs of different HTTP requests: every request handler wrapped -/// in this type will get request info logged in the wrapping span, including the unique request ID. +/// with this will get request info logged in the wrapping span, including the unique request ID. /// /// This also handles errors, logging them and converting them to an HTTP error response. /// @@ -54,65 +56,56 @@ struct RequestId(String); /// tries to achive with its `.instrument` used in the current approach. /// /// If needed, a declarative macro to substitute the |r| ... closure boilerplate could be introduced. -pub struct RequestSpan(pub H) +pub async fn request_span(request: Request, handler: H) -> R::Output where R: Future, ApiError>> + Send + 'static, - H: Fn(Request) -> R + Send + Sync + 'static; - -impl RequestSpan -where - R: Future, ApiError>> + Send + 'static, - H: Fn(Request) -> R + Send + Sync + 'static, + H: FnOnce(Request) -> R + Send + Sync + 'static, { - /// Creates a tracing span around inner request handler and executes the request handler in the contex of that span. - /// Use as `|r| RequestSpan(my_handler).handle(r)` instead of `my_handler` as the request handler to get the span enabled. - pub async fn handle(self, request: Request) -> Result, ApiError> { - let request_id = request.context::().unwrap_or_default().0; - let method = request.method(); - let path = request.uri().path(); - let request_span = info_span!("request", %method, %path, %request_id); + let request_id = request.context::().unwrap_or_default().0; + let method = request.method(); + let path = request.uri().path(); + let request_span = info_span!("request", %method, %path, %request_id); - let log_quietly = method == Method::GET; - async move { - let cancellation_guard = RequestCancelled::warn_when_dropped_without_responding(); - if log_quietly { - debug!("Handling request"); - } else { - info!("Handling request"); - } - - // No special handling for panics here. There's a `tracing_panic_hook` from another - // module to do that globally. - let res = (self.0)(request).await; - - cancellation_guard.disarm(); - - // Log the result if needed. - // - // We also convert any errors into an Ok response with HTTP error code here. - // `make_router` sets a last-resort error handler that would do the same, but - // we prefer to do it here, before we exit the request span, so that the error - // is still logged with the span. - // - // (Because we convert errors to Ok response, we never actually return an error, - // and we could declare the function to return the never type (`!`). However, - // using `routerify::RouterBuilder` requires a proper error type.) - match res { - Ok(response) => { - let response_status = response.status(); - if log_quietly && response_status.is_success() { - debug!("Request handled, status: {response_status}"); - } else { - info!("Request handled, status: {response_status}"); - } - Ok(response) - } - Err(err) => Ok(api_error_handler(err)), - } + let log_quietly = method == Method::GET; + async move { + let cancellation_guard = RequestCancelled::warn_when_dropped_without_responding(); + if log_quietly { + debug!("Handling request"); + } else { + info!("Handling request"); + } + + // No special handling for panics here. There's a `tracing_panic_hook` from another + // module to do that globally. + let res = handler(request).await; + + cancellation_guard.disarm(); + + // Log the result if needed. + // + // We also convert any errors into an Ok response with HTTP error code here. + // `make_router` sets a last-resort error handler that would do the same, but + // we prefer to do it here, before we exit the request span, so that the error + // is still logged with the span. + // + // (Because we convert errors to Ok response, we never actually return an error, + // and we could declare the function to return the never type (`!`). However, + // using `routerify::RouterBuilder` requires a proper error type.) + match res { + Ok(response) => { + let response_status = response.status(); + if log_quietly && response_status.is_success() { + debug!("Request handled, status: {response_status}"); + } else { + info!("Request handled, status: {response_status}"); + } + Ok(response) + } + Err(err) => Ok(api_error_handler(err)), } - .instrument(request_span) - .await } + .instrument(request_span) + .await } /// Drop guard to WARN in case the request was dropped before completion. @@ -212,9 +205,7 @@ pub fn make_router() -> RouterBuilder { .middleware(Middleware::post_with_info( add_request_id_header_to_response, )) - .get("/metrics", |r| { - RequestSpan(prometheus_metrics_handler).handle(r) - }) + .get("/metrics", |r| request_span(r, prometheus_metrics_handler)) .err_handler(route_error_handler) } @@ -225,12 +216,14 @@ pub fn attach_openapi_ui( ui_mount_path: &'static str, ) -> RouterBuilder { router_builder - .get(spec_mount_path, move |r| { - RequestSpan(move |_| async move { Ok(Response::builder().body(Body::from(spec)).unwrap()) }) - .handle(r) - }) - .get(ui_mount_path, move |r| RequestSpan( move |_| async move { - Ok(Response::builder().body(Body::from(format!(r#" + .get(spec_mount_path, + move |r| request_span(r, move |_| async move { + Ok(Response::builder().body(Body::from(spec)).unwrap()) + }) + ) + .get(ui_mount_path, + move |r| request_span(r, move |_| async move { + Ok(Response::builder().body(Body::from(format!(r#" @@ -260,7 +253,8 @@ pub fn attach_openapi_ui( "#, spec_mount_path))).unwrap()) - }).handle(r)) + }) + ) } fn parse_token(header_value: &str) -> Result<&str, ApiError> { diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 30c219f773..279f069be7 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -11,7 +11,7 @@ use storage_broker::BrokerClientChannel; use tenant_size_model::{SizeResult, StorageModel}; use tokio_util::sync::CancellationToken; use tracing::*; -use utils::http::endpoint::RequestSpan; +use utils::http::endpoint::request_span; use utils::http::json::json_request_or_empty_body; use utils::http::request::{get_request_param, must_get_query_param, parse_query_param}; @@ -1179,7 +1179,7 @@ pub fn make_router( #[cfg(not(feature = "testing"))] let handler = cfg_disabled; - move |r| RequestSpan(handler).handle(r) + move |r| request_span(r, handler) }}; } @@ -1194,54 +1194,50 @@ pub fn make_router( ) .context("Failed to initialize router state")?, )) - .get("/v1/status", |r| RequestSpan(status_handler).handle(r)) + .get("/v1/status", |r| request_span(r, status_handler)) .put( "/v1/failpoints", testing_api!("manage failpoints", failpoints_handler), ) - .get("/v1/tenant", |r| RequestSpan(tenant_list_handler).handle(r)) - .post("/v1/tenant", |r| { - RequestSpan(tenant_create_handler).handle(r) - }) - .get("/v1/tenant/:tenant_id", |r| { - RequestSpan(tenant_status).handle(r) - }) + .get("/v1/tenant", |r| request_span(r, tenant_list_handler)) + .post("/v1/tenant", |r| request_span(r, tenant_create_handler)) + .get("/v1/tenant/:tenant_id", |r| request_span(r, tenant_status)) .get("/v1/tenant/:tenant_id/synthetic_size", |r| { - RequestSpan(tenant_size_handler).handle(r) + request_span(r, tenant_size_handler) }) .put("/v1/tenant/config", |r| { - RequestSpan(update_tenant_config_handler).handle(r) + request_span(r, update_tenant_config_handler) }) .get("/v1/tenant/:tenant_id/config", |r| { - RequestSpan(get_tenant_config_handler).handle(r) + request_span(r, get_tenant_config_handler) }) .get("/v1/tenant/:tenant_id/timeline", |r| { - RequestSpan(timeline_list_handler).handle(r) + request_span(r, timeline_list_handler) }) .post("/v1/tenant/:tenant_id/timeline", |r| { - RequestSpan(timeline_create_handler).handle(r) + request_span(r, timeline_create_handler) }) .post("/v1/tenant/:tenant_id/attach", |r| { - RequestSpan(tenant_attach_handler).handle(r) + request_span(r, tenant_attach_handler) }) .post("/v1/tenant/:tenant_id/detach", |r| { - RequestSpan(tenant_detach_handler).handle(r) + request_span(r, tenant_detach_handler) }) .post("/v1/tenant/:tenant_id/load", |r| { - RequestSpan(tenant_load_handler).handle(r) + request_span(r, tenant_load_handler) }) .post("/v1/tenant/:tenant_id/ignore", |r| { - RequestSpan(tenant_ignore_handler).handle(r) + request_span(r, tenant_ignore_handler) }) .get("/v1/tenant/:tenant_id/timeline/:timeline_id", |r| { - RequestSpan(timeline_detail_handler).handle(r) + request_span(r, timeline_detail_handler) }) .get( "/v1/tenant/:tenant_id/timeline/:timeline_id/get_lsn_by_timestamp", - |r| RequestSpan(get_lsn_by_timestamp_handler).handle(r), + |r| request_span(r, get_lsn_by_timestamp_handler), ) .put("/v1/tenant/:tenant_id/timeline/:timeline_id/do_gc", |r| { - RequestSpan(timeline_gc_handler).handle(r) + request_span(r, timeline_gc_handler) }) .put( "/v1/tenant/:tenant_id/timeline/:timeline_id/compact", @@ -1253,34 +1249,34 @@ pub fn make_router( ) .post( "/v1/tenant/:tenant_id/timeline/:timeline_id/download_remote_layers", - |r| RequestSpan(timeline_download_remote_layers_handler_post).handle(r), + |r| request_span(r, timeline_download_remote_layers_handler_post), ) .get( "/v1/tenant/:tenant_id/timeline/:timeline_id/download_remote_layers", - |r| RequestSpan(timeline_download_remote_layers_handler_get).handle(r), + |r| request_span(r, timeline_download_remote_layers_handler_get), ) .delete("/v1/tenant/:tenant_id/timeline/:timeline_id", |r| { - RequestSpan(timeline_delete_handler).handle(r) + request_span(r, timeline_delete_handler) }) .get("/v1/tenant/:tenant_id/timeline/:timeline_id/layer", |r| { - RequestSpan(layer_map_info_handler).handle(r) + request_span(r, layer_map_info_handler) }) .get( "/v1/tenant/:tenant_id/timeline/:timeline_id/layer/:layer_file_name", - |r| RequestSpan(layer_download_handler).handle(r), + |r| request_span(r, layer_download_handler), ) .delete( "/v1/tenant/:tenant_id/timeline/:timeline_id/layer/:layer_file_name", - |r| RequestSpan(evict_timeline_layer_handler).handle(r), + |r| request_span(r, evict_timeline_layer_handler), ) .put("/v1/disk_usage_eviction/run", |r| { - RequestSpan(disk_usage_eviction_run).handle(r) + request_span(r, disk_usage_eviction_run) }) .put( "/v1/tenant/:tenant_id/break", testing_api!("set tenant state to broken", handle_tenant_break), ) - .get("/v1/panic", |r| RequestSpan(always_panic_handler).handle(r)) + .get("/v1/panic", |r| request_span(r, always_panic_handler)) .post( "/v1/tracing/event", testing_api!("emit a tracing event", post_tracing_event_handler),