Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 26 additions & 0 deletions opentelemetry-appender-tracing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,32 @@
Fixes [1682](https://github.com/open-telemetry/opentelemetry-rust/issues/1682).
"spec_unstable_logs_enabled" feature now do not suppress logs for other layers.

The special treatment of the "message" field has been extended when recording
string values. With this change, when a log is emitted with a field named
"message" (and string value), its value is directly assigned to the LogRecord’s
body rather than being stored as an attribute named "message". This offers a
slight performance improvement over previous.

For example, the below will now produce LogRecord with the message value
populated as LogRecord's body:

```rust
error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]", message = "This is an example message");
```

Previously, Body was only populated when the below style was used.

```rust
error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]", "This is an example message");
```

This style, while slightly slower, should still be used when the value is not a
simple string, but require format arguments as in the below example.

```rust
error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]", "This is an example message with format arguments {} and {}", "foo", "bar");
```

## 0.28.1

Released 2025-Feb-12
Expand Down
13 changes: 6 additions & 7 deletions opentelemetry-appender-tracing/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,14 @@
if is_duplicated_metadata(field.name()) {
return;
}
//TODO: Consider special casing "message" to populate body and document
// to users to use message field for log message, to avoid going to the
// record_debug, which has dyn dispatch, string allocation and
// formatting cost.

//TODO: Fix heap allocation. Check if lifetime of &str can be used
// to optimize sync exporter scenario.
self.log_record
.add_attribute(Key::new(field.name()), AnyValue::from(value.to_owned()));
if field.name() == "message" {
Copy link
Member

Choose a reason for hiding this comment

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

This would be a breaking change if user is expecting the message to go an attribute?

Copy link
Member

Choose a reason for hiding this comment

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

And what should be the expected behaviour if both message attribute and default body field is provided

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a breaking change if user is expecting the message to go an attribute?

There was no contract documented about this, so while this change behavior, I don't think it should be a major issue. (We were already special casing "message" for the record_debug, so missing this may also be considered a bug)

Copy link
Member Author

Choose a reason for hiding this comment

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

And what should be the expected behavior if both message attribute and default body field is provided

We can add tests to make sure this is covered, but the behavior is really controlled by tracing itself. We can make some decision and document it in the event of conflict, as there can only be one body.

Copy link
Member Author

@cijothomas cijothomas Feb 20, 2025

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-appender-tracing/src/layer.rs#L121 Fixing these TODOs are also going to change the behavior, so is fixing error to be reported specially. It's questionable to do that post 1.0 stable (but maybe okay), but I don't see an issue doing it at this stage. Let me know your thoughts.

Copy link
Member

@lalitb lalitb Feb 20, 2025

Choose a reason for hiding this comment

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

We were already special casing "message" for the record_debug, so missing this may also be considered a bug

Yes, I didn't realize that. With special casing, user can't have both explicit and implicit string message attributes, as they would be overwritten. But this PR doesn't add any new bug :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to document the final behavior before 1.0.
#2150 will also be related to this...

self.log_record.set_body(AnyValue::from(value.to_owned()));

Check warning on line 94 in opentelemetry-appender-tracing/src/layer.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-appender-tracing/src/layer.rs#L94

Added line #L94 was not covered by tests
} else {
self.log_record
.add_attribute(Key::new(field.name()), AnyValue::from(value.to_owned()));
}
}

fn record_bool(&mut self, field: &tracing_core::Field, value: bool) {
Expand Down