attribute: fix slice related function bug

[fatsheep9146]

Signed-off-by: Ziqi Zhao [email protected]

fix #3108

[MrAlias]

This looks good overall. Thanks for the included tests!

This needs a changelog update with a Fixed entry describing the bug it resolves.

[MrAlias]

It looks like the decode logic also needs to be addressed, there are tests failing currently.

[MadVikingGod]

I know this does fix the problem, but it does so at a very high cost, copying the slices into a dynamically created array. I would think we need to explore some way to achieve our goal without all the copying.

[jmacd]

To fix this PR, at the very least, requires fixing Emit(), however even with this fix the tests do not all pass.

diff --git a/attribute/value.go b/attribute/value.go
index 4c5bf74c..b85888a7 100644
--- a/attribute/value.go
+++ b/attribute/value.go
@@ -266,19 +266,19 @@ func (v Value) AsInterface() interface{} {
 func (v Value) Emit() string {
 	switch v.Type() {
 	case BOOLSLICE:
-		return fmt.Sprint(*(v.slice.(*[]bool)))
+		return fmt.Sprint(v.AsBoolSlice())
 	case BOOL:
 		return strconv.FormatBool(v.AsBool())
 	case INT64SLICE:
-		return fmt.Sprint(*(v.slice.(*[]int64)))
+		return fmt.Sprint(v.AsInt64Slice())
 	case INT64:
 		return strconv.FormatInt(v.AsInt64(), 10)
 	case FLOAT64SLICE:
-		return fmt.Sprint(*(v.slice.(*[]float64)))
+		return fmt.Sprint(v.AsFloat64Slice())
 	case FLOAT64:
 		return fmt.Sprint(v.AsFloat64())
 	case STRINGSLICE:
-		return fmt.Sprint(*(v.slice.(*[]string)))
+		return fmt.Sprint(v.AsStringSlice())
 	case STRING:
 		return v.stringly
 	default:

[codecov]

Codecov Report

Merging #3252 (1ed993e) into main (4a3adaa) will increase coverage by 0.1%.
The diff coverage is 88.5%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3252     +/-   ##
=======================================
+ Coverage   77.6%   77.7%   +0.1%     
=======================================
  Files        161     163      +2     
  Lines      11260   11227     -33     
=======================================
- Hits        8741    8730     -11     
+ Misses      2319    2299     -20     
+ Partials     200     198      -2     

Impacted Files	Coverage Δ
attribute/value.go	89.5% <76.4%> (+0.5%)	⬆️
internal/attribute/attribute.go	81.2% <81.2%> (ø)
sdk/trace/provider.go	89.2% <100.0%> (+3.5%)	⬆️
sdk/trace/span.go	87.9% <100.0%> (-0.4%)	⬇️
sdk/trace/span_processor.go	100.0% <100.0%> (ø)
sdk/trace/tracer.go	100.0% <100.0%> (ø)
exporters/jaeger/jaeger.go	91.1% <0.0%> (+0.8%)	⬆️
attribute/kv.go	91.6% <0.0%> (+41.6%)	⬆️

[fatsheep9146]

I know this does fix the problem, but it does so at a very high cost, copying the slices into a dynamically created array. I would think we need to explore some way to achieve our goal without all the copying.

Yes, for now, I do not find any other more efficient way to implement this, I will keep digging about this. Do you have any suggestions? @MadVikingGod @jmacd @MrAlias

[fatsheep9146]

It looks like the decode logic also needs to be addressed, there are tests failing currently.

All failed checks are resolved. Please help review this again. @MrAlias