Fix attributes protobuf encoding

[albertored]

Attributes with array values are wrongly encoded by exporter:

1> otel_otlp_common:to_attributes(#{
        bin_list => [<<"a">>, <<"b">>],
        int_list => [1, 2],
        float_list => [1.2, 3.4],
        bool_list => [true, false]
    }).

[#{key => <<"int_list">>,
   value => #{value => {string_value,<<1,2>>}}},
 #{key => <<"float_list">>,
   value =>
       #{value =>
             {array_value,#{values =>
                                [#{value => {double_value,1.2}},
                                 #{value => {double_value,3.4}}]}}}},
 #{key => <<"bool_list">>,
   value =>
       #{value =>
             {array_value,#{values =>
                                [#{value => {string_value,<<"true">>}},
                                 #{value => {string_value,<<"false">>}}]}}}},
 #{key => <<"bin_list">>,
   value => #{value => {string_value,<<"ab">>}}}]

See specification about attributes

At the moment I choose to emit an error for nested arrays and omit them from the export but not sure it is the best option

[codecov]

Codecov Report

Patch coverage: 92.59% and project coverage change: +0.31% 🎉

Comparison is base (23c49ec) 72.26% compared to head (358bee6) 72.58%.
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
+ Coverage   72.26%   72.58%   +0.31%     
==========================================
  Files          60       60              
  Lines        1904     1922      +18     
==========================================
+ Hits         1376     1395      +19     
+ Misses        528      527       -1     

Flag	Coverage Δ
api	68.06% <ø> (ø)
elixir	18.31% <ø> (ø)
erlang	73.89% <92.59%> (+0.31%)	⬆️
exporter	69.49% <92.59%> (+2.69%)	⬆️
sdk	78.13% <ø> (ø)
zipkin	54.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...ps/opentelemetry_exporter/src/otel_otlp_common.erl	83.63% <92.59%> (+10.66%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tsloughter]

Why is this used now instead of unicode:characters_to_binary?

[ferd]

similarly, if using ~p I would imagine wanting ~tp as well at least.

[albertored]

This is not a new addition but only old code that changed position. It is the catch-all clause for values that do not fall into the attributes allowed types.

Be aware that it's not calling list_to_binary but only a private to_binary function (already defined in the old code) that is exactly unicode:characters_to_binary

[tsloughter]

Ah, I see now. Got confused using the diff.

But the use of unicode:character_to_binary to tell if a list is a string is still removed, right? That is what I noticed and lead me to misread this code as new.

[albertored]

Yes, it has been removed, that part was the bug.

Attribute values are correctly typed to not be charlist because in that case they are totally undistinguishable from list of integers (that is an allowed attribute value).

So I removed the conversion of charlist to string_value

[tsloughter]

One reason to construct an attributes record manually is to use in multiple signals, so then it'd somehow have to know if you aren't using any signal (traces, metrics, logs) to know if it should no-op.

Having the option to pass just a map/list instead of the record does at least give the user the ability to pass attributes only to a single signal in a call like with_span and have them ignored.

[albertored]

We can accept both raw maps/lists or attributes record and document that if a record is used the validation and truncation overhead is executed also in a noop case. If a user want to be sure to not add overhead when not needed then he should use maps/lists.

But he main point is that I would like to only save attributes in the validated record form so that exporters downstream do not need to re-validate attributes.

[tsloughter]

Did we have a path forward on this? hehe

[albertored]

No, sorry, too many open branches and I forgot about this one, I'll take a look in the next few days

[albertored]

Closing in favor of #633