mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(tvix/store/grpc/pathinfo): skip_all fields, handle errors

Browse source 
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 <cbrewster@hey.com>
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
This commit is contained in:
Florian Klink 2024-03-08 23:04:35 +02:00 committed by clbot
parent 4e78de7393
commit bd1def3ec4
1 changed files with 20 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
View file

@ -27,7 +27,7 @@ where
{ {
type ListStream = BoxStream<'static, tonic::Result<proto::PathInfo, Status>>; type ListStream = BoxStream<'static, tonic::Result<proto::PathInfo, Status>>;
#[instrument(skip(self))] #[instrument(skip_all)]
async fn get( async fn get(
&self, &self,
request: Request<proto::GetPathInfoRequest>, request: Request<proto::GetPathInfoRequest>,
@ -43,7 +43,7 @@ where
Ok(None) => Err(Status::not_found("PathInfo not found")), Ok(None) => Err(Status::not_found("PathInfo not found")),
Ok(Some(path_info)) => Ok(Response::new(path_info)), Ok(Some(path_info)) => Ok(Response::new(path_info)),
Err(e) => { Err(e) => {
warn!("failed to retrieve PathInfo: {}", e); warn!(err = %e, "failed to get PathInfo");
Err(e.into()) Err(e.into())
} }
} }
@ -51,7 +51,7 @@ where
} }
} }
#[instrument(skip(self))] #[instrument(skip_all)]
async fn put(&self, request: Request<proto::PathInfo>) -> Result<Response<proto::PathInfo>> { async fn put(&self, request: Request<proto::PathInfo>) -> Result<Response<proto::PathInfo>> {
let path_info = request.into_inner(); let path_info = request.into_inner();
@ -60,13 +60,13 @@ where
match self.inner.put(path_info).await { match self.inner.put(path_info).await {
Ok(path_info_new) => Ok(Response::new(path_info_new)), Ok(path_info_new) => Ok(Response::new(path_info_new)),
Err(e) => { Err(e) => {
warn!("failed to insert PathInfo: {}", e); warn!(err = %e, "failed to put PathInfo");
Err(e.into()) Err(e.into())
} }
} }
} }
#[instrument(skip(self))] #[instrument(skip_all)]
async fn calculate_nar( async fn calculate_nar(
&self, &self,
request: Request<castorepb::Node>, request: Request<castorepb::Node>,
@ -74,21 +74,26 @@ where
match request.into_inner().node { match request.into_inner().node {
None => Err(Status::invalid_argument("no root node sent")), None => Err(Status::invalid_argument("no root node sent")),
Some(root_node) => { Some(root_node) => {
let (nar_size, nar_sha256) = self if let Err(e) = root_node.validate() {
.inner warn!(err = %e, "invalid root node");
.calculate_nar(&root_node) Err(Status::invalid_argument("invalid root node"))?
.await }
.expect("error during nar calculation"); // TODO: handle error
Ok(Response::new(proto::CalculateNarResponse { match self.inner.calculate_nar(&root_node).await {
Ok((nar_size, nar_sha256)) => Ok(Response::new(proto::CalculateNarResponse {
nar_size, nar_size,
nar_sha256: nar_sha256.to_vec().into(), 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( async fn list(
&self, &self,
_request: Request<proto::ListPathInfoRequest>, _request: Request<proto::ListPathInfoRequest>,