Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make LogRecord fields private and add getters for encapsulation #2314

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+94 −52
Open

Make LogRecord fields private and add getters for encapsulation #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcd6927
make logrecord fields private
lalitb Nov 21, 2024
f85aa89
more changes
lalitb Nov 21, 2024
29474f4
Merge branch 'main' into logrecord-getters
lalitb Nov 21, 2024
4507e16
Merge branch 'main' into logrecord-getters
lalitb Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions opentelemetry-appender-log/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,11 +830,11 @@ mod tests {

assert_eq!(logs.len(), 5);
for log in logs {
let body: String = match log.record.body.as_ref().unwrap() {
let body: String = match log.record.body().unwrap() {
super::AnyValue::String(s) => s.to_string(),
_ => panic!("AnyValue::String expected"),
};
assert_eq!(body, log.record.severity_text.unwrap());
assert_eq!(body, log.record.severity_text().unwrap());
}
}

Expand Down
38 changes: 14 additions & 24 deletions opentelemetry-appender-tracing/src/layer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,10 @@ mod tests {

// Validate common fields
assert_eq!(log.instrumentation.name(), "opentelemetry-appender-tracing");
assert_eq!(log.record.severity_number, Some(Severity::Error));
assert_eq!(log.record.severity_number(), Some(Severity::Error));

// Validate trace context is none.
assert!(log.record.trace_context.is_none());
assert!(log.record.trace_context().is_none());

// Validate attributes
#[cfg(not(feature = "experimental_metadata_attributes"))]
Expand Down Expand Up @@ -428,25 +428,20 @@ mod tests {

// validate common fields.
assert_eq!(log.instrumentation.name(), "opentelemetry-appender-tracing");
assert_eq!(log.record.severity_number, Some(Severity::Error));
assert_eq!(log.record.severity_number(), Some(Severity::Error));

// validate trace context.
assert!(log.record.trace_context.is_some());
assert!(log.record.trace_context().is_some());
assert_eq!(
log.record.trace_context.as_ref().unwrap().trace_id,
log.record.trace_context().unwrap().trace_id,
trace_id_expected
);
assert_eq!(
log.record.trace_context.as_ref().unwrap().span_id,
log.record.trace_context().unwrap().span_id,
span_id_expected
);
assert_eq!(
log.record
.trace_context
.as_ref()
.unwrap()
.trace_flags
.unwrap(),
log.record.trace_context().unwrap().trace_flags.unwrap(),
TraceFlags::SAMPLED
);

Expand Down Expand Up @@ -526,10 +521,10 @@ mod tests {

// Validate common fields
assert_eq!(log.instrumentation.name(), "opentelemetry-appender-tracing");
assert_eq!(log.record.severity_number, Some(Severity::Error));
assert_eq!(log.record.severity_number(), Some(Severity::Error));

// Validate trace context is none.
assert!(log.record.trace_context.is_none());
assert!(log.record.trace_context().is_none());

// Attributes can be polluted when we don't use this feature.
#[cfg(feature = "experimental_metadata_attributes")]
Expand Down Expand Up @@ -605,25 +600,20 @@ mod tests {

// validate common fields.
assert_eq!(log.instrumentation.name(), "opentelemetry-appender-tracing");
assert_eq!(log.record.severity_number, Some(Severity::Error));
assert_eq!(log.record.severity_number(), Some(Severity::Error));

// validate trace context.
assert!(log.record.trace_context.is_some());
assert!(log.record.trace_context().is_some());
assert_eq!(
log.record.trace_context.as_ref().unwrap().trace_id,
log.record.trace_context().unwrap().trace_id,
trace_id_expected
);
assert_eq!(
log.record.trace_context.as_ref().unwrap().span_id,
log.record.trace_context().unwrap().span_id,
span_id_expected
);
assert_eq!(
log.record
.trace_context
.as_ref()
.unwrap()
.trace_flags
.unwrap(),
log.record.trace_context().unwrap().trace_flags.unwrap(),
TraceFlags::SAMPLED
);

Expand Down
23 changes: 13 additions & 10 deletions opentelemetry-proto/src/transform/logs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@

impl From<&opentelemetry_sdk::logs::LogRecord> for LogRecord {
fn from(log_record: &opentelemetry_sdk::logs::LogRecord) -> Self {
let trace_context = log_record.trace_context.as_ref();
let severity_number = match log_record.severity_number {
let trace_context = log_record.trace_context();
let severity_number = match log_record.severity_number() {
Some(Severity::Trace) => SeverityNumber::Trace,
Some(Severity::Trace2) => SeverityNumber::Trace2,
Some(Severity::Trace3) => SeverityNumber::Trace3,
Expand Down Expand Up @@ -88,8 +88,8 @@
};

LogRecord {
time_unix_nano: log_record.timestamp.map(to_nanos).unwrap_or_default(),
observed_time_unix_nano: to_nanos(log_record.observed_timestamp.unwrap()),
time_unix_nano: log_record.timestamp().map(to_nanos).unwrap_or_default(),
observed_time_unix_nano: to_nanos(log_record.observed_timestamp().unwrap()),
attributes: {
let attributes: Vec<KeyValue> = log_record
.attributes_iter()
Expand All @@ -102,7 +102,7 @@
.collect();
#[cfg(feature = "populate-logs-event-name")]
{
if let Some(event_name) = &log_record.event_name {
if let Some(event_name) = &log_record.event_name() {
let mut attributes_with_name = attributes;
attributes_with_name.push(KeyValue {
key: "event.name".into(),
Expand All @@ -119,8 +119,11 @@
attributes
},
severity_number: severity_number.into(),
severity_text: log_record.severity_text.map(Into::into).unwrap_or_default(),
body: log_record.body.clone().map(Into::into),
severity_text: log_record
.severity_text()
.map(Into::into)
.unwrap_or_default(),
body: log_record.body().cloned().map(Into::into),
dropped_attributes_count: 0,
flags: trace_context
.map(|ctx| {
Expand Down Expand Up @@ -170,7 +173,7 @@
.schema_url()
.map(ToOwned::to_owned)
.unwrap_or_default(),
scope: Some((instrumentation, log_record.target.clone()).into()),
scope: Some((instrumentation, log_record.target().cloned()).into()),

Check warning on line 176 in opentelemetry-proto/src/transform/logs.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/logs.rs#L176 

Added line #L176 was not covered by tests
log_records: vec![log_record.into()],
}],
}
Expand All @@ -193,8 +196,8 @@
>,
(log_record, instrumentation)| {
let key = log_record
.target
.clone()
.target()
.cloned()
.unwrap_or_else(|| Cow::Owned(instrumentation.name().to_owned()));
scope_map
.entry(key)
Expand Down
65 changes: 57 additions & 8 deletions opentelemetry-sdk/src/logs/record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,28 +24,28 @@ pub(crate) type LogRecordAttributes =
/// is provided to `LogExporter`s as input.
pub struct LogRecord {
/// Event name. Optional as not all the logging API support it.
pub event_name: Option<&'static str>,
pub(crate) event_name: Option<&'static str>,

/// Target of the log record
pub target: Option<Cow<'static, str>>,
pub(crate) target: Option<Cow<'static, str>>,

/// Record timestamp
pub timestamp: Option<SystemTime>,
pub(crate) timestamp: Option<SystemTime>,

/// Timestamp for when the record was observed by OpenTelemetry
pub observed_timestamp: Option<SystemTime>,
pub(crate) observed_timestamp: Option<SystemTime>,

/// Trace context for logs associated with spans
pub trace_context: Option<TraceContext>,
pub(crate) trace_context: Option<TraceContext>,

/// The original severity string from the source
pub severity_text: Option<&'static str>,
pub(crate) severity_text: Option<&'static str>,

/// The corresponding severity value, normalized
pub severity_number: Option<Severity>,
pub(crate) severity_number: Option<Severity>,

/// Record body
pub body: Option<AnyValue>,
pub(crate) body: Option<AnyValue>,

/// Additional attributes associated with this record
pub(crate) attributes: LogRecordAttributes,
Expand Down Expand Up @@ -118,7 +118,56 @@ impl opentelemetry::logs::LogRecord for LogRecord {
}

impl LogRecord {
/// Returns the event name
#[inline]
pub fn event_name(&self) -> Option<&'static str> {
self.event_name
}

/// Returns the target
#[inline]
pub fn target(&self) -> Option<&Cow<'static, str>> {
self.target.as_ref()
}
Copy link
Member Author

@lalitb lalitb Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning &str would simplify the API, but want to preserve the ability for consumers(i.e., exporter) to optimize based on whether the target is a static string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could favor simplicity of the API here and use &str as the return type. The current exporters that we have cannot utilize the static str reference to their benefit anyway.

Only exporters that would benefit from Cow<'static, str> would be the ones that hold these values in memory for some reason as they could then avoid the cost to clone a static str.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a strong opinion here as it's mainly used by exporters and not end users. We can expect exporter authors to work with Cow<'static,str>.

Copy link
Member Author

@lalitb lalitb Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a strong opinion here as it's mainly used by exporters and not end users. We can expect exporter authors to work with Cow<'static,str>.

Yes, that's what I thought - keeping it for the custom export authors if they want to reap the benefit of static target. Existing exporter (in main and contrib repo) won't get the benefit as they serialize the data to be sent across.


/// Returns the timestamp
#[inline]
pub fn timestamp(&self) -> Option<SystemTime> {
self.timestamp
}

/// Returns the observed timestamp
#[inline]
pub fn observed_timestamp(&self) -> Option<SystemTime> {
self.observed_timestamp
}

/// Returns the trace context
#[inline]
pub fn trace_context(&self) -> Option<&TraceContext> {
self.trace_context.as_ref()
}

/// Returns the severity text
#[inline]
pub fn severity_text(&self) -> Option<&'static str> {
self.severity_text
}

/// Returns the severity number
#[inline]
pub fn severity_number(&self) -> Option<Severity> {
self.severity_number
}

/// Returns the body
#[inline]
pub fn body(&self) -> Option<&AnyValue> {
self.body.as_ref()
}

/// Provides an iterator over the attributes.
#[inline]
pub fn attributes_iter(&self) -> impl Iterator<Item = &(Key, AnyValue)> {
self.attributes.iter().filter_map(|opt| opt.as_ref())
}
Expand Down
16 changes: 8 additions & 8 deletions opentelemetry-stdout/src/logs/exporter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,34 +69,34 @@
for (i, log) in batch.iter().enumerate() {
println!("Log #{}", i);
let (record, _library) = log;
if let Some(event_name) = record.event_name {
if let Some(event_name) = record.event_name() {

Check warning on line 72 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L72 

Added line #L72 was not covered by tests
println!("\t EventName: {:?}", event_name);
}
if let Some(target) = &record.target {
if let Some(target) = record.target() {

Check warning on line 75 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L75 

Added line #L75 was not covered by tests
println!("\t Target (Scope): {:?}", target);
}
if let Some(trace_context) = &record.trace_context {
if let Some(trace_context) = record.trace_context() {

Check warning on line 78 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L78 

Added line #L78 was not covered by tests
println!("\t TraceId: {:?}", trace_context.trace_id);
println!("\t SpanId: {:?}", trace_context.span_id);
}
if let Some(timestamp) = record.timestamp {
if let Some(timestamp) = record.timestamp() {

Check warning on line 82 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L82 

Added line #L82 was not covered by tests
let datetime: DateTime<Utc> = timestamp.into();
println!("\t Timestamp: {}", datetime.format("%Y-%m-%d %H:%M:%S%.6f"));
}
if let Some(timestamp) = record.observed_timestamp {
if let Some(timestamp) = record.observed_timestamp() {

Check warning on line 86 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L86 

Added line #L86 was not covered by tests
let datetime: DateTime<Utc> = timestamp.into();
println!(
"\t Observed Timestamp: {}",
datetime.format("%Y-%m-%d %H:%M:%S%.6f")
);
}
if let Some(severity) = record.severity_text {
if let Some(severity) = record.severity_text() {

Check warning on line 93 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L93 

Added line #L93 was not covered by tests
println!("\t SeverityText: {:?}", severity);
}
if let Some(severity) = record.severity_number {
if let Some(severity) = record.severity_number() {

Check warning on line 96 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L96 

Added line #L96 was not covered by tests
println!("\t SeverityNumber: {:?}", severity);
}
if let Some(body) = &record.body {
if let Some(body) = record.body() {

Check warning on line 99 in opentelemetry-stdout/src/logs/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/logs/exporter.rs#L99 

Added line #L99 was not covered by tests
println!("\t Body: {:?}", body);
}

Expand Down
Loading