refactor!: diagnostic visit is now fallible (#121)

tisonkun · web-flow · commit 3bc2be6d8f05 · 2025-04-08T00:07:13.000+08:00
Signed-off-by: tison &lt;wander4096@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
 
 * `Diagnostic` is now a trait. `Visitor`'s method signature is simplified.
 * `Append::flush` is now fallible.
+* `Diagnostic`'s and `Visitor`'s `visit` methods are fallible.
 
 ## [0.23.1] 2025-03-23
 
diff --git a/src/append/fastrace.rs b/src/append/fastrace.rs
@@ -44,7 +44,7 @@ impl Append for FastraceEvent {
         let mut collector = KvCollector { kv: Vec::new() };
         record.key_values().visit(&mut collector)?;
         for d in diagnostics {
-            d.visit(&mut collector);
+            d.visit(&mut collector)?;
         }
 
         fastrace::local::LocalSpan::add_event(fastrace::Event::new(message).with_properties(
@@ -88,9 +88,8 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvCollector {
 }
 
 impl Visitor for KvCollector {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
-        let key = key.into_owned();
-        let value = value.into_owned();
-        self.kv.push((key, value));
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
+        self.kv.push((key.into_owned(), value.into_owned()));
+        Ok(())
     }
 }
diff --git a/src/append/journald/mod.rs b/src/append/journald/mod.rs
@@ -247,10 +247,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for WriteKeyValues<'_> {
 }
 
 impl Visitor for WriteKeyValues<'_> {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
         let key = key.as_ref();
         let value = value.as_bytes();
         field::put_field_length_encoded(self.0, field::FieldName::WriteEscaped(key), value);
+        Ok(())
     }
 }
 
@@ -310,7 +311,7 @@ impl Append for Journald {
         let mut visitor = WriteKeyValues(&mut buffer);
         record.key_values().visit(&mut visitor)?;
         for d in diagnostics {
-            d.visit(&mut visitor);
+            d.visit(&mut visitor)?;
         }
         // Put all extra fields of the appender
         buffer.extend_from_slice(&self.extra_fields);
diff --git a/src/append/opentelemetry.rs b/src/append/opentelemetry.rs
@@ -259,7 +259,7 @@ impl Append for OpentelemetryLog {
         };
         record.key_values().visit(&mut extractor)?;
         for d in diagnostics {
-            d.visit(&mut extractor);
+            d.visit(&mut extractor)?;
         }
 
         self.logger.emit(log_record);
@@ -300,9 +300,10 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvExtractor<'_> {
 }
 
 impl Visitor for KvExtractor<'_> {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
         let key = key.into_owned();
         let value = value.into_owned();
         self.record.add_attribute(key, value);
+        Ok(())
     }
 }
diff --git a/src/diagnostic/fastrace.rs b/src/diagnostic/fastrace.rs
@@ -40,10 +40,12 @@ use crate::Diagnostic;
 pub struct FastraceDiagnostic {}
 
 impl Diagnostic for FastraceDiagnostic {
-    fn visit(&self, visitor: &mut dyn Visitor) {
+    fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {
         if let Some(span) = fastrace::collector::SpanContext::current_local_parent() {
             let trace_id = format!("{:016x}", span.trace_id.0);
-            visitor.visit("trace_id".into(), trace_id.into());
+            visitor.visit("trace_id".into(), trace_id.into())?;
         }
+
+        Ok(())
     }
 }
diff --git a/src/diagnostic/mod.rs b/src/diagnostic/mod.rs
@@ -31,12 +31,12 @@ mod thread_local;
 /// A visitor to walk through diagnostic key-value pairs.
 pub trait Visitor {
     /// Visits a key-value pair.
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>);
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()>;
 }
 
 /// A trait representing a Mapped Diagnostic Context (MDC) that provides diagnostic key-values.
 pub trait Diagnostic: fmt::Debug + Send + Sync + 'static {
-    fn visit(&self, visitor: &mut dyn Visitor);
+    fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()>;
 }
 
 impl<T: Diagnostic> From<T> for Box<dyn Diagnostic> {
diff --git a/src/diagnostic/static_global.rs b/src/diagnostic/static_global.rs
@@ -52,9 +52,11 @@ impl StaticDiagnostic {
 }
 
 impl Diagnostic for StaticDiagnostic {
-    fn visit(&self, visitor: &mut dyn Visitor) {
+    fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {
         for (key, value) in self.kvs.iter() {
-            visitor.visit(key.into(), value.into());
+            visitor.visit(key.into(), value.into())?;
         }
+
+        Ok(())
     }
 }
diff --git a/src/diagnostic/thread_local.rs b/src/diagnostic/thread_local.rs
@@ -54,12 +54,13 @@ impl ThreadLocalDiagnostic {
 }
 
 impl Diagnostic for ThreadLocalDiagnostic {
-    fn visit(&self, visitor: &mut dyn Visitor) {
+    fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {
         CONTEXT.with(|map| {
             let map = map.borrow();
             for (key, value) in map.iter() {
-                visitor.visit(key.into(), value.into());
+                visitor.visit(key.into(), value.into())?;
             }
+            Ok(())
         })
     }
 }
diff --git a/src/layout/json.rs b/src/layout/json.rs
@@ -86,10 +86,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvCollector<'_> {
 }
 
 impl Visitor for KvCollector<'_> {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
         let key = key.into_owned();
         let value = value.into_owned();
         self.kvs.insert(key, value.into());
+        Ok(())
     }
 }
 
@@ -130,7 +131,7 @@ impl Layout for JsonLayout {
         let mut visitor = KvCollector { kvs: &mut kvs };
         record.key_values().visit(&mut visitor)?;
         for d in diagnostics {
-            d.visit(&mut visitor);
+            d.visit(&mut visitor)?;
         }
 
         let record_line = RecordLine {
diff --git a/src/layout/logfmt.rs b/src/layout/logfmt.rs
@@ -70,20 +70,21 @@ impl LogfmtLayout {
 }
 
 // The encode logic is copied from https://github.com/go-logfmt/logfmt/blob/76262ea7/encode.go.
-fn encode_key_value(result: &mut String, key: &str, value: &str) {
+fn encode_key_value(result: &mut String, key: &str, value: &str) -> anyhow::Result<()> {
     use std::fmt::Write;
 
     if key.contains([' ', '=', '"']) {
         // omit keys contain special chars
-        return;
+        anyhow::bail!("key contains special chars: {key}");
     }
 
-    // SAFETY: extend a string is always possible
     if value.contains([' ', '=', '"']) {
-        write!(result, " {key}=\"{}\"", value.escape_debug()).unwrap();
+        write!(result, " {key}=\"{}\"", value.escape_debug())?;
     } else {
-        write!(result, " {key}={value}").unwrap();
+        write!(result, " {key}={value}")?;
     }
+
+    Ok(())
 }
 
 struct KvFormatter {
@@ -96,14 +97,17 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvFormatter {
         key: log::kv::Key<'kvs>,
         value: log::kv::Value<'kvs>,
     ) -> Result<(), log::kv::Error> {
-        encode_key_value(&mut self.text, key.as_str(), value.to_string().as_str());
-        Ok(())
+        match encode_key_value(&mut self.text, key.as_str(), value.to_string().as_str()) {
+            Ok(()) => Ok(()),
+            Err(err) => Err(log::kv::Error::boxed(err)),
+        }
     }
 }
 
 impl Visitor for KvFormatter {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
-        encode_key_value(&mut self.text, key.as_ref(), value.as_ref());
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
+        encode_key_value(&mut self.text, key.as_ref(), value.as_ref())?;
+        Ok(())
     }
 }
 
@@ -127,17 +131,17 @@ impl Layout for LogfmtLayout {
             text: format!("timestamp={time:.6}"),
         };
 
-        visitor.visit(Cow::Borrowed("level"), level.into());
-        visitor.visit(Cow::Borrowed("module"), target.into());
+        visitor.visit(Cow::Borrowed("level"), level.into())?;
+        visitor.visit(Cow::Borrowed("module"), target.into())?;
         visitor.visit(
             Cow::Borrowed("position"),
             format!("{}:{}", file, line).into(),
-        );
-        visitor.visit(Cow::Borrowed("message"), message.to_string().into());
+        )?;
+        visitor.visit(Cow::Borrowed("message"), message.to_string().into())?;
 
         record.key_values().visit(&mut visitor)?;
         for d in diagnostics {
-            d.visit(&mut visitor);
+            d.visit(&mut visitor)?;
         }
 
         Ok(visitor.text.into_bytes())
diff --git a/src/layout/text.rs b/src/layout/text.rs
@@ -171,12 +171,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvWriter {
 }
 
 impl Visitor for KvWriter {
-    fn visit(&mut self, key: Cow<str>, value: Cow<str>) {
+    fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {
         use std::fmt::Write;
 
-        // SAFETY: no more than an allocate-less version
-        //  self.text.push_str(&format!(" {key}={value}"))
-        write!(&mut self.text, " {key}={value}").unwrap();
+        write!(&mut self.text, " {key}={value}")?;
+        Ok(())
     }
 }
 
@@ -201,7 +200,7 @@ impl Layout for TextLayout {
         };
         record.key_values().visit(&mut visitor)?;
         for d in diagnostics {
-            d.visit(&mut visitor);
+            d.visit(&mut visitor)?;
         }
 
         Ok(visitor.text.into_bytes())

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ impl Append for FastraceEvent {`
`44`	`44`	`let mut collector = KvCollector { kv: Vec::new() };`
`45`	`45`	`record.key_values().visit(&mut collector)?;`
`46`	`46`	`for d in diagnostics {`
`47`		`- d.visit(&mut collector);`
	`47`	`+ d.visit(&mut collector)?;`
`48`	`48`	`}`
`49`	`49`
`50`	`50`	`fastrace::local::LocalSpan::add_event(fastrace::Event::new(message).with_properties(`
`@@ -88,9 +88,8 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvCollector {`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`impl Visitor for KvCollector {`
`91`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>) {`
`92`		`- let key = key.into_owned();`
`93`		`- let value = value.into_owned();`
`94`		`- self.kv.push((key, value));`
	`91`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {`
	`92`	`+ self.kv.push((key.into_owned(), value.into_owned()));`
	`93`	`+ Ok(())`
`95`	`94`	`}`
`96`	`95`	`}`
Original file line number	Diff line number	Diff line change
`@@ -247,10 +247,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for WriteKeyValues<'_> {`
`247`	`247`	`}`
`248`	`248`
`249`	`249`	`impl Visitor for WriteKeyValues<'_> {`
`250`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>) {`
	`250`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {`
`251`	`251`	`let key = key.as_ref();`
`252`	`252`	`let value = value.as_bytes();`
`253`	`253`	`field::put_field_length_encoded(self.0, field::FieldName::WriteEscaped(key), value);`
	`254`	`+ Ok(())`
`254`	`255`	`}`
`255`	`256`	`}`
`256`	`257`
`@@ -310,7 +311,7 @@ impl Append for Journald {`
`310`	`311`	`let mut visitor = WriteKeyValues(&mut buffer);`
`311`	`312`	`record.key_values().visit(&mut visitor)?;`
`312`	`313`	`for d in diagnostics {`
`313`		`- d.visit(&mut visitor);`
	`314`	`+ d.visit(&mut visitor)?;`
`314`	`315`	`}`
`315`	`316`	`// Put all extra fields of the appender`
`316`	`317`	`buffer.extend_from_slice(&self.extra_fields);`
Original file line number	Diff line number	Diff line change
`@@ -259,7 +259,7 @@ impl Append for OpentelemetryLog {`
`259`	`259`	`};`
`260`	`260`	`record.key_values().visit(&mut extractor)?;`
`261`	`261`	`for d in diagnostics {`
`262`		`- d.visit(&mut extractor);`
	`262`	`+ d.visit(&mut extractor)?;`
`263`	`263`	`}`
`264`	`264`
`265`	`265`	`self.logger.emit(log_record);`
`@@ -300,9 +300,10 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvExtractor<'_> {`
`300`	`300`	`}`
`301`	`301`
`302`	`302`	`impl Visitor for KvExtractor<'_> {`
`303`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>) {`
	`303`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {`
`304`	`304`	`let key = key.into_owned();`
`305`	`305`	`let value = value.into_owned();`
`306`	`306`	`self.record.add_attribute(key, value);`
	`307`	`+ Ok(())`
`307`	`308`	`}`
`308`	`309`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,10 +40,12 @@ use crate::Diagnostic;`
`40`	`40`	`pub struct FastraceDiagnostic {}`
`41`	`41`
`42`	`42`	`impl Diagnostic for FastraceDiagnostic {`
`43`		`- fn visit(&self, visitor: &mut dyn Visitor) {`
	`43`	`+ fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {`
`44`	`44`	`if let Some(span) = fastrace::collector::SpanContext::current_local_parent() {`
`45`	`45`	`let trace_id = format!("{:016x}", span.trace_id.0);`
`46`		`- visitor.visit("trace_id".into(), trace_id.into());`
	`46`	`+ visitor.visit("trace_id".into(), trace_id.into())?;`
`47`	`47`	`}`
	`48`	`+`
	`49`	`+ Ok(())`
`48`	`50`	`}`
`49`	`51`	`}`
Original file line number	Diff line number	Diff line change
`@@ -31,12 +31,12 @@ mod thread_local;`
`31`	`31`	`/// A visitor to walk through diagnostic key-value pairs.`
`32`	`32`	`pub trait Visitor {`
`33`	`33`	`/// Visits a key-value pair.`
`34`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>);`
	`34`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()>;`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`/// A trait representing a Mapped Diagnostic Context (MDC) that provides diagnostic key-values.`
`38`	`38`	`pub trait Diagnostic: fmt::Debug + Send + Sync + 'static {`
`39`		`- fn visit(&self, visitor: &mut dyn Visitor);`
	`39`	`+ fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()>;`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`impl<T: Diagnostic> From<T> for Box<dyn Diagnostic> {`
Original file line number	Diff line number	Diff line change
`@@ -52,9 +52,11 @@ impl StaticDiagnostic {`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`impl Diagnostic for StaticDiagnostic {`
`55`		`- fn visit(&self, visitor: &mut dyn Visitor) {`
	`55`	`+ fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {`
`56`	`56`	`for (key, value) in self.kvs.iter() {`
`57`		`- visitor.visit(key.into(), value.into());`
	`57`	`+ visitor.visit(key.into(), value.into())?;`
`58`	`58`	`}`
	`59`	`+`
	`60`	`+ Ok(())`
`59`	`61`	`}`
`60`	`62`	`}`
Original file line number	Diff line number	Diff line change
`@@ -54,12 +54,13 @@ impl ThreadLocalDiagnostic {`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`impl Diagnostic for ThreadLocalDiagnostic {`
`57`		`- fn visit(&self, visitor: &mut dyn Visitor) {`
	`57`	`+ fn visit(&self, visitor: &mut dyn Visitor) -> anyhow::Result<()> {`
`58`	`58`	`CONTEXT.with(\|map\| {`
`59`	`59`	`let map = map.borrow();`
`60`	`60`	`for (key, value) in map.iter() {`
`61`		`- visitor.visit(key.into(), value.into());`
	`61`	`+ visitor.visit(key.into(), value.into())?;`
`62`	`62`	`}`
	`63`	`+ Ok(())`
`63`	`64`	`})`
`64`	`65`	`}`
`65`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -86,10 +86,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvCollector<'_> {`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`impl Visitor for KvCollector<'_> {`
`89`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>) {`
	`89`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {`
`90`	`90`	`let key = key.into_owned();`
`91`	`91`	`let value = value.into_owned();`
`92`	`92`	`self.kvs.insert(key, value.into());`
	`93`	`+ Ok(())`
`93`	`94`	`}`
`94`	`95`	`}`
`95`	`96`
`@@ -130,7 +131,7 @@ impl Layout for JsonLayout {`
`130`	`131`	`let mut visitor = KvCollector { kvs: &mut kvs };`
`131`	`132`	`record.key_values().visit(&mut visitor)?;`
`132`	`133`	`for d in diagnostics {`
`133`		`- d.visit(&mut visitor);`
	`134`	`+ d.visit(&mut visitor)?;`
`134`	`135`	`}`
`135`	`136`
`136`	`137`	`let record_line = RecordLine {`
Original file line number	Diff line number	Diff line change
`@@ -171,12 +171,11 @@ impl<'kvs> log::kv::VisitSource<'kvs> for KvWriter {`
`171`	`171`	`}`
`172`	`172`
`173`	`173`	`impl Visitor for KvWriter {`
`174`		`- fn visit(&mut self, key: Cow<str>, value: Cow<str>) {`
	`174`	`+ fn visit(&mut self, key: Cow<str>, value: Cow<str>) -> anyhow::Result<()> {`
`175`	`175`	`use std::fmt::Write;`
`176`	`176`
`177`		`- // SAFETY: no more than an allocate-less version`
`178`		`- // self.text.push_str(&format!(" {key}={value}"))`
`179`		`- write!(&mut self.text, " {key}={value}").unwrap();`
	`177`	`+ write!(&mut self.text, " {key}={value}")?;`
	`178`	`+ Ok(())`
`180`	`179`	`}`
`181`	`180`	`}`
`182`	`181`
`@@ -201,7 +200,7 @@ impl Layout for TextLayout {`
`201`	`200`	`};`
`202`	`201`	`record.key_values().visit(&mut visitor)?;`
`203`	`202`	`for d in diagnostics {`
`204`		`- d.visit(&mut visitor);`
	`203`	`+ d.visit(&mut visitor)?;`
`205`	`204`	`}`
`206`	`205`
`207`	`206`	`Ok(visitor.text.into_bytes())`