refactor(error): remove backtrace, and introduce call-site location for debugging (#1329)

* wip: global replace

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix compile

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix warnings

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* remove unneeded tests of errors

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix ErrorExt trait implementator

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix warnings

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix format

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* fix pyo3 tests

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

---------

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

src/common/error/src/ext.rs

@@ -23,10 +23,12 @@ pub trait ErrorExt: std::error::Error {
         StatusCode::Unknown
     }
 
-    /// Get the reference to the backtrace of this error, None if the backtrace is unavailable.
-    // Add `_opt` suffix to avoid confusing with similar method in `std::error::Error`, once backtrace
-    // in std is stable, we can deprecate this method.
-    fn backtrace_opt(&self) -> Option<&crate::snafu::Backtrace>;
+    // TODO(ruihang): remove this default implementation
+    /// Get the location of this error, None if the location is unavailable.
+    /// Add `_opt` suffix to avoid confusing with similar method in `std::error::Error`
+    fn location_opt(&self) -> Option<crate::snafu::Location> {
+        None
+    }
 
     /// Returns the error as [Any](std::any::Any) so that it can be
     /// downcast to a specific implementation.
@@ -71,8 +73,8 @@ impl crate::ext::ErrorExt for BoxedError {
         self.inner.status_code()
     }
 
-    fn backtrace_opt(&self) -> Option<&crate::snafu::Backtrace> {
-        self.inner.backtrace_opt()
+    fn location_opt(&self) -> Option<crate::snafu::Location> {
+        self.inner.location_opt()
     }
 
     fn as_any(&self) -> &dyn std::any::Any {
@@ -84,7 +86,7 @@ impl crate::ext::ErrorExt for BoxedError {
 // via `ErrorCompat::backtrace()`.
 impl crate::snafu::ErrorCompat for BoxedError {
     fn backtrace(&self) -> Option<&crate::snafu::Backtrace> {
-        self.inner.backtrace_opt()
+        None
     }
 }
 
@@ -118,7 +120,7 @@ impl crate::ext::ErrorExt for PlainError {
         self.status_code
     }
 
-    fn backtrace_opt(&self) -> Option<&crate::snafu::Backtrace> {
+    fn location_opt(&self) -> Option<crate::snafu::Location> {
         None
     }
 
@@ -126,62 +128,3 @@ impl crate::ext::ErrorExt for PlainError {
         self as _
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use std::error::Error;
-
-    use snafu::ErrorCompat;
-
-    use super::*;
-    use crate::format::DebugFormat;
-    use crate::mock::MockError;
-
-    #[test]
-    fn test_opaque_error_without_backtrace() {
-        let err = BoxedError::new(MockError::new(StatusCode::Internal));
-        assert!(err.backtrace_opt().is_none());
-        assert_eq!(StatusCode::Internal, err.status_code());
-        assert!(err.as_any().downcast_ref::<MockError>().is_some());
-        assert!(err.source().is_none());
-
-        assert!(ErrorCompat::backtrace(&err).is_none());
-    }
-
-    #[test]
-    fn test_opaque_error_with_backtrace() {
-        let err = BoxedError::new(MockError::with_backtrace(StatusCode::Internal));
-        assert!(err.backtrace_opt().is_some());
-        assert_eq!(StatusCode::Internal, err.status_code());
-        assert!(err.as_any().downcast_ref::<MockError>().is_some());
-        assert!(err.source().is_none());
-
-        assert!(ErrorCompat::backtrace(&err).is_some());
-
-        let msg = format!("{err:?}");
-        assert!(msg.contains("\nBacktrace:\n"));
-        let fmt_msg = format!("{:?}", DebugFormat::new(&err));
-        assert_eq!(msg, fmt_msg);
-
-        let msg = err.to_string();
-        msg.contains("Internal");
-    }
-
-    #[test]
-    fn test_opaque_error_with_source() {
-        let leaf_err = MockError::with_backtrace(StatusCode::Internal);
-        let internal_err = MockError::with_source(leaf_err);
-        let err = BoxedError::new(internal_err);
-
-        assert!(err.backtrace_opt().is_some());
-        assert_eq!(StatusCode::Internal, err.status_code());
-        assert!(err.as_any().downcast_ref::<MockError>().is_some());
-        assert!(err.source().is_some());
-
-        let msg = format!("{err:?}");
-        assert!(msg.contains("\nBacktrace:\n"));
-        assert!(msg.contains("Caused by"));
-
-        assert!(ErrorCompat::backtrace(&err).is_some());
-    }
-}

src/common/error/src/format.rs

@@ -33,9 +33,9 @@ impl<'a, E: ErrorExt + ?Sized> fmt::Debug for DebugFormat<'a, E> {
             // Source error use debug format for more verbose info.
             write!(f, " Caused by: {source:?}")?;
         }
-        if let Some(backtrace) = self.0.backtrace_opt() {
+        if let Some(location) = self.0.location_opt() {
             // Add a newline to separate causes and backtrace.
-            write!(f, "\nBacktrace:\n{backtrace}")?;
+            write!(f, " at: {location}")?;
         }
 
         Ok(())
@@ -47,7 +47,7 @@ mod tests {
     use std::any::Any;
 
     use snafu::prelude::*;
-    use snafu::{Backtrace, GenerateImplicitData};
+    use snafu::{GenerateImplicitData, Location};
 
     use super::*;
 
@@ -56,7 +56,7 @@ mod tests {
     struct Leaf;
 
     impl ErrorExt for Leaf {
-        fn backtrace_opt(&self) -> Option<&Backtrace> {
+        fn location_opt(&self) -> Option<Location> {
             None
         }
 
@@ -66,14 +66,14 @@ mod tests {
     }
 
     #[derive(Debug, Snafu)]
-    #[snafu(display("This is a leaf with backtrace"))]
-    struct LeafWithBacktrace {
-        backtrace: Backtrace,
+    #[snafu(display("This is a leaf with location"))]
+    struct LeafWithLocation {
+        location: Location,
     }
 
-    impl ErrorExt for LeafWithBacktrace {
-        fn backtrace_opt(&self) -> Option<&Backtrace> {
-            Some(&self.backtrace)
+    impl ErrorExt for LeafWithLocation {
+        fn location_opt(&self) -> Option<Location> {
+            None
         }
 
         fn as_any(&self) -> &dyn Any {
@@ -86,12 +86,12 @@ mod tests {
     struct Internal {
         #[snafu(source)]
         source: Leaf,
-        backtrace: Backtrace,
+        location: Location,
     }
 
     impl ErrorExt for Internal {
-        fn backtrace_opt(&self) -> Option<&Backtrace> {
-            Some(&self.backtrace)
+        fn location_opt(&self) -> Option<Location> {
+            None
         }
 
         fn as_any(&self) -> &dyn Any {
@@ -106,19 +106,21 @@ mod tests {
         let msg = format!("{:?}", DebugFormat::new(&err));
         assert_eq!("This is a leaf error.", msg);
 
-        let err = LeafWithBacktrace {
-            backtrace: Backtrace::generate(),
+        let err = LeafWithLocation {
+            location: Location::generate(),
         };
 
+        // TODO(ruihang): display location here
         let msg = format!("{:?}", DebugFormat::new(&err));
-        assert!(msg.starts_with("This is a leaf with backtrace.\nBacktrace:\n"));
+        assert!(msg.starts_with("This is a leaf with location."));
 
         let err = Internal {
             source: Leaf,
-            backtrace: Backtrace::generate(),
+            location: Location::generate(),
         };
 
+        // TODO(ruihang): display location here
         let msg = format!("{:?}", DebugFormat::new(&err));
-        assert!(msg.contains("Internal error. Caused by: Leaf\nBacktrace:\n"));
+        assert!(msg.contains("Internal error. Caused by: Leaf"));
     }
 }

src/common/error/src/mock.rs

@@ -17,7 +17,7 @@
 use std::any::Any;
 use std::fmt;
 
-use snafu::GenerateImplicitData;
+use snafu::Location;
 
 use crate::prelude::*;
 
@@ -25,34 +25,19 @@ use crate::prelude::*;
 #[derive(Debug)]
 pub struct MockError {
     pub code: StatusCode,
-    backtrace: Option<Backtrace>,
     source: Option<Box<MockError>>,
 }
 
 impl MockError {
     /// Create a new [MockError] without backtrace.
     pub fn new(code: StatusCode) -> MockError {
-        MockError {
-            code,
-            backtrace: None,
-            source: None,
-        }
-    }
-
-    /// Create a new [MockError] with backtrace.
-    pub fn with_backtrace(code: StatusCode) -> MockError {
-        MockError {
-            code,
-            backtrace: Some(Backtrace::generate()),
-            source: None,
-        }
+        MockError { code, source: None }
     }
 
     /// Create a new [MockError] with source.
     pub fn with_source(source: MockError) -> MockError {
         MockError {
             code: source.code,
-            backtrace: None,
             source: Some(Box::new(source)),
         }
     }
@@ -75,39 +60,11 @@ impl ErrorExt for MockError {
         self.code
     }
 
-    fn backtrace_opt(&self) -> Option<&Backtrace> {
-        self.backtrace
-            .as_ref()
-            .or_else(|| self.source.as_ref().and_then(|err| err.backtrace_opt()))
+    fn location_opt(&self) -> Option<Location> {
+        None
     }
 
     fn as_any(&self) -> &dyn Any {
         self
     }
 }
-
-impl ErrorCompat for MockError {
-    fn backtrace(&self) -> Option<&Backtrace> {
-        self.backtrace_opt()
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use std::error::Error;
-
-    use super::*;
-
-    #[test]
-    fn test_mock_error() {
-        let err = MockError::new(StatusCode::Unknown);
-        assert!(err.backtrace_opt().is_none());
-
-        let err = MockError::with_backtrace(StatusCode::Unknown);
-        assert!(err.backtrace_opt().is_some());
-
-        let root_err = MockError::with_source(err);
-        assert!(root_err.source().is_some());
-        assert!(root_err.backtrace_opt().is_some());
-    }
-}