#869 - export array attributes to OTLP

[stefanprisca]

PR to resolve issue #869

• create new toArrayAttribute(v kv.KeyValue) function which converts each accepted array type to the otel-common equivalent
• added new test case for array attributes, covering the currently accepted array types: "bool", "int", "int32", "int64", "float32", "float64", "string", "uint", "uint32", "uint64"

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Stefan Prisca (b76c665, a5d7c7b, 9945bdd, fe0ca1b)

[MrAlias]

Looks good overall. Minor follow up questions.

[stefanprisca]

I tried to see how this change looks when exporting traces with array attributes through the collector and visualizing them from Jaeger (using the otel-collector example).
Without the change, the attributes would appear as "INVALID" strings. But with this change, the attributes appear as empty strings. This is a bit weird, as I'd expect them to actually contain the array values.

Could this be an error on the go-sdk side, the collector or on jaeger? Or is this expected given this combination? What do you think?

[MrAlias]

I tried to see how this change looks when exporting traces with array attributes through the collector and visualizing them from Jaeger (using the otel-collector example).
Without the change, the attributes would appear as "INVALID" strings. But with this change, the attributes appear as empty strings. This is a bit weird, as I'd expect them to actually contain the array values.

Could this be an error on the go-sdk side, the collector or on jaeger? Or is this expected given this combination? What do you think?

It looks like the Jaeger translator in the collector doesn't have a translation for an Array type yet. Though, it looks like the collector's internal representation doesn't support this yet either.

I think the level of testing you have done seems reasonable enough from a client perspective. It would be cool to include some integration testing when the support is added.

[MrAlias]

@stefanprisca is this ready to merge, or did you want to continue investigating the integration story?

[stefanprisca]

@stefanprisca is this ready to merge, or did you want to continue investigating the integration story?

Sorry, I didn't realize you were waiting for an answer from me.

There seems to be an integration test already, covering attributes as well: otel_integration_tests. Although it just tests integers, the others should work also, since it's the same code path.

Maybe it would be interesting to create more use-cases for the attributes, by testing the other types (string, bool, array, etc) as well. But I'm not sure how many benefits it would bring.

Either way, I think this should be merged. Integration tests can be covered in a different issue, since we might want to have more than just for arrays. What do you think?

[MrAlias]

Sorry, I didn't realize you were waiting for an answer from me.

No worries. I figured as much, just wanted to be sure.

Integration tests can be covered in a different issue, since we might want to have more than just for arrays. What do you think?

Sounds good to me 👍