From b881712b187ef1168be57756a7b4e89fd6a58ba3 Mon Sep 17 00:00:00 2001 From: Gautam Gupta Date: Mon, 21 Oct 2024 17:19:19 -0700 Subject: [PATCH] rename NotFound error GitOrigin-RevId: f74b563063286a05e86fbd3305f0a0c0df9d2c37 --- crates/application/src/lib.rs | 26 +++++++------ crates/application/src/snapshot_import.rs | 2 +- crates/errors/src/lib.rs | 38 +++++++++---------- crates/local_backend/src/schema.rs | 2 +- crates/model/src/components/config.rs | 2 +- crates/model/src/environment_variables/mod.rs | 2 +- crates/model/src/snapshot_imports/mod.rs | 13 +++---- crates/pb/src/error_metadata.rs | 4 +- crates/runtime/src/prod.rs | 12 ++++++ 9 files changed, 56 insertions(+), 45 deletions(-) diff --git a/crates/application/src/lib.rs b/crates/application/src/lib.rs index 64daff39..ac879258 100644 --- a/crates/application/src/lib.rs +++ b/crates/application/src/lib.rs @@ -1394,7 +1394,7 @@ impl Application { .completed_export_at_ts(snapshot_ts) .await?; let export: ParsedDocument = export_doc - .context(ErrorMetadata::transient_not_found( + .context(ErrorMetadata::not_found( "ExportNotFound", format!("The requested export {snapshot_ts} was not found"), ))? @@ -1409,12 +1409,14 @@ impl Application { }, } }; - let storage_get_stream = self.exports_storage.get(&object_key).await?.context( - ErrorMetadata::transient_not_found( - "ExportNotFound", - format!("The requested export {snapshot_ts}/{object_key:?} was not found"), - ), - )?; + let storage_get_stream = + self.exports_storage + .get(&object_key) + .await? + .context(ErrorMetadata::not_found( + "ExportNotFound", + format!("The requested export {snapshot_ts}/{object_key:?} was not found"), + ))?; Ok(storage_get_stream) } @@ -2468,7 +2470,7 @@ impl Application { .get_file_entry(&mut file_storage_tx, component.into(), storage_id.clone()) .await? else { - return Err(ErrorMetadata::transient_not_found( + return Err(ErrorMetadata::not_found( "FileNotFound", format!("File {storage_id} not found"), ) @@ -2491,14 +2493,14 @@ impl Application { .get_file_entry(&mut file_storage_tx, component.into(), storage_id.clone()) .await? else { - return Err(ErrorMetadata::transient_not_found( + return Err(ErrorMetadata::not_found( "FileNotFound", format!("File {storage_id} not found"), ) .into()); }; let Some(component_path) = file_storage_tx.get_component_path(component) else { - return Err(ErrorMetadata::transient_not_found( + return Err(ErrorMetadata::not_found( "FileNotFound", format!("Component {component:?} not found"), ) @@ -2528,14 +2530,14 @@ impl Application { .get_file_entry(&mut file_storage_tx, component.into(), storage_id.clone()) .await? else { - return Err(ErrorMetadata::transient_not_found( + return Err(ErrorMetadata::not_found( "FileNotFound", format!("File {storage_id} not found"), ) .into()); }; let Some(component_path) = file_storage_tx.get_component_path(component) else { - return Err(ErrorMetadata::transient_not_found( + return Err(ErrorMetadata::not_found( "FileNotFound", format!("Component {component:?} not found"), ) diff --git a/crates/application/src/snapshot_import.rs b/crates/application/src/snapshot_import.rs index 34508973..82d04652 100644 --- a/crates/application/src/snapshot_import.rs +++ b/crates/application/src/snapshot_import.rs @@ -1501,7 +1501,7 @@ async fn wait_for_import_worker( import_model .get(import_id) .await? - .context(ErrorMetadata::transient_not_found( + .context(ErrorMetadata::not_found( "ImportNotFound", format!("import {import_id} not found"), ))?; diff --git a/crates/errors/src/lib.rs b/crates/errors/src/lib.rs index 3f89cbfe..a77eb978 100644 --- a/crates/errors/src/lib.rs +++ b/crates/errors/src/lib.rs @@ -45,7 +45,7 @@ pub enum ErrorCode { BadRequest, Unauthenticated, Forbidden, - TransientNotFound, + NotFound, ClientDisconnect, RateLimited, @@ -91,17 +91,17 @@ impl ErrorMetadata { /// a deterministic user error. It should typically be used when the /// resource can't be currently found, e.g. the backend is not currently /// in service discovery. If the UDF is missing, this should throw - /// `bad_request`` insteaFd, which is a deterministic user error. + /// `bad_request`` instead, which is a deterministic user error. /// /// The short_msg should be a CapitalCamelCased describing the error (eg /// FileNotFound). The msg should be a descriptive message targeted /// toward the developer. - pub fn transient_not_found( + pub fn not_found( short_msg: impl Into>, msg: impl Into>, ) -> Self { Self { - code: ErrorCode::TransientNotFound, + code: ErrorCode::NotFound, short_msg: short_msg.into(), msg: msg.into(), } @@ -331,8 +331,8 @@ impl ErrorMetadata { self.code == ErrorCode::BadRequest } - pub fn is_transient_not_found(&self) -> bool { - self.code == ErrorCode::TransientNotFound + pub fn is_not_found(&self) -> bool { + self.code == ErrorCode::NotFound } pub fn is_overloaded(&self) -> bool { @@ -358,7 +358,7 @@ impl ErrorMetadata { | ErrorCode::Forbidden => true, ErrorCode::OperationalInternalServerError | ErrorCode::ClientDisconnect - | ErrorCode::TransientNotFound + | ErrorCode::NotFound | ErrorCode::RateLimited | ErrorCode::OCC | ErrorCode::OutOfRetention @@ -377,7 +377,7 @@ impl ErrorMetadata { ErrorCode::ClientDisconnect => None, ErrorCode::RateLimited => Some((sentry::Level::Info, Some(0.01))), ErrorCode::BadRequest - | ErrorCode::TransientNotFound + | ErrorCode::NotFound | ErrorCode::PaginationLimit | ErrorCode::Unauthenticated | ErrorCode::Forbidden @@ -402,7 +402,7 @@ impl ErrorMetadata { | ErrorCode::ClientDisconnect | ErrorCode::MisdirectedRequest | ErrorCode::RateLimited => None, - ErrorCode::TransientNotFound => Some("transient_not_found"), + ErrorCode::NotFound => Some("not_found"), ErrorCode::OCC => Some("occ"), ErrorCode::OutOfRetention => Some("out_of_retention"), ErrorCode::Overloaded => Some("overloaded"), @@ -424,7 +424,7 @@ impl ErrorMetadata { ErrorCode::Unauthenticated => Some(&crate::metrics::SYNC_AUTH_ERROR_TOTAL), ErrorCode::Forbidden => Some(&crate::metrics::FORBIDDEN_ERROR_TOTAL), ErrorCode::OCC => Some(&crate::metrics::COMMIT_RACE_TOTAL), - ErrorCode::TransientNotFound => None, + ErrorCode::NotFound => None, ErrorCode::PaginationLimit => None, ErrorCode::OutOfRetention => None, ErrorCode::Overloaded => None, @@ -436,7 +436,7 @@ impl ErrorMetadata { pub fn close_frame(&self) -> Option> { let code = match self.code { - ErrorCode::TransientNotFound + ErrorCode::NotFound | ErrorCode::PaginationLimit | ErrorCode::Forbidden | ErrorCode::ClientDisconnect => Some(CloseCode::Normal), @@ -473,7 +473,7 @@ impl ErrorCode { // https://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses ErrorCode::Unauthenticated => StatusCode::UNAUTHORIZED, ErrorCode::Forbidden => StatusCode::FORBIDDEN, - ErrorCode::TransientNotFound => StatusCode::NOT_FOUND, + ErrorCode::NotFound => StatusCode::NOT_FOUND, ErrorCode::RateLimited => StatusCode::TOO_MANY_REQUESTS, ErrorCode::OperationalInternalServerError => StatusCode::INTERNAL_SERVER_ERROR, ErrorCode::OCC @@ -490,7 +490,7 @@ impl ErrorCode { ErrorCode::BadRequest => tonic::Code::InvalidArgument, ErrorCode::Unauthenticated => tonic::Code::Unauthenticated, ErrorCode::Forbidden => tonic::Code::FailedPrecondition, - ErrorCode::TransientNotFound => tonic::Code::NotFound, + ErrorCode::NotFound => tonic::Code::NotFound, ErrorCode::ClientDisconnect => tonic::Code::Aborted, ErrorCode::Overloaded | ErrorCode::RejectedBeforeExecution | ErrorCode::RateLimited => { tonic::Code::ResourceExhausted @@ -507,7 +507,7 @@ impl ErrorCode { match code { StatusCode::UNAUTHORIZED => Some(ErrorCode::Unauthenticated), StatusCode::FORBIDDEN => Some(ErrorCode::Forbidden), - StatusCode::NOT_FOUND => Some(ErrorCode::TransientNotFound), + StatusCode::NOT_FOUND => Some(ErrorCode::NotFound), StatusCode::TOO_MANY_REQUESTS => Some(ErrorCode::RateLimited), StatusCode::MISDIRECTED_REQUEST => Some(ErrorCode::MisdirectedRequest), // Tries to categorize in one of the above more specific 4xx codes first, @@ -525,7 +525,7 @@ pub trait ErrorMetadataAnyhowExt { fn is_unauthenticated(&self) -> bool; fn is_out_of_retention(&self) -> bool; fn is_bad_request(&self) -> bool; - fn is_transient_not_found(&self) -> bool; + fn is_not_found(&self) -> bool; fn is_overloaded(&self) -> bool; fn is_rejected_before_execution(&self) -> bool; fn is_forbidden(&self) -> bool; @@ -587,9 +587,9 @@ impl ErrorMetadataAnyhowExt for anyhow::Error { } /// Returns true if error is tagged as NotFound - fn is_transient_not_found(&self) -> bool { + fn is_not_found(&self) -> bool { if let Some(e) = self.downcast_ref::() { - return e.is_transient_not_found(); + return e.is_not_found(); } false } @@ -795,7 +795,7 @@ mod proptest { fn arbitrary_with((): Self::Parameters) -> Self::Strategy { any::().prop_map(|ec| match ec { ErrorCode::BadRequest => ErrorMetadata::bad_request("bad", "request"), - ErrorCode::TransientNotFound => ErrorMetadata::transient_not_found("not", "found"), + ErrorCode::NotFound => ErrorMetadata::not_found("not", "found"), ErrorCode::PaginationLimit => { ErrorMetadata::pagination_limit("pagination", "limit") }, @@ -839,7 +839,7 @@ mod tests { // Error has visibility through sentry or custom metric. assert!(err.should_report_to_sentry().is_some() || err.custom_metric().is_some()); if err.metric_server_error_label().is_some() - && err.code != ErrorCode::TransientNotFound { + && err.code != ErrorCode::NotFound { assert!(err.should_report_to_sentry().unwrap().0 >= sentry::Level::Warning); if err.code == ErrorCode::Overloaded || err.code == ErrorCode::RejectedBeforeExecution { diff --git a/crates/local_backend/src/schema.rs b/crates/local_backend/src/schema.rs index fc76af4c..579bd032 100644 --- a/crates/local_backend/src/schema.rs +++ b/crates/local_backend/src/schema.rs @@ -370,7 +370,7 @@ pub async fn schema_state( .context(invalid_schema_id(&schema_id))?; let doc = tx.get(schema_id).await?.ok_or_else(|| { - anyhow::anyhow!(ErrorMetadata::transient_not_found( + anyhow::anyhow!(ErrorMetadata::not_found( "SchemaNotFound", format!("Schema with id {} not found", schema_id), )) diff --git a/crates/model/src/components/config.rs b/crates/model/src/components/config.rs index 35301af0..243f3a8c 100644 --- a/crates/model/src/components/config.rs +++ b/crates/model/src/components/config.rs @@ -719,7 +719,7 @@ impl<'a, RT: Runtime> ComponentConfigModel<'a, RT> { .load_component(component_id) .await?; let Some(component) = component else { - anyhow::bail!(ErrorMetadata::transient_not_found( + anyhow::bail!(ErrorMetadata::not_found( "ComponentNotFound", format!("Component with ID {:?} not found", component_id) )); diff --git a/crates/model/src/environment_variables/mod.rs b/crates/model/src/environment_variables/mod.rs index b54dbbc6..5b875340 100644 --- a/crates/model/src/environment_variables/mod.rs +++ b/crates/model/src/environment_variables/mod.rs @@ -221,7 +221,7 @@ impl<'a, RT: Runtime> EnvironmentVariablesModel<'a, RT> { for (id, environment_variable) in changes.clone() { let new_env_var_name = environment_variable.name().to_owned(); let document = self.tx.get(id).await?.ok_or_else(|| { - ErrorMetadata::transient_not_found( + ErrorMetadata::not_found( "ModifiedEnvVarNotFound", "The modified environment variable couldn’t be found.", ) diff --git a/crates/model/src/snapshot_imports/mod.rs b/crates/model/src/snapshot_imports/mod.rs index d225dd3b..c6658b79 100644 --- a/crates/model/src/snapshot_imports/mod.rs +++ b/crates/model/src/snapshot_imports/mod.rs @@ -120,7 +120,7 @@ impl<'a, RT: Runtime> SnapshotImportModel<'a, RT> { let state = self .get(id) .await? - .context(ErrorMetadata::transient_not_found( + .context(ErrorMetadata::not_found( "ImportNotFound", format!("import {id} not found"), ))? @@ -162,13 +162,10 @@ impl<'a, RT: Runtime> SnapshotImportModel<'a, RT> { id: ResolvedDocumentId, update_checkpoints: impl FnOnce(&mut Vec), ) -> anyhow::Result<()> { - let mut import = self - .get(id) - .await? - .context(ErrorMetadata::transient_not_found( - "ImportNotFound", - format!("import {id} not found"), - ))?; + let mut import = self.get(id).await?.context(ErrorMetadata::not_found( + "ImportNotFound", + format!("import {id} not found"), + ))?; let mut checkpoints = import.checkpoints.clone().unwrap_or_default(); update_checkpoints(&mut checkpoints); import.checkpoints = Some(checkpoints); diff --git a/crates/pb/src/error_metadata.rs b/crates/pb/src/error_metadata.rs index 1fee9c41..64fd253f 100644 --- a/crates/pb/src/error_metadata.rs +++ b/crates/pb/src/error_metadata.rs @@ -19,7 +19,7 @@ impl From for ErrorCodeProto { ErrorCode::BadRequest => ErrorCodeProto::BadRequest, ErrorCode::Unauthenticated => ErrorCodeProto::Unauthenticated, ErrorCode::Forbidden => ErrorCodeProto::Forbidden, - ErrorCode::TransientNotFound => ErrorCodeProto::TransientNotFound, + ErrorCode::NotFound => ErrorCodeProto::TransientNotFound, ErrorCode::ClientDisconnect => ErrorCodeProto::ClientDisconnect, ErrorCode::RateLimited => ErrorCodeProto::RateLimited, ErrorCode::Overloaded => ErrorCodeProto::Overloaded, @@ -41,7 +41,7 @@ impl From for ErrorCode { ErrorCodeProto::BadRequest => ErrorCode::BadRequest, ErrorCodeProto::Unauthenticated => ErrorCode::Unauthenticated, ErrorCodeProto::Forbidden => ErrorCode::Forbidden, - ErrorCodeProto::TransientNotFound => ErrorCode::TransientNotFound, + ErrorCodeProto::TransientNotFound => ErrorCode::NotFound, ErrorCodeProto::ClientDisconnect => ErrorCode::ClientDisconnect, ErrorCodeProto::RateLimited => ErrorCode::RateLimited, ErrorCodeProto::Overloaded => ErrorCode::Overloaded, diff --git a/crates/runtime/src/prod.rs b/crates/runtime/src/prod.rs index c510ad0e..feb5f0f3 100644 --- a/crates/runtime/src/prod.rs +++ b/crates/runtime/src/prod.rs @@ -205,6 +205,18 @@ impl ProdRuntime { let monitor = GLOBAL_TASK_MANAGER.lock().get(name); self.rt.block_on(monitor.instrument(f)) } + + /// Transitional function while we move away from using our own special + /// `spawn`. Just wraps `tokio::spawn` with our tokio metrics + /// integration. + pub fn tokio_spawn(name: &'static str, f: F) -> tokio::task::JoinHandle + where + F: Future + Send + 'static, + F::Output: Send + 'static, + { + let monitor = GLOBAL_TASK_MANAGER.lock().get(name); + tokio::spawn(monitor.instrument(f)) + } } #[async_trait]