From bd1def3ec4dce82fb069815ab5de29f359bae78d Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Fri, 8 Mar 2024 23:04:35 +0200 Subject: fix(tvix/store/grpc/pathinfo): skip_all fields, handle errors request only contains the outer metadata wrapping, and that's not too interesting: > Request { metadata: MetadataMap { headers: {"content-type": > "application/grpc", "user-agent": "grpc-go/1.60.1", "te": "trailers", > "grpc-accept-encoding": "gzip"} }, message: Streaming, extensions: > Extensions } Drop these fields for now, and rely on the underlying implementations to add instrumentation for the application-specific fields. Also, ensure we handle all error cases properly, and log them. We don't use `err` from instrument, as that'd also log an error on `Status::not_found`. Change-Id: Id1b983cb8b059c148c8a376f8802a1d28c59ba97 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11103 Reviewed-by: Connor Brewster Tested-by: BuildkiteCI Autosubmit: flokli --- .../src/proto/grpc_pathinfoservice_wrapper.rs | 35 ++++++++++++---------- 1 file changed, 20 insertions(+), 15 deletions(-) (limited to 'tvix/store/src/proto') diff --git a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs index a5c5776cd4ef..9f458182274d 100644 --- a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs +++ b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs @@ -27,7 +27,7 @@ where { type ListStream = BoxStream<'static, tonic::Result>; - #[instrument(skip(self))] + #[instrument(skip_all)] async fn get( &self, request: Request, @@ -43,7 +43,7 @@ where Ok(None) => Err(Status::not_found("PathInfo not found")), Ok(Some(path_info)) => Ok(Response::new(path_info)), Err(e) => { - warn!("failed to retrieve PathInfo: {}", e); + warn!(err = %e, "failed to get PathInfo"); Err(e.into()) } } @@ -51,7 +51,7 @@ where } } - #[instrument(skip(self))] + #[instrument(skip_all)] async fn put(&self, request: Request) -> Result> { let path_info = request.into_inner(); @@ -60,13 +60,13 @@ where match self.inner.put(path_info).await { Ok(path_info_new) => Ok(Response::new(path_info_new)), Err(e) => { - warn!("failed to insert PathInfo: {}", e); + warn!(err = %e, "failed to put PathInfo"); Err(e.into()) } } } - #[instrument(skip(self))] + #[instrument(skip_all)] async fn calculate_nar( &self, request: Request, @@ -74,21 +74,26 @@ where match request.into_inner().node { None => Err(Status::invalid_argument("no root node sent")), Some(root_node) => { - let (nar_size, nar_sha256) = self - .inner - .calculate_nar(&root_node) - .await - .expect("error during nar calculation"); // TODO: handle error + if let Err(e) = root_node.validate() { + warn!(err = %e, "invalid root node"); + Err(Status::invalid_argument("invalid root node"))? + } - Ok(Response::new(proto::CalculateNarResponse { - nar_size, - nar_sha256: nar_sha256.to_vec().into(), - })) + match self.inner.calculate_nar(&root_node).await { + Ok((nar_size, nar_sha256)) => Ok(Response::new(proto::CalculateNarResponse { + nar_size, + nar_sha256: nar_sha256.to_vec().into(), + })), + Err(e) => { + warn!(err = %e, "error during NAR calculation"); + Err(e.into()) + } + } } } } - #[instrument(skip(self))] + #[instrument(skip_all, err)] async fn list( &self, _request: Request, -- cgit 1.4.1