fix: Avoid stringifying int values unless necessary #2795
Conversation
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
=====================================
Coverage 79.6% 79.6%
=====================================
Files 124 124
Lines 23179 23219 +40
=====================================
+ Hits 18461 18501 +40
Misses 4718 4718 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - `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. |
There was a problem hiding this comment.
nit - indicate it as breaking change, for customers who were expecting u64 to be serialized as string in backends?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm curious. It looks like you have only added record_u64 method implementation. How does it help usize values?
There was a problem hiding this comment.
tracing fired the u64 callbacks for usize. (they don't have a usize callback)
There was a problem hiding this comment.
Not sure if we treat this as bug fix. - converting u64 to string, or u64 to i64 both are type conversion.
There was a problem hiding this comment.
if we get an int, and instead of storing it as int and converting to string - this is what I treat as bug.
There was a problem hiding this comment.
I believe as long as end-user is losing its type - it's still a bug. It's a breaking change for me :)
3 participants
Though this works, I am wondering if AnyValue should contain variants to store u64 too?