OTLP exporter: Standardize handling of attributes #3262
Conversation
| // case ulong: May throw an exception on overflow. | ||
| // case decimal: Converting to double produces rounding errors. | ||
| default: | ||
| return null; |
There was a problem hiding this comment.
this would be a breaking change? as we were doing ToString() before, and now we ignore the value? should we retain the current behavior?
(Or we treat current behavior as a unintentional bug, and this is indeed the fix)
Either way - please add to changelog entry as well.
There was a problem hiding this comment.
Or we treat current behavior as a unintentional bug, and this is indeed the fix
This is where I'm leaning but want to hear other opinions. While unusual, I have seen a ToString overload be susceptible to exceptions, mutating state, and even deadlock.
As an end user, I'd prefer to have to be intentional about how I'm ToStringing my attributes that are not just simple numeric types or strings.
There was a problem hiding this comment.
I think this is the right change.
One thing to note is - do we log anywhere about this? i.e if I miss some values, and i enable self-diagnostics, would there be something that tells me exporter is dropping things?
There was a problem hiding this comment.
At one point, I thought the Jaeger and Zipkin exporters did not ToString arbitrary attributes. I guess they do now. So, I'm ok ToStringing everything else if that's what folks prefer.
There was a problem hiding this comment.
One thing to note is - do we log anywhere about this? i.e if I miss some values, and i enable self-diagnostics, would there be something that tells me exporter is dropping things?
No logging at the moment - will add.
There was a problem hiding this comment.
I think this is the right change.
In that case, we should update Zipkin, Jaeger, Console, Prometheus to behave the same. Maybe there's some way to unify this logic, but I haven't put my mind to this yet.
There was a problem hiding this comment.
From SIG meet: revert to use ToString(), and explore IFormattable in a follow up.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpCommonExtensions.cs
Outdated
Show resolved
Hide resolved
alanwest
force-pushed
the
alanwest/otlp-attributes
branch
from
2f4d235 to
8624117
Compare
Codecov Report
@@ Coverage Diff @@
## main #3262 +/- ##
==========================================
+ Coverage 85.48% 85.65% +0.16%
==========================================
Files 262 263 +1
Lines 9502 9515 +13
==========================================
+ Hits 8123 8150 +27
+ Misses 1379 1365 -14
|
…lemetry-dotnet into alanwest/otlp-attributes
|
@cijothomas I think this is good to go now. As discussed in the SIG meeting, I've restored the behavior of calling |
Related to #2010.
This PR is specifically in the context of the OTLP exporter but my purpose is to drive the more general conversation for what datatypes all SDK components should handle.
I've taken a pretty broad stance to begin with. This PR ensures that the OTLP exporter handles strings and nearly all .NET built-in value types. Arrays of these types are handled as well (partially addresses #3229 for OTLP exporter).
The
ToOtlpAttributemethod handles attributes on logs, metrics, and resources. Attributes on trace data is handled in a different part of the code and has not yet been expanded to handle the same set of datatypes. I'll wait until we settle on the set of datatypes we want to handle to fix traces.