Skip to content

Commit 75d0e82

Browse files
tsloughtertsloughter
authored
Merge branch 'main' into bump-rc4
2 parents 4fb0ce1 + 6585266 commit 75d0e82

File tree

13 files changed

+367
-102
lines changed

13 files changed

+367
-102
lines changed

‎CHANGELOG.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3737
`OTEL_REGISTER_LOADED_APPLICATIONS` and `register_loaded_applications`, will
3838
continue to work as well.
3939

40+
### Fixed
41+
42+
- Attribute values now validate against what is allowable per the specification
43+
rather than allowing anything the protobuf could encode. This may be breaking
44+
to some users who were relying on the incorrect behavior, such as allowing
45+
dictionaries or non-homogenous lists/tuples. The one exception we have
46+
kept is continuing to allow atoms in place of binaries for performance.
47+
- Attribute values of type list/tuple must be homogenous.
48+
- Span start opts are now validated. Previously, opts underwent no validations.
49+
- Event and link attributes are now validated. Previously only span attributes
50+
were validated.
51+
- Events accept atoms for the name again.
52+
4053
### Removed
4154

4255
- The `sampler` option to `start_span` and `with_span` was removed.

‎apps/opentelemetry/src/otel_attributes.erl‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -71,35 +71,38 @@ update_attributes(List, Attributes) ->
7171
%% add key/value if the size is still under the limit or the key is already in the map
7272
update_attribute(Key, Value, Attributes=#attributes{count_limit=CountLimit,
7373
value_length_limit=ValueLengthLimit,
74-
dropped=Dropped,
7574
map=Map})
76-
when (map_size(Map) < CountLimit orelse is_map_key(Key, Map)),
77-
(is_binary(Key) orelse is_atom(Key)) ->
78-
case is_allowed(Value, ValueLengthLimit) of
79-
{true, V} ->
80-
Attributes#attributes{map=Map#{Key => V}};
81-
82-
%% Value isn't a primitive or a list/array so drop it
83-
false ->
84-
Attributes#attributes{dropped=Dropped + 1}
85-
end;
86-
%% Key must be a binary string or atom
87-
update_attribute(_Key, _Value, Attributes=#attributes{dropped=Dropped}) ->
88-
Attributes#attributes{dropped=Dropped + 1}.
75+
when is_binary(Value) , (map_size(Map) < CountLimit orelse is_map_key(Key, Map)) ->
76+
Attributes#attributes{map=Map#{Key => maybe_truncate_binary(Value, ValueLengthLimit)}};
77+
%% value is a list of binaries, so potentially truncate
78+
update_attribute(Key, [Value1 | _Rest] = Value, Attributes=#attributes{count_limit=CountLimit,
79+
value_length_limit=ValueLengthLimit,
80+
map=Map})
81+
when is_binary(Value1) , (map_size(Map) < CountLimit orelse is_map_key(Key, Map)) ->
82+
Attributes#attributes{map=Map#{Key => [maybe_truncate_binary(V, ValueLengthLimit) || V <- Value]}};
83+
%% already in the map and not a binary so update
84+
update_attribute(Key, Value, Attributes=#attributes{map=Map}) when is_map_key(Key, Map) ->
85+
Attributes#attributes{map=Map#{Key := Value}};
86+
%% we've already started dropping, so just increment
87+
update_attribute(_Key, _Value, Attributes=#attributes{dropped=Dropped})
88+
when Dropped > 0 ->
89+
Attributes#attributes{dropped=Dropped + 1};
90+
%% met or exceeded the limit
91+
update_attribute(_Key, _Value, Attributes=#attributes{count_limit=CountLimit,
92+
dropped=Dropped,
93+
map=Map})
94+
when map_size(Map) >= CountLimit ->
95+
Attributes#attributes{dropped=Dropped + 1};
96+
%% new attribute
97+
update_attribute(Key, Value, Attributes=#attributes{map=Map}) ->
98+
Attributes#attributes{map=Map#{Key => Value}}.
8999

90-
%% if value is a primitive or a list/array then just return it
91-
is_allowed(Value, _) when is_atom(Value) ;
92-
is_integer(Value) ;
93-
is_float(Value) ;
94-
is_list(Value) ->
95-
{true, Value};
96-
%% if a binary then it is a string and we may need to trim its length
97-
is_allowed(Value, ValueLengthLimit) when is_binary(Value) ->
100+
maybe_truncate_binary(Value, infinity) ->
101+
Value;
102+
maybe_truncate_binary(Value, ValueLengthLimit) ->
98103
case string:length(Value) > ValueLengthLimit of
99104
true ->
100-
{true, string:slice(Value, 0, ValueLengthLimit)};
105+
string:slice(Value, 0, ValueLengthLimit);
101106
false ->
102-
{true, Value}
103-
end;
104-
is_allowed(_, _) ->
105-
false.
107+
Value
108+
end.

‎apps/opentelemetry/test/opentelemetry_SUITE.erl‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ all_cases() ->
3333
root_span_sampling_always_on, root_span_sampling_always_off,
3434
record_but_not_sample, record_exception_works, record_exception_with_message_works,
3535
propagator_configuration, propagator_configuration_with_os_env, force_flush,
36-
dropped_attributes, too_many_attributes].
36+
dropped_attributes, too_many_attributes, truncated_binary_attributes].
3737

3838
groups() ->
3939
[{otel_simple_processor, [], all_cases()},
@@ -693,13 +693,32 @@ dropped_attributes(Config) ->
693693
?set_current_span(SpanCtx),
694694

695695
?set_attribute(<<"attr-1">>, <<"attr-value-1">>),
696-
?set_attribute(<<"attr-2">>, {not_allowed, in, attributes}),
696+
?set_attribute(<<"attr-2">>, {non_homogeneous, <<"attribute">>}),
697697

698698
otel_span:end_span(SpanCtx),
699699
[Span] = assert_exported(Tid, SpanCtx),
700700

701701
?assertEqual(#{<<"attr-1">> => <<"at">>}, otel_attributes:map(Span#span.attributes)),
702-
?assertEqual(1, otel_attributes:dropped(Span#span.attributes)),
702+
703+
ok.
704+
705+
truncated_binary_attributes(_Config) ->
706+
InfinityLengthAttributes = otel_attributes:new(#{<<"attr-1">> => <<"abcde">>,
707+
<<"attr-2">> => [<<"a">>, <<"abcde">>, <<"abcde">>]},
708+
128, infinity),
709+
710+
%% when length limit is inifinity
711+
?assertMatch(#{<<"attr-1">> := <<"abcde">>,
712+
<<"attr-2">> := [<<"a">>, <<"abcde">>, <<"abcde">>]},
713+
otel_attributes:map(InfinityLengthAttributes)),
714+
715+
LengthLimitAttributes = otel_attributes:new(#{<<"attr-1">> => <<"abcde">>,
716+
<<"attr-2">> => [<<"a">>, <<"abcde">>, <<"abcde">>]},
717+
128, 2),
718+
% with default
719+
?assertMatch(#{<<"attr-1">> := <<"ab">>,
720+
<<"attr-2">> := [<<"a">>, <<"ab">>, <<"ab">>]},
721+
otel_attributes:map(LengthLimitAttributes)),
703722

704723
ok.
705724

@@ -709,33 +728,39 @@ too_many_attributes(Config) ->
709728

710729
?set_current_span(SpanCtx),
711730

712-
?set_attribute(<<"attr-1">>, <<"attr-value-1">>),
731+
%% tuple tests cover lists, as well.
732+
?set_attribute(attr1, {homogenous, tuple}),
713733

714-
%% dropped because of tuple as value
715-
?set_attribute(<<"attr-2">>, {not_allowed, in, attributes}),
734+
%% dropped because of non-homogenous
735+
?set_attribute(<<"attr-2-dropped">>, {non_homogenous, <<"attributes">>}),
716736

717-
?set_attribute(<<"attr-3">>, <<"attr-value-3">>),
737+
?set_attribute(<<"attr-3">>, attr_3_value),
718738

719739
%% dropped because count limit was set to 2
720740
?set_attribute(<<"attr-4">>, <<"attr-value-4">>),
721741
%% won't be dropped because attr-1 already exists so it overrides the value
722742
?set_attribute(<<"attr-1">>, <<"attr-value-5">>),
743+
%% already exists and not a binary, so skips the binary length check.
744+
?set_attribute(<<"attr-3">>, 4),
745+
746+
%% dropping skips all checks now that we're already dropping attrs
747+
?set_attribute(<<"attr-6">>, <<"attr-value-6">>),
723748

724749
otel_span:end_span(SpanCtx),
725750
[Span] = assert_exported(Tid, SpanCtx),
726751

727-
?assertEqual(#{<<"attr-1">> => <<"attr-value-5">>,
728-
<<"attr-3">> => <<"attr-value-3">>}, otel_attributes:map(Span#span.attributes)),
729-
?assertEqual(2, otel_attributes:dropped(Span#span.attributes)),
752+
?assertEqual(#{attr1 => [homogenous, tuple],
753+
<<"attr-3">> => 4}, otel_attributes:map(Span#span.attributes)),
754+
?assertEqual(3, otel_attributes:dropped(Span#span.attributes)),
730755

731756
%% test again using the `set_attributes' macro
732757
SpanCtx2 = ?start_span(<<"span-2">>),
733758

734759
?set_current_span(SpanCtx2),
735760

736761
?set_attributes(#{<<"attr-1">> => <<"attr-value-1">>,
737-
<<"attr-2">> => {not_allowed, in, attributes},
738-
<<"attr-3">> => <<"attr-value-3">>,
762+
<<"attr-2">> => {homogenous, attribute},
763+
<<"attr-3">> => attr_3_value,
739764
<<"attr-4">> => <<"attr-value-4">>}),
740765

741766
%% won't be dropped because attr-1 already exists so it overrides the value
@@ -744,6 +769,7 @@ too_many_attributes(Config) ->
744769
otel_span:end_span(SpanCtx2),
745770
[Span2] = assert_exported(Tid, SpanCtx2),
746771

772+
747773
%% order isn't guaranteed so just verify the number dropped is right
748774
?assertEqual(2, otel_attributes:dropped(Span2#span.attributes)),
749775

‎apps/opentelemetry_api/lib/open_telemetry.ex‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ defmodule OpenTelemetry do
4646
contiguous - there may be gaps or overlaps between spans in a trace.
4747
"""
4848
@type span() :: :opentelemetry.span()
49-
5049
@type span_kind() :: :opentelemetry.span_kind()
50+
@type span_name() :: :opentelemetry.span_name()
5151

5252
@typedoc """
5353
TraceId is a unique identifier for a trace. All spans from the same trace share
@@ -65,6 +65,7 @@ defmodule OpenTelemetry do
6565

6666
@type attribute_key() :: :opentelemetry.attribute_key()
6767
@type attribute_value() :: :opentelemetry.attribute_value()
68+
@type attribute() :: :opentelemetry.attribute()
6869

6970
@typedoc """
7071
Attributes are a collection of key/value pairs. The value can be a string,

‎apps/opentelemetry_api/lib/open_telemetry/span.ex‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ defmodule OpenTelemetry.Span do
2727
- A Status (`t:OpenTelemetry.status/0`)
2828
"""
2929

30+
@type start_opts() :: :otel_span.start_opts()
31+
3032
@doc """
3133
Get the SpanId of a Span.
3234
"""
@@ -164,6 +166,6 @@ defmodule OpenTelemetry.Span do
164166
and may lead to re-calculation of sampling or filtering decisions made previously
165167
depending on the implementation.
166168
"""
167-
@spec update_name(OpenTelemetry.span_ctx(), String.t()) :: boolean()
169+
@spec update_name(OpenTelemetry.span_ctx(), OpenTelemetry.span_name()) :: boolean()
168170
defdelegate update_name(span_ctx, name), to: :otel_span
169171
end

‎apps/opentelemetry_api/lib/open_telemetry/tracer.ex‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,6 @@ defmodule OpenTelemetry.Tracer do
1616
end
1717
"""
1818

19-
@type start_opts() :: %{
20-
optional(:attributes) => OpenTelemetry.attributes_map(),
21-
optional(:links) => [OpenTelemetry.link()],
22-
optional(:is_recording) => boolean(),
23-
optional(:start_time) => :opentelemetry.timestamp(),
24-
optional(:kind) => OpenTelemetry.span_kind()
25-
}
26-
2719
@doc """
2820
Starts a new span and does not make it the current active span of the current process.
2921

‎apps/opentelemetry_api/src/opentelemetry.erl‎

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
link/0,
7474
attribute_key/0,
7575
attribute_value/0,
76+
attribute/0,
7677
attributes_map/0,
7778
event/0,
7879
event_name/0,
@@ -101,20 +102,23 @@
101102
-type span_name() :: unicode:unicode_binary() | atom().
102103

103104
-type attribute_key() :: unicode:unicode_binary() | atom().
104-
-type attribute_value() :: unicode:unicode_binary() |
105-
float() |
106-
integer() |
107-
[unicode:unicode_binary() | float() | integer()].
105+
-type attribute_value() :: unicode:unicode_binary() |
106+
atom() |
107+
number() |
108+
boolean() |
109+
[unicode:unicode_binary() | atom() | float() | integer() | boolean()] |
110+
{unicode:unicode_binary() | atom() | float() | integer() | boolean()}.
111+
-type attribute() :: {attribute_key(), attribute_value()}.
108112
-type attributes_map() :: #{attribute_key() => attribute_value()} |
109-
[{attribute_key(), attribute_value()}].
113+
[attribute()].
110114

111115
-type span_kind() :: ?SPAN_KIND_INTERNAL |
112116
?SPAN_KIND_SERVER |
113117
?SPAN_KIND_CLIENT |
114118
?SPAN_KIND_PRODUCER |
115119
?SPAN_KIND_CONSUMER.
116120
-type event() :: #{system_time_nano => non_neg_integer(),
117-
name := unicode:unicode_binary(),
121+
name := event_name(),
118122
attributes := attributes_map()}.
119123
-type event_name() :: unicode:unicode_binary() | atom().
120124
-type link() :: #{trace_id := trace_id(),
@@ -286,13 +290,13 @@ links(List) when is_list(List) ->
286290
lists:filtermap(fun({TraceId, SpanId, Attributes, TraceState}) when is_integer(TraceId) ,
287291
is_integer(SpanId) ,
288292
is_list(TraceState) ->
289-
link_or_false(TraceId, SpanId, Attributes, TraceState);
293+
link_or_false(TraceId, SpanId, otel_span:process_attributes(Attributes), TraceState);
290294
({#span_ctx{trace_id=TraceId,
291295
span_id=SpanId,
292296
tracestate=TraceState}, Attributes}) when is_integer(TraceId) ,
293297
is_integer(SpanId) ,
294298
is_list(TraceState) ->
295-
link_or_false(TraceId, SpanId, Attributes, TraceState);
299+
link_or_false(TraceId, SpanId, otel_span:process_attributes(Attributes), TraceState);
296300
(#span_ctx{trace_id=TraceId,
297301
span_id=SpanId,
298302
tracestate=TraceState}) when is_integer(TraceId) ,
@@ -313,7 +317,7 @@ link(SpanCtx) ->
313317
link(#span_ctx{trace_id=TraceId,
314318
span_id=SpanId,
315319
tracestate=TraceState}, Attributes) ->
316-
?MODULE:link(TraceId, SpanId, Attributes, TraceState);
320+
?MODULE:link(TraceId, SpanId, otel_span:process_attributes(Attributes), TraceState);
317321
link(_, _) ->
318322
undefined.
319323

@@ -328,45 +332,45 @@ link(TraceId, SpanId, Attributes, TraceState) when is_integer(TraceId),
328332
is_list(TraceState) ->
329333
#{trace_id => TraceId,
330334
span_id => SpanId,
331-
attributes => Attributes,
335+
attributes => otel_span:process_attributes(Attributes),
332336
tracestate => TraceState};
333337
link(_, _, _, _) ->
334338
undefined.
335339

336340
-spec event(Name, Attributes) -> event() | undefined when
337-
Name :: unicode:unicode_binary(),
341+
Name :: event_name(),
338342
Attributes :: attributes_map().
339-
event(Name, Attributes) when is_binary(Name),
340-
(is_list(Attributes) orelse is_map(Attributes)) ->
341-
event(erlang:system_time(nanosecond), Name, Attributes);
342-
event(_, _) ->
343-
undefined.
343+
event(Name, Attributes) ->
344+
event(erlang:system_time(nanosecond), Name, Attributes).
344345

345346
-spec event(Timestamp, Name, Attributes) -> event() | undefined when
346347
Timestamp :: non_neg_integer(),
347-
Name :: unicode:unicode_binary(),
348+
Name :: event_name(),
348349
Attributes :: attributes_map().
349350
event(Timestamp, Name, Attributes) when is_integer(Timestamp),
350-
is_binary(Name),
351351
(is_list(Attributes) orelse is_map(Attributes)) ->
352-
#{system_time_nano => Timestamp,
353-
name => Name,
354-
attributes => Attributes};
352+
353+
case otel_span:is_valid_name(Name) of
354+
true ->
355+
#{system_time_nano => Timestamp,
356+
name => Name,
357+
attributes => otel_span:process_attributes(Attributes)};
358+
false ->
359+
undefined
360+
end;
355361
event(_, _, _) ->
356362
undefined.
357363

358364
events(List) ->
359365
Now = erlang:system_time(nanosecond),
360-
lists:filtermap(fun({Time, Name, Attributes}) when is_binary(Name),
361-
(is_list(Attributes) orelse is_map(Attributes)) ->
366+
lists:filtermap(fun({Time, Name, Attributes}) ->
362367
case event(Time, Name, Attributes) of
363368
undefined ->
364369
false;
365370
Event ->
366371
{true, Event}
367372
end;
368-
({Name, Attributes}) when is_binary(Name),
369-
(is_list(Attributes) orelse is_map(Attributes)) ->
373+
({Name, Attributes}) ->
370374
case event(Now, Name, Attributes) of
371375
undefined ->
372376
false;
@@ -449,7 +453,7 @@ vsn_to_binary(Vsn) when is_binary(Vsn) ; is_list(Vsn) ->
449453
vsn_to_binary(_) ->
450454
<<>>.
451455

452-
%% name can be atom, list or binary. But atom `undefined'
456+
%% Instrumentation name can be atom, list or binary. But atom `undefined'
453457
%% must stay as `undefined' atom.
454458
name_to_binary(undefined)->
455459
undefined;

0 commit comments

Comments
 (0)