Skip to content
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

make trace_id and span_id types binaries instead of integers #590

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

make trace_id and span_id types binaries instead of integers #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bc3e909
update API to use binary trace and span id
tsloughter May 16, 2023
d731ba8
update SDK to use binary trace and span id
tsloughter May 16, 2023
629f5cf
use OTP-26 encode_hex/2 when available
tsloughter May 17, 2023
a5a3822
fix zipkin exporter for use of binary trace/span ids
tsloughter May 17, 2023
371b2cb
update sampler trace id ratio based tests for binary ids
tsloughter May 18, 2023
f541f41
fix elixir trace_id/span_id types to reference the Erlang types
tsloughter May 18, 2023
fcf0b9a
expand trace id ratio based tests
tsloughter May 18, 2023
6137911
add test of propagator fields function
tsloughter May 18, 2023
e3ac4be
cleanup b3single code to remove unnecessary guards
tsloughter May 18, 2023
1d0d7c5
update changelog for PR-590
tsloughter May 18, 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
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## API

### Changed

- [`trace_id` and `span_id` now represented as binaries instead of
integers](https://github.com/open-telemetry/opentelemetry-erlang/pull/590/)

## SDK

### Added
Expand Down
4 changes: 2 additions & 2 deletions apps/opentelemetry/src/otel_id_generator.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ generate_span_id(Module) ->
%% @doc Generates a 128 bit random integer to use as a trace id.
-spec generate_trace_id() -> opentelemetry:trace_id().
generate_trace_id() ->
rand:uniform(?assert_type(2 bsl 127 - 1, pos_integer())). %% 2 shifted left by 127 == 2 ^ 128
rand:bytes(16).

%% @doc Generates a 64 bit random integer to use as a span id.
-spec generate_span_id() -> opentelemetry:span_id().
generate_span_id() ->
rand:uniform(?assert_type(2 bsl 63 - 1, pos_integer())). %% 2 shifted left by 63 == 2 ^ 64
rand:bytes(8).
4 changes: 2 additions & 2 deletions apps/opentelemetry/src/otel_links.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ create_links([L | Rest], Limit, AttributePerLinkLimit, AttributeValueLengthLimit
%%

new_link({TraceId, SpanId, Attributes, TraceState}, AttributePerLinkLimit, AttributeValueLengthLimit)
when is_integer(TraceId),
is_integer(SpanId),
when is_binary(TraceId),
is_binary(SpanId),
(is_list(Attributes) orelse is_map(Attributes)),
is_list(TraceState) ->
#link{trace_id=TraceId,
Expand Down
4 changes: 2 additions & 2 deletions apps/opentelemetry/src/otel_resource_detector.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ add_service_instance(Resource) ->
false ->
case erlang:node() of
nonode@nohost ->
ServiceInstanceId = otel_id_generator:generate_trace_id(),
ServiceInstanceId = otel_utils:encode_hex(otel_id_generator:generate_trace_id()),
ServiceInstanceResource = otel_resource:create([{?SERVICE_INSTANCE_ID, ServiceInstanceId}]),
otel_resource:merge(ServiceInstanceResource, Resource);
ServiceInstance ->
Expand All @@ -263,7 +263,7 @@ add_service_instance(Resource) ->
ServiceInstanceResource = otel_resource:create([{?SERVICE_INSTANCE_ID, ServiceInstance1}]),
otel_resource:merge(ServiceInstanceResource, Resource);
_Match ->
ServiceInstanceId = otel_id_generator:generate_trace_id(),
ServiceInstanceId = otel_utils:encode_hex(otel_id_generator:generate_trace_id()),
ServiceInstanceResource = otel_resource:create([{?SERVICE_INSTANCE_ID, ServiceInstanceId}]),
otel_resource:merge(ServiceInstanceResource, Resource)
end
Expand Down
5 changes: 2 additions & 3 deletions apps/opentelemetry/src/otel_sampler_trace_id_ratio_based.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,8 @@ decide(undefined, _IdUpperBound) ->
?DROP;
decide(0, _IdUpperBound) ->
?DROP;
decide(TraceId, IdUpperBound) ->
Lower64Bits = TraceId band ?MAX_VALUE,
case erlang:abs(Lower64Bits) < IdUpperBound of
decide(<<_:65, LowerBits:63>>, IdUpperBound) ->
Copy link
Member Author

Choose a reason for hiding this comment

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

I uh.. don't know why this is only 63 bits.. I just know that is what equaled the value I'd get with TraceId band ?MAX_VALUE which I copied from somewhere else..

Copy link
Member

Choose a reason for hiding this comment

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

So is there any kind of spec explaining this specific behavior? I guess this is just because ?MAX_VALUE is the max 64 bits signed integer value (2^63-1) so that's unsurprising you'd look at the 63 lower bits, but I have no idea where the MAX_VALUE requirement comes from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the sign!

Trying to find now where it comes from.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may need to change (not in this PR) because the w3c spec now specifies 56 bits to be random if the new random trace id flag is set: https://w3c.github.io/trace-context/#random-trace-id-flag

case erlang:abs(LowerBits) < IdUpperBound of
true -> ?RECORD_AND_SAMPLE;
false -> ?DROP
end.
16 changes: 8 additions & 8 deletions apps/opentelemetry/test/opentelemetry_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,8 @@ shutdown_sdk_noop_spans(_Config) ->

SpanCtx2 = ?start_span(<<"span-2">>),

?assertMatch(#span_ctx{trace_id=0,
span_id=0}, SpanCtx2),
?assertMatch(#span_ctx{trace_id = <<0:128>>,
span_id = <<0:64>>}, SpanCtx2),

?assertEqual(false, otel_span:set_attribute(SpanCtx1, a, 1)),

Expand Down Expand Up @@ -784,17 +784,17 @@ stop_temporary_app(_Config) ->
Tracer = opentelemetry:get_tracer(),

SpanCtx1 = ?start_span(<<"span-1">>),
?assertNotMatch(#span_ctx{trace_id=0,
span_id=0}, SpanCtx1),
?assertNotMatch(#span_ctx{trace_id = <<0:128>>,
span_id = <<0:64>>}, SpanCtx1),

ok = application:stop(opentelemetry),

%% stopping opentelemetry deletes the active span ETS table
%% so eventually newly started spans will be no-ops because
%% inserting a new span will fail
?UNTIL(case ?start_span(<<"span-2">>) of
#span_ctx{trace_id=0,
span_id=0} ->
#span_ctx{trace_id = <<0:128>>,
span_id = <<0:64>>} ->
true;
_ ->
false
Expand Down Expand Up @@ -1074,8 +1074,8 @@ too_many_attributes(Config) ->
disabled_sdk(_Config) ->
SpanCtx1 = ?start_span(<<"span-1">>),

?assertMatch(#span_ctx{trace_id=0,
span_id=0}, SpanCtx1),
?assertMatch(#span_ctx{trace_id = <<0:128>>,
span_id = <<0:64>>}, SpanCtx1),
ok.

no_exporter(_Config) ->
Expand Down
5 changes: 3 additions & 2 deletions apps/opentelemetry/test/otel_propagation_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ propagation(Config) ->
?assertMatch(#span_ctx{trace_flags=1}, ?current_span_ctx),
?assertMatch(#span_ctx{is_recording=true}, ?current_span_ctx),

?assertMatch([_ | _], otel_propagator_text_map:fields(opentelemetry:get_text_map_injector())),

otel_baggage:set("key-1", <<"value=1">>, []),
%% TODO: should the whole baggage entry be dropped if metadata is bad?
Expand All @@ -77,8 +78,8 @@ propagation(Config) ->

Headers = otel_propagator_text_map:inject([{<<"existing-header">>, <<"I exist">>}]),

EncodedTraceId = io_lib:format("~32.16.0b", [TraceId]),
EncodedSpanId = io_lib:format("~16.16.0b", [SpanId]),
EncodedTraceId = otel_utils:encode_hex(TraceId),
EncodedSpanId = otel_utils:encode_hex(SpanId),

?assertListsEqual([{<<"baggage">>, <<"key-2=value-2;metadata;md-k-1=md-v-1,key-1=value%3D1">>},
{<<"existing-header">>, <<"I exist">>} |
Expand Down
184 changes: 121 additions & 63 deletions apps/opentelemetry/test/otel_samplers_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ get_description(_Config) ->
trace_id_ratio_based(_Config) ->
SpanName = <<"span-prob-sampled">>,
Probability = 0.5,
DoSample = 120647249294066572380176333851662846319,
DoNotSample = 53020601517903903921384168845238205400,
DoSample = <<120647249294066572380176333851662846319:128>>,
DoNotSample = <<53020601517903903921384168845238205400:128>>,

Ctx = otel_ctx:new(),

Expand All @@ -58,90 +58,148 @@ trace_id_ratio_based(_Config) ->

%% checks the trace id is under the upper bound
?assertMatch(
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoSample,
[],
SpanName,
undefined,
[],
Opts
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoSample,
[],
SpanName,
undefined,
[],
Opts
)
),
),

%% checks the trace id is is over the upper bound
?assertMatch(
{?DROP, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoNotSample,
[],
SpanName,
undefined,
[],
Opts
{?DROP, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoNotSample,
[],
SpanName,
undefined,
[],
Opts
)
),
),

%% ignores the parent span context trace flags
?assertMatch(
{?DROP, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 1,
is_remote = true
}),
DoNotSample,
[],
SpanName,
undefined,
[],
Opts
{?DROP, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 1,
is_remote = true
}),
DoNotSample,
[],
SpanName,
undefined,
[],
Opts
)
),
),

%% ignores the parent span context trace flags
?assertMatch(
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 0,
is_remote = false
}),
DoSample,
[],
SpanName,
undefined,
[],
Opts
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 0,
is_remote = false
}),
DoSample,
[],
SpanName,
undefined,
[],
Opts
)
),
),

%% trace id is under the upper bound
?assertMatch(
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 0,
is_remote = true
}),
DoSample,
[],
SpanName,
undefined,
[],
Opts
{?RECORD_AND_SAMPLE, [], []},
Sampler:should_sample(
otel_tracer:set_current_span(Ctx, #span_ctx{
trace_flags = 0,
is_remote = true
}),
DoSample,
[],
SpanName,
undefined,
[],
Opts
)
),
),

%% everything is dropped
{ZeroSampler, _, ZeroOpts} = otel_sampler:new({trace_id_ratio_based, 0.0}),
?assertMatch(
{?DROP, [], []},
ZeroSampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoNotSample,
[],
SpanName,
undefined,
[],
ZeroOpts
)
),

%% everything is sampled
{OneSampler, _, OneOpts} = otel_sampler:new({trace_id_ratio_based, 1.0}),
?assertMatch(
{?RECORD_AND_SAMPLE, [], []},
OneSampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
DoSample,
[],
SpanName,
undefined,
[],
OneOpts
)
),

%% drop for undefined trace id
?assertMatch(
{?DROP, [], []},
OneSampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
undefined,
[],
SpanName,
undefined,
[],
OneOpts
)
),

%% drop for 0 trace id
?assertMatch(
{?DROP, [], []},
OneSampler:should_sample(
otel_tracer:set_current_span(Ctx, undefined),
undefined,
[],
SpanName,
undefined,
[],
OneOpts
)
),

ok.

parent_based(_Config) ->
SpanName = <<"span-prob-sampled">>,
Probability = 0.5,
DoSample = 120647249294066572380176333851662846319,
DoNotSample = 53020601517903903921384168845238205400,
DoSample = <<120647249294066572380176333851662846319:128>>,
DoNotSample = <<53020601517903903921384168845238205400:128>>,

Ctx = otel_ctx:new(),

Expand Down
6 changes: 3 additions & 3 deletions apps/opentelemetry_api/lib/open_telemetry.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ defmodule OpenTelemetry do
the same `trace_id`. The ID is a 16-byte array. An ID with all zeroes
is considered invalid.
"""
@type trace_id() :: non_neg_integer()
@type trace_id() :: :opentelemetry.trace_id()

@typedoc """
SpanId is a unique identifier for a span within a trace, assigned when the span
is created. The ID is an 8-byte array. An ID with all zeroes is considered
invalid.
"""
@type span_id() :: non_neg_integer()
@type span_id() :: :opentelemetry.span_id()

@type attribute_key() :: :opentelemetry.attribute_key()
@type attribute_value() :: :opentelemetry.attribute_value()
Expand Down Expand Up @@ -167,7 +167,7 @@ defmodule OpenTelemetry do
Creates a list of `t:link/0` from a list of 4-tuples.
"""
@spec links([
{integer(), integer(), attributes_map(), tracestate()}
{trace_id(), span_id(), attributes_map(), tracestate()}
| span_ctx()
| {span_ctx(), attributes_map()}
]) :: [link()]
Expand Down
Loading