review comments

Author: lalitb

opentelemetry-appender-log/src/lib.rs

@@ -130,7 +130,7 @@ where
             log_record.set_severity_number(severity_of_level(record.level()));
             log_record.set_severity_text(record.level().as_str().into());
             log_record.set_body(AnyValue::from(record.args().to_string()));
-            log_record.set_attributes(log_attributes(record.key_values()));
+            log_record.add_attributes(log_attributes(record.key_values()));
 
             self.logger.emit(log_record);
         }

opentelemetry-appender-tracing/src/layer.rs

@@ -75,7 +75,7 @@ impl EventVisitor {
         if let Some(body) = self.log_record_body {
             log_record.set_body(body);
         }
-        log_record.set_attributes(self.log_record_attributes);
+        log_record.add_attributes(self.log_record_attributes);
     }
 }
 

opentelemetry-sdk/CHANGELOG.md

@@ -26,6 +26,8 @@
   - `shutdown` methods in `LoggerProvider` and `LogProcessor` now takes a immutable reference
   - After `shutdown`, `LoggerProvider` will return noop `Logger`
   - After `shutdown`, `LogProcessor` will not process any new logs
+- Moving LogRecord implementation to the SDK. [1702](https://github.com/open-telemetry/opentelemetry-rust/pull/1702).
+    - Relocated `LogRecord` struct to SDK, as an implementation for the trait in the API.
 
 ## v0.22.1
 

opentelemetry-sdk/benches/log.rs

@@ -69,36 +69,36 @@ fn criterion_benchmark(c: &mut Criterion) {
     log_benchmark_group(c, "simple-log-with-int", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testint", 2);
+        log_record.add_attribute("testint", 2);
         logger.emit(log_record);
     });
 
     log_benchmark_group(c, "simple-log-with-double", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testdouble", 2.2);
+        log_record.add_attribute("testdouble", 2.2);
         logger.emit(log_record);
     });
 
     log_benchmark_group(c, "simple-log-with-string", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("teststring", "test");
+        log_record.add_attribute("teststring", "test");
         logger.emit(log_record);
     });
 
     log_benchmark_group(c, "simple-log-with-bool", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testbool", AnyValue::Boolean(true));
+        log_record.add_attribute("testbool", AnyValue::Boolean(true));
         logger.emit(log_record);
     });
 
     let bytes = AnyValue::Bytes(vec![25u8, 30u8, 40u8]);
     log_benchmark_group(c, "simple-log-with-bytes", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testbytes", bytes.clone());
+        log_record.add_attribute("testbytes", bytes.clone());
         logger.emit(log_record);
     });
 
@@ -113,15 +113,15 @@ fn criterion_benchmark(c: &mut Criterion) {
     log_benchmark_group(c, "simple-log-with-a-lot-of-bytes", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testbytes", bytes.clone());
+        log_record.add_attribute("testbytes", bytes.clone());
         logger.emit(log_record);
     });
 
     let vec_any_values = AnyValue::ListAny(vec![AnyValue::Int(25), "test".into(), true.into()]);
     log_benchmark_group(c, "simple-log-with-vec-any-value", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testvec", vec_any_values.clone());
+        log_record.add_attribute("testvec", vec_any_values.clone());
         logger.emit(log_record);
     });
 
@@ -135,7 +135,7 @@ fn criterion_benchmark(c: &mut Criterion) {
     log_benchmark_group(c, "simple-log-with-inner-vec-any-value", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testvec", vec_any_values.clone());
+        log_record.add_attribute("testvec", vec_any_values.clone());
         logger.emit(log_record);
     });
 
@@ -147,7 +147,7 @@ fn criterion_benchmark(c: &mut Criterion) {
     log_benchmark_group(c, "simple-log-with-map-any-value", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testmap", map_any_values.clone());
+        log_record.add_attribute("testmap", map_any_values.clone());
         logger.emit(log_record);
     });
 
@@ -165,7 +165,7 @@ fn criterion_benchmark(c: &mut Criterion) {
     log_benchmark_group(c, "simple-log-with-inner-map-any-value", |logger| {
         let mut log_record = logger.create_log_record();
         log_record.set_body("simple log".into());
-        log_record.set_attribute("testmap", map_any_values.clone());
+        log_record.add_attribute("testmap", map_any_values.clone());
         logger.emit(log_record);
     });
 
@@ -193,10 +193,10 @@ fn criterion_benchmark(c: &mut Criterion) {
         log_record.set_observed_timestamp(now);
         log_record.set_severity_number(Severity::Warn);
         log_record.set_severity_text(Severity::Warn.name().into());
-        log_record.set_attribute("name", "my-event-name");
-        log_record.set_attribute("event.id", 20);
-        log_record.set_attribute("user.name", "otel");
-        log_record.set_attribute("user.email", "otel@opentelemetry.io");
+        log_record.add_attribute("name", "my-event-name");
+        log_record.add_attribute("event.id", 20);
+        log_record.add_attribute("user.name", "otel");
+        log_record.add_attribute("user.email", "otel@opentelemetry.io");
         logger.emit(log_record);
     });
 
@@ -207,15 +207,15 @@ fn criterion_benchmark(c: &mut Criterion) {
         log_record.set_observed_timestamp(now);
         log_record.set_severity_number(Severity::Warn);
         log_record.set_severity_text(Severity::Warn.name().into());
-        log_record.set_attribute("name", "my-event-name");
-        log_record.set_attribute("event.id", 20);
-        log_record.set_attribute("user.name", "otel");
-        log_record.set_attribute("user.email", "otel@opentelemetry.io");
-        log_record.set_attribute("code.filename", "log.rs");
-        log_record.set_attribute("code.filepath", "opentelemetry_sdk/benches/log.rs");
-        log_record.set_attribute("code.lineno", 96);
-        log_record.set_attribute("code.namespace", "opentelemetry_sdk::benches::log");
-        log_record.set_attribute("log.target", "opentelemetry_sdk::benches::log");
+        log_record.add_attribute("name", "my-event-name");
+        log_record.add_attribute("event.id", 20);
+        log_record.add_attribute("user.name", "otel");
+        log_record.add_attribute("user.email", "otel@opentelemetry.io");
+        log_record.add_attribute("code.filename", "log.rs");
+        log_record.add_attribute("code.filepath", "opentelemetry_sdk/benches/log.rs");
+        log_record.add_attribute("code.lineno", 96);
+        log_record.add_attribute("code.namespace", "opentelemetry_sdk::benches::log");
+        log_record.add_attribute("log.target", "opentelemetry_sdk::benches::log");
         logger.emit(log_record);
     });
 
@@ -246,7 +246,7 @@ fn criterion_benchmark(c: &mut Criterion) {
         log_record.set_observed_timestamp(now);
         log_record.set_severity_number(Severity::Warn);
         log_record.set_severity_text(Severity::Warn.name().into());
-        log_record.set_attributes(attributes.clone());
+        log_record.add_attributes(attributes.clone());
         logger.emit(log_record);
     });
 }

opentelemetry-sdk/src/logs/mod.rs

@@ -34,7 +34,7 @@ mod tests {
         let mut log_record = logger.create_log_record();
         log_record.set_severity_number(Severity::Error);
         log_record.set_severity_text("Error".into());
-        log_record.set_attributes(vec![
+        log_record.add_attributes(vec![
             (Key::new("key1"), "value1".into()),
             (Key::new("key2"), "value2".into()),
         ]);

opentelemetry-sdk/src/logs/record.rs

@@ -40,14 +40,6 @@ impl opentelemetry::logs::LogRecord for LogRecord {
         self.observed_timestamp = Some(timestamp);
     }
 
-    fn set_span_context(&mut self, span_context: &SpanContext) {
-        self.trace_context = Some(TraceContext {
-            span_id: span_context.span_id(),
-            trace_id: span_context.trace_id(),
-            trace_flags: Some(span_context.trace_flags()),
-        });
-    }
-
     fn set_severity_text(&mut self, severity_text: Cow<'static, str>) {
         self.severity_text = Some(severity_text);
     }
@@ -60,11 +52,11 @@ impl opentelemetry::logs::LogRecord for LogRecord {
         self.body = Some(body);
     }
 
-    fn set_attributes(&mut self, attributes: Vec<(Key, AnyValue)>) {
+    fn add_attributes(&mut self, attributes: Vec<(Key, AnyValue)>) {
         self.attributes = Some(attributes);
     }
 
-    fn set_attribute<K, V>(&mut self, key: K, value: V)
+    fn add_attribute<K, V>(&mut self, key: K, value: V)
     where
         K: Into<Key>,
         V: Into<AnyValue>,
@@ -104,20 +96,9 @@ impl From<&SpanContext> for TraceContext {
 mod tests {
     use super::*;
     use opentelemetry::logs::{AnyValue, LogRecord as _, Severity};
-    use opentelemetry::trace::{SpanContext, SpanId, TraceFlags, TraceId};
     use std::borrow::Cow;
     use std::time::SystemTime;
 
-    // Helper function to create a TraceId from a u128 number
-    fn trace_id_from_u128(num: u128) -> TraceId {
-        TraceId::from_bytes(num.to_be_bytes())
-    }
-
-    // Helper function to create a SpanId from a u64 number
-    fn span_id_from_u64(num: u64) -> SpanId {
-        SpanId::from_bytes(num.to_be_bytes())
-    }
-
     #[test]
     fn test_set_timestamp() {
         let mut log_record = LogRecord::default();
@@ -134,31 +115,6 @@ mod tests {
         assert_eq!(log_record.observed_timestamp, Some(now));
     }
 
-    #[test]
-    fn test_set_span_context() {
-        let mut log_record = LogRecord::default();
-        let span_context = SpanContext::new(
-            trace_id_from_u128(123),
-            span_id_from_u64(456),
-            TraceFlags::default(),
-            true,
-            Default::default(),
-        );
-        log_record.set_span_context(&span_context);
-        assert_eq!(
-            log_record.trace_context.clone().unwrap().trace_id,
-            span_context.trace_id()
-        );
-        assert_eq!(
-            log_record.trace_context.clone().unwrap().span_id,
-            span_context.span_id()
-        );
-        assert_eq!(
-            log_record.trace_context.unwrap().trace_flags,
-            Some(span_context.trace_flags())
-        );
-    }
-
     #[test]
     fn test_set_severity_text() {
         let mut log_record = LogRecord::default();
@@ -187,14 +143,14 @@ mod tests {
     fn test_set_attributes() {
         let mut log_record = LogRecord::default();
         let attributes = vec![(Key::new("key"), AnyValue::String("value".into()))];
-        log_record.set_attributes(attributes.clone());
+        log_record.add_attributes(attributes.clone());
         assert_eq!(log_record.attributes, Some(attributes));
     }
 
     #[test]
     fn test_set_attribute() {
         let mut log_record = LogRecord::default();
-        log_record.set_attribute("key", "value");
+        log_record.add_attribute("key", "value");
         assert_eq!(
             log_record.attributes,
             Some(vec![(Key::new("key"), AnyValue::String("value".into()))])

opentelemetry/CHANGELOG.md

@@ -25,6 +25,10 @@
 ### Changed
 
 - Deprecate `versioned_logger()` in favor of `logger_builder()` [1567](https://github.com/open-telemetry/opentelemetry-rust/pull/1567).
+- **BREAKING** Moving LogRecord implementation to the SDK. [1702](https://github.com/open-telemetry/opentelemetry-rust/pull/1702).
+    - Relocated `LogRecord` struct to SDK.
+    - Introduced the `LogRecord` trait in the API for populating log records. This trait is implemented by the SDK.
+    This is the breaking change for the authors of Log Appenders. Refer to the [opentelemetry-appender-tracing](https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-appender-tracing) for more details.
 
 Before:
 

opentelemetry/src/logs/noop.rs

@@ -2,7 +2,6 @@ use std::{borrow::Cow, sync::Arc, time::SystemTime};
 
 use crate::{
     logs::{AnyValue, LogRecord, Logger, LoggerProvider, Severity},
-    trace::SpanContext,
     InstrumentationLibrary, Key, KeyValue,
 };
 
@@ -46,28 +45,20 @@ impl LogRecord for NoopLogRecord {
     #[inline]
     fn set_observed_timestamp(&mut self, _timestamp: SystemTime) {}
     #[inline]
-    fn set_span_context(&mut self, _context: &SpanContext) {}
-    #[inline]
     fn set_severity_text(&mut self, _text: Cow<'static, str>) {}
     #[inline]
     fn set_severity_number(&mut self, _number: Severity) {}
     #[inline]
     fn set_body(&mut self, _body: AnyValue) {}
     #[inline]
-    fn set_attributes(&mut self, _attributes: Vec<(Key, AnyValue)>) {}
+    fn add_attributes(&mut self, _attributes: Vec<(Key, AnyValue)>) {}
     #[inline]
-    fn set_attribute<K, V>(&mut self, _key: K, _value: V)
+    fn add_attribute<K, V>(&mut self, _key: K, _value: V)
     where
         K: Into<Key>,
         V: Into<AnyValue>,
     {
     }
-    #[inline]
-    fn set_context<T>(&mut self, _context: &T)
-    where
-        T: crate::trace::TraceContextExt,
-    {
-    }
 }
 
 /// A no-op implementation of a [`Logger`]

opentelemetry/src/logs/record.rs

@@ -1,21 +1,14 @@
-use crate::{
-    trace::{SpanContext, TraceContextExt},
-    Array, Key, StringValue, Value,
-};
+use crate::{Array, Key, StringValue, Value};
 use std::{borrow::Cow, collections::HashMap, time::SystemTime};
 
-/// Abstract interface for managing log records. The implementation is provided by the SDK
-/// Abstract interface for log records in an observability framework.
+/// SDK implemented trait for managing log records
 pub trait LogRecord {
     /// Sets the time when the event occurred measured by the origin clock, i.e. the time at the source.
     fn set_timestamp(&mut self, timestamp: SystemTime);
 
     /// Sets the observed event timestamp.
     fn set_observed_timestamp(&mut self, timestamp: SystemTime);
 
-    /// Associates a span context.
-    fn set_span_context(&mut self, context: &SpanContext);
-
     /// Sets severity as text.
     fn set_severity_text(&mut self, text: Cow<'static, str>);
 
@@ -26,23 +19,13 @@ pub trait LogRecord {
     fn set_body(&mut self, body: AnyValue);
 
     /// Adds multiple attributes.
-    fn set_attributes(&mut self, attributes: Vec<(Key, AnyValue)>);
+    fn add_attributes(&mut self, attributes: Vec<(Key, AnyValue)>);
 
-    /// Adds or updates a single attribute.
-    fn set_attribute<K, V>(&mut self, key: K, value: V)
+    /// Adds a single attribute.
+    fn add_attribute<K, V>(&mut self, key: K, value: V)
     where
         K: Into<Key>,
         V: Into<AnyValue>;
-
-    /// Sets the trace context, pulling from the active span if available.
-    fn set_context<T>(&mut self, context: &T)
-    where
-        T: TraceContextExt,
-    {
-        if context.has_active_span() {
-            self.set_span_context(context.span().span_context())
-        }
-    }
 }
 
 /// Value types for representing arbitrary values in a log record.