Zipkin span exporter does not serialize arrays as JSON

[mateuszrzeszutek]

Describe the bug
The spec mentions that arrays must be serialized to string as a JSON list:

Array values MUST be serialized to string like a JSON list as mentioned in semantic conventions.

Zipkin exporter just naively joins all values with a comma, it doesn't add the brackets, it doesn't wrap string values with "". Note that Jaeger exporter does that correctly and writes all arrays as JSON lists.

Additional context
Zipkin unit tests, Jaeger unit tests

[jack-berg]

Are array tags actually supported in zipkin's data model? I get a 400 Bad request when I try to send up a payload with them.

For example, the following with just a string tag yields a 202 status code:

curl 'http://localhost:9411/api/v2/spans' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  --data-raw $'[{"traceId":"1e276c4d8f892d3e80fb2fa9463ff387","parentId":"aa5ec909d57c3dc0","id":"262c725901ec8f8c","kind":"SERVER","name":"dummy","timestamp":1631286779996107,"duration":541865,"localEndpoint":{"serviceName":"foo","ipv4":"192.168.0.91"},"tags":{"string":"value"}}]' \
  --compressed \
  -v
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 9411 (#0)
> POST /api/v2/spans HTTP/1.1
> Host: localhost:9411
> User-Agent: curl/7.64.1
> Accept-Encoding: deflate, gzip
> accept: application/json
> Content-Type: application/json
> Content-Length: 266
> 
* upload completely sent off: 266 out of 266 bytes
< HTTP/1.1 202 Accepted
< content-length: 0
< server: Armeria/1.3.0
< date: Fri, 10 Sep 2021 15:28:59 GMT
< 
* Connection #0 to host localhost left intact
* Closing connection 0

But add an array tag, and you get a 400 status code:

curl 'http://localhost:9411/api/v2/spans' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  --data-raw $'[{"traceId":"1e276c4d8f892d3e80fb2fa9463ff387","parentId":"aa5ec909d57c3dc0","id":"262c725901ec8f8c","kind":"SERVER","name":"dummy","timestamp":1631286779996107,"duration":541865,"localEndpoint":{"serviceName":"foo","ipv4":"192.168.0.91"},"tags":{"string":"value","array":["value1","value2"]}}]' \
  --compressed \
  -v
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 9411 (#0)
> POST /api/v2/spans HTTP/1.1
> Host: localhost:9411
> User-Agent: curl/7.64.1
> Accept-Encoding: deflate, gzip
> accept: application/json
> Content-Type: application/json
> Content-Length: 294
> 
* upload completely sent off: 294 out of 294 bytes
< HTTP/1.1 400 Bad Request
< content-type: text/*
< content-length: 108
< server: Armeria/1.3.0
< date: Fri, 10 Sep 2021 15:29:54 GMT
< 
* Connection #0 to host localhost left intact
Expected a string but was BEGIN_ARRAY at line 1 column 274 path $[0].tags.array reading List<Span> from json* Closing connection 0

The zipkin proto definition seems to confirm this.

[jkwatson]

We definitely can't put json arrays into zipkin as if they were standard attribute values. @mateuszrzeszutek Are you saying we should escape the json-array so that it can be coerced into a String? I think what we built here was the recommendation of the zipkin maintainer at the time, as the closest that zipkin could get to supporting arrays.

[mateuszrzeszutek]

Are you saying we should escape the json-array so that it can be coerced into a String?

Hmm, possibly? I think that's what we do in Jaeger (well, it's a string-in-protobuf not string-in-json though). I think that's probably what spec means by serialized to string like a JSON list too - the result is a string.

[jack-berg]

The dotnet implementation appears to just comma separate the values, rather than a JSON list string.

The js implementation does the same as dotnot. (String(['foo', 'bar']) => 'foo,bar')

The go implementation serializes as JSON list string as @jkwatson is suggesting.

So there's already divergence in the implementations. It may be worth clarifying with the spec.

[jkwatson]

So, it sounds like .net and java are currently in alignment, then?

[jack-berg]

So, it sounds like .net and java are currently in alignment, then?

Yes.

[jack-berg]

FYI, spec conversation related to this here: open-telemetry/opentelemetry-specification#5053 (comment)

We're definitely not complaint with the spec, but given that the zipkin exporter is deprecated for removal, probably not worth fixing at this point.