Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 5 additions & 0 deletions opentelemetry-appender-tracing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ transparent to most users.
implementations (SDK, processor, exporters) to leverage this additional
information to determine if an event is enabled.

- `u64` and `usize` values are stored as `opentelemetry::logs::AnyValue::Int`
when conversion is feasible. Otherwise stored as
`opentelemetry::logs::AnyValue::String`. This avoids unnecessary string
allocation when values can be represented in their original types.
Copy link
Member

Choose a reason for hiding this comment

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

nit - indicate it as breaking change, for customers who were expecting u64 to be serialized as string in backends?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am unsure about that. I think it is a bug to convert a u64 to string by default, so this is more like a bug fix. And naturally, every bug fix can be thought of as breaking change to those whole relied on buggy behavior 🤣

We may still do more tweaks here..Do you think it should NOT be done post 1.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious. It looks like you have only added record_u64 method implementation. How does it help usize values?

Copy link
Member Author

Choose a reason for hiding this comment

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

tracing fired the u64 callbacks for usize. (they don't have a usize callback)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we treat this as bug fix. - converting u64 to string, or u64 to i64 both are type conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we get an int, and instead of storing it as int and converting to string - this is what I treat as bug.

Copy link
Member

Choose a reason for hiding this comment

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

I believe as long as end-user is losing its type - it's still a bug. It's a breaking change for me :)


## 0.28.1

Released 2025-Feb-12
Expand Down
51 changes: 48 additions & 3 deletions opentelemetry-appender-tracing/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,23 @@ impl<LR: LogRecord> tracing::field::Visit for EventVisitor<'_, LR> {
.add_attribute(Key::new(field.name()), AnyValue::from(value));
}

fn record_u64(&mut self, field: &tracing::field::Field, value: u64) {
#[cfg(feature = "experimental_metadata_attributes")]
if is_duplicated_metadata(field.name()) {
return;
}
match i64::try_from(value) {
Ok(signed) => {
self.log_record
.add_attribute(Key::new(field.name()), AnyValue::from(signed));
}
Err(_) => {
self.log_record
.add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}")));
}
}
}

// TODO: Remaining field types from AnyValue : Bytes, ListAny, Boolean
}

Expand Down Expand Up @@ -331,7 +348,11 @@ mod tests {
let _guard = tracing::subscriber::set_default(subscriber);

// Act
error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]");
let small_u64value: u64 = 42;
let big_u64value: u64 = u64::MAX;
let small_usizevalue: usize = 42;
let big_usizevalue: usize = usize::MAX;
error!(name: "my-event-name", target: "my-system", event_id = 20, small_u64value, big_u64value, small_usizevalue, big_usizevalue, user_name = "otel", user_email = "[email protected]");
assert!(logger_provider.force_flush().is_ok());

// Assert TODO: move to helper methods
Expand Down Expand Up @@ -362,9 +383,9 @@ mod tests {

// Validate attributes
#[cfg(not(feature = "experimental_metadata_attributes"))]
assert_eq!(log.record.attributes_iter().count(), 3);
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(log.record.attributes_iter().count(), 7);
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(log.record.attributes_iter().count(), 11);
assert!(attributes_contains(
&log.record,
&Key::new("event_id"),
Expand All @@ -380,6 +401,26 @@ mod tests {
&Key::new("user_email"),
&AnyValue::String("[email protected]".into())
));
assert!(attributes_contains(
&log.record,
&Key::new("small_u64value"),
&AnyValue::Int(42.into())
));
assert!(attributes_contains(
&log.record,
&Key::new("big_u64value"),
&AnyValue::String(format!("{}", u64::MAX).into())
));
assert!(attributes_contains(
&log.record,
&Key::new("small_usizevalue"),
&AnyValue::Int(42.into())
));
assert!(attributes_contains(
&log.record,
&Key::new("big_usizevalue"),
&AnyValue::String(format!("{}", u64::MAX).into())
));
#[cfg(feature = "experimental_metadata_attributes")]
{
assert!(attributes_contains(
Expand Down Expand Up @@ -753,6 +794,10 @@ mod tests {
TraceFlags::SAMPLED
);

for attribute in log.record.attributes_iter() {
println!("key: {:?}, value: {:?}", attribute.0, attribute.1);
}

// Attributes can be polluted when we don't use this feature.
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(log.record.attributes_iter().count(), 4);
Expand Down