Skip to content

Fix attributes protobuf encoding #609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix attributes protobuf encoding #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1492640
Fix attributes protobuf encoding
albertored Aug 22, 2023
f9f7b48
Consider also eunit cover reports
albertored Aug 23, 2023
358bee6
Update apps/opentelemetry_exporter/src/otel_otlp_common.erl
albertored Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 98 additions & 21 deletions apps/opentelemetry_exporter/src/otel_otlp_common.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@
to_key_value_list/1,
to_binary/1]).

-include_lib("kernel/include/logger.hrl").
-include_lib("opentelemetry_api/include/opentelemetry.hrl").

-ifdef(TEST).
-include_lib("eunit/include/eunit.hrl").
-endif.

to_instrumentation_scope_proto(undefined) ->
#{};
to_instrumentation_scope_proto(#instrumentation_scope{name=Name,
Expand All @@ -42,9 +47,14 @@ to_instrumentation_scope_proto(#instrumentation_scope{name=Name,
-spec to_attributes(opentelemetry:attributes_map() | otel_attributes:t() | undefined) -> [opentelemetry_exporter_trace_service_pb:key_value()].
to_attributes(Attributes) when is_map(Attributes) ->
maps:fold(fun(Key, Value, Acc) ->
[#{key => to_binary(Key),
value => to_any_value(Value)} | Acc]
end, [], Attributes);
case to_any_value(Value) of
invalid ->
?LOG_DEBUG("OpenTelemetry exporter: discarded invalid attribute, ~p", [{Key, Value}]),
Acc;
ParsedValue ->
[#{key => to_binary(Key), value => ParsedValue} | Acc]
end
end, [], Attributes);
to_attributes(Attributes) when is_list(Attributes) ->
to_attributes(maps:from_list(Attributes));
to_attributes(Attributes) when is_tuple(Attributes) ->
Expand All @@ -71,28 +81,22 @@ to_any_value(Value) when is_boolean(Value) ->
to_any_value(Value) when is_map(Value) ->
#{value => {kvlist_value, to_key_value_list(maps:to_list(Value))}};
to_any_value(Value) when is_tuple(Value) ->
#{value => {array_value, to_array_value(tuple_to_list(Value))}};
to_any_value(Value) when is_list(Value) ->
try unicode:characters_to_binary(Value) of
{Failure, _, _} when Failure =:= error ;
Failure =:= incomplete ->
to_array_or_kvlist(Value);
String ->
#{value => {string_value, String}}
catch
_:_ ->
to_array_or_kvlist(Value)
case to_array_value(tuple_to_list(Value)) of
invalid -> invalid;
ArrayValue -> #{value => {array_value, ArrayValue}}
end;
to_any_value(Value) ->
#{value => {string_value, to_binary(io_lib:format("~p", [Value]))}}.

to_array_or_kvlist(Value) ->
to_any_value(Value) when is_list(Value) ->
case is_proplist(Value) of
true ->
#{value => {kvlist_value, to_key_value_list(Value)}};
false ->
#{value => {array_value, to_array_value(Value)}}
end.
case to_array_value(Value) of
invalid -> invalid;
ArrayValue -> #{value => {array_value, ArrayValue}}
end
end;
to_any_value(Value) ->
#{value => {string_value, to_binary(io_lib:format("~p", [Value]))}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this used now instead of unicode:characters_to_binary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we have a path forward on this? hehe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing in favor of #633


to_key_value_list(List) ->
#{values => to_key_value_list(List, [])}.
Expand All @@ -109,10 +113,21 @@ to_key_value(Key, Value) ->
value => to_any_value(Value)}.

to_array_value(List) when is_list(List) ->
#{values => [to_any_value(V) || V <- List]};
case to_array_value(List, []) of
invalid -> invalid;
Values -> #{values => Values}
end;
to_array_value(_) ->
#{values => []}.


to_array_value([Elem | _List], _Acc) when is_tuple(Elem); is_list(Elem) ->
invalid;
to_array_value([Elem | List], Acc) ->
to_array_value(List, [to_any_value(Elem) | Acc]);
to_array_value([], Acc) ->
lists:reverse(Acc).

is_proplist([]) ->
true;
is_proplist([{K, _} | L]) when is_atom(K) ; is_binary(K) ->
Expand All @@ -124,3 +139,65 @@ to_binary(Term) when is_atom(Term) ->
erlang:atom_to_binary(Term, unicode);
to_binary(Term) ->
unicode:characters_to_binary(Term).


-ifdef(TEST).

to_attributes_test_() ->
[
?_assertEqual([
#{
key => <<"bin_list">>,
value => #{
value => {array_value, #{
values => [
#{value => {string_value,<<"a">>}},
#{value => {string_value,<<"b">>}}
]
}}
}
}
], to_attributes(#{bin_list => [<<"a">>, <<"b">>]})),
?_assertEqual([
#{
key => <<"int_list">>,
value => #{
value => {array_value, #{
values => [
#{value => {int_value,1}},
#{value => {int_value,2}}
]
}}
}
}
], to_attributes(#{int_list => [1, 2]})),
?_assertEqual([
#{
key => <<"float_list">>,
value => #{
value => {array_value, #{
values => [
#{value => {double_value,1.2}},
#{value => {double_value,3.4}}
]
}}
}
}
], to_attributes(#{float_list => [1.2, 3.4]})),
?_assertEqual([
#{
key => <<"bool_list">>,
value => #{
value => {array_value, #{
values => [
#{value => {string_value,<<"true">>}},
#{value => {string_value,<<"false">>}}
]
}}
}
}
], to_attributes(#{bool_list => [true, false]})),
?_assertEqual([], to_attributes(#{attr => [1, [2, 3]]}))
].

-endif.
2 changes: 1 addition & 1 deletion rebar.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@

{cover_enabled, true}.
{cover_export_enabled, true}.
{covertool, [{coverdata_files, ["ct.coverdata"]}]}.
{covertool, [{coverdata_files, ["ct.coverdata", "eunit.coverdata"]}]}.
{cover_excl_apps, [opentelemetry_api_experimental, opentelemetry_experimental]}.
{cover_excl_mods, [opentelemetry_exporter_trace_service_pb, opentelemetry_trace_service,
opentelemetry_zipkin_pb, opentelemetry_exporter_metrics_service_pb,
Expand Down