Fix handling of OTLP ArrayValue attributes#3238
Conversation
| Assert.Null(stringArray[2].Value); | ||
| Assert.Equal("test", stringArray.Value.ArrayValue.Values[0].StringValue); | ||
| Assert.Equal(string.Empty, stringArray.Value.ArrayValue.Values[1].StringValue); | ||
| Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.None, stringArray.Value.ArrayValue.Values[2].ValueCase); |
There was a problem hiding this comment.
This may not be right. It seems closest to what we were doing before, but technically an array should be homogenous.
| default: | ||
| PooledList<OtlpCommon.KeyValue>.Add(ref this.Tags, CreateOtlpKeyValue(key, new OtlpCommon.AnyValue { StringValue = activityTag.Value.ToString() })); |
There was a problem hiding this comment.
This is a little suspect. I think we should consider dropping things that do not fit the types we want to support.
ToString will often just serialize to a type name and worse it could even throw an exception.
There was a problem hiding this comment.
Agree. Log and drop would be safer here.
|
|
||
| PooledList<OtlpCommon.KeyValue>.Add(ref this.Tags, CreateOtlpKeyValue(key, new OtlpCommon.AnyValue { ArrayValue = arrayValue })); | ||
| break; | ||
| case double[] doubleArray: |
There was a problem hiding this comment.
There's a number of types we're not consistently handling across everything. For example, we're not handling float or float[] in this code. Should we? Will continue discussion here #2010
Codecov Report
@@ Coverage Diff @@
## main #3238 +/- ##
==========================================
+ Coverage 85.40% 85.53% +0.13%
==========================================
Files 261 261
Lines 9385 9395 +10
==========================================
+ Hits 8015 8036 +21
+ Misses 1370 1359 -11
|
|
#3229 is this related as well? |
Yes, definitely. I want to address this as well. Problem is we have a number of disparate code paths that transform attributes to OTLP, and they're not equivalent. I imagine we may have similar issues in some of the other exporters as well (Jaeger/Zipkin). The code path for resource attributes does not yet handle arrays. The same code path is used by the OTLP logs exporter. I suspect I can refactor things to be a little less disjointed, but I want to do things in a few passes. I think I'm going to take on the work of #2010 - first in the context of OTLP trace data. That way we can come to a consensus of what types of attributes we want to support and then I can work on making sure everything works uniformly. |
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM. User facing bug fix, so changelog.md also please!
2 participants
The proto has an
ArrayValue. We were not using it. This PR fixes things forActivitys.There's a slew of other bugs in and around handling OTLP attributes. Will continue to follow up with some more PRs.