-
Notifications
You must be signed in to change notification settings - Fork 559
feat: add valuable serialization support to appender-tracing #3033
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.
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
base: main
Are you sure you want to change the base?
feat: add valuable serialization support to appender-tracing #3033
Conversation
b99a751
to
bfd21ac
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3033 +/- ##
=======================================
+ Coverage 80.4% 80.8% +0.3%
=======================================
Files 126 127 +1
Lines 22237 22390 +153
=======================================
+ Hits 17898 18110 +212
+ Misses 4339 4280 -59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bfd21ac
to
6b24f5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds experimental support for valuable serialization to appender-tracing by extracting the serde::Serialize extension into the main package and updating dependencies accordingly.
- Introduces the valuable feature support in the tracing layer.
- Updates Cargo.toml and allowed-external-types.toml to include serde and valuable dependencies.
- Adds a new test to verify the valuable serialization behavior.
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
opentelemetry/src/logs/mod.rs | Adds serde serialization support for logs using conditional compilation. |
opentelemetry/allowed-external-types.toml | Includes serde::ser::Serialize among allowed external types. |
opentelemetry/Cargo.toml | Adds serde dependency and a new "with-serde" feature. |
opentelemetry-appender-tracing/src/layer.rs | Implements serialization for valuable support and adds a corresponding test. |
opentelemetry-appender-tracing/Cargo.toml | Introduces valuable and valuable-serde dependencies and updates feature mapping. |
opentelemetry-appender-tracing/CHANGELOG.md | Updates changelog to document the new valuable support feature. |
opentelemetry-appender-log/Cargo.toml | Adjusts the with-serde feature dependencies to reference the updated serde support. |
Cargo.toml | Adds an unexpected_cfgs lint for tracing_unstable builds to the workspace configuration. |
.cargo/config.toml | Sets the rustflags to enable tracing_unstable configuration. |
Comments suppressed due to low confidence (1)
opentelemetry-appender-tracing/src/layer.rs:187
- Consider adding an inline comment here clarifying the rationale behind using the debug formatted value as a fallback when serialization fails.
}
0c59e45
to
1cd7694
Compare
Some(value) => self.log_record.add_attribute(Key::new(field.name()), value), | ||
None => { | ||
self.log_record | ||
.add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want debug printout here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the fallback in a few other cases above, what's the right behavior here?
} | ||
|
||
#[derive(Debug)] | ||
struct ValueError(String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also include information of which types are we trying to serialize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. I don't see any generated errors serializing into AnyValue
?
use valuable::Valuable; | ||
|
||
#[derive(Clone, Debug, Valuable)] | ||
struct User { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add tests with deeper nested structures? is there any limit to the depth? (Spec is still unclear on how to handle this, but want to make sure we have some limits.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test. I don't see a way to limit the depth in valuable
, what would the expected output be?
e4ff5a4
to
dabc7ad
Compare
Fixes #2819
Design discussion issue (if applicable) N/A
Changes
Supports the
valuable
extension totracing
for appender-tracing. I moved theserde::Serialize
extension out of appender-log into the main package in order to share it with appender-tracing, but if that's not ideal, happy to do something else.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes