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.

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

add tracestate module and record #607

Merged
merged 5 commits into from
Aug 27, 2023

Conversation

tsloughter
Copy link
Member

tracestate is now a record with members element which are validated when added to the tracestate.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch coverage: 85.36% and project coverage change: +0.27% 🎉

Comparison is base (3ed777a) 72.26% compared to head (d34817e) 72.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
+ Coverage   72.26%   72.54%   +0.27%     
==========================================
  Files          60       61       +1     
  Lines        1904     1923      +19     
==========================================
+ Hits         1376     1395      +19     
  Misses        528      528              
Flag Coverage Δ
api 69.05% <86.84%> (+0.98%) ⬆️
elixir 17.98% <2.63%> (-0.34%) ⬇️
erlang 73.85% <85.36%> (+0.27%) ⬆️
exporter 66.66% <ø> (-0.14%) ⬇️
sdk 78.07% <66.66%> (-0.07%) ⬇️
zipkin 54.16% <ø> (ø)

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

Files Changed Coverage Δ
apps/opentelemetry/src/otel_links.erl 62.50% <ø> (ø)
apps/opentelemetry/src/otel_span_utils.erl 90.62% <ø> (ø)
apps/opentelemetry_api/lib/open_telemetry.ex 26.66% <ø> (ø)
apps/opentelemetry_api/src/otel_tracer_noop.erl 90.90% <ø> (ø)
...ps/opentelemetry_exporter/src/otel_otlp_traces.erl 83.87% <ø> (-0.51%) ⬇️
apps/opentelemetry/src/otel_sampler.erl 90.00% <66.66%> (-10.00%) ⬇️
apps/opentelemetry_api/src/otel_tracestate.erl 85.29% <85.29%> (ø)
apps/opentelemetry_api/src/opentelemetry.erl 81.81% <100.00%> (+0.18%) ⬆️
...elemetry_api/src/otel_propagator_trace_context.erl 74.19% <100.00%> (+3.36%) ⬆️
apps/opentelemetry_api/src/otel_span.erl 75.53% <100.00%> (ø)

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

tracestate is now a record with members element which are validated
when added to the tracestate.
%% for the limits and string requirements that make up the regexes
-define(MAX_MEMBERS, 32).
-define(KEY_MP, element(2, re:compile("^([a-z][_0-9a-z\-\*\/]{0,255})|([a-z0-9][_0-9a-z-*/]{0,240}@[a-z][_0-9a-z-*/]{0,13})$"))).
-define(VALUE_MP, element(2, re:compile("^([ -+--<>-~]{0,255}[!-+--<>-~])$"))).
Copy link
Member

Choose a reason for hiding this comment

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

not a blocker here, but since you use ?KEY_MP and ?VALUE_MP directly in the calls, I believe the recompilation will happen every time and provide limited benefits.

I realize this is code moved from another module and not a blocker, but I wanted to point it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh. Should I replace it with like:

-define(VALUE_MP, {re_pattern,1,0,0,
                <<69,82,67,80,152,0,0,0,16,0,0,0,1,0,0,0,255,255,255,
                  255,255,255,255,255,0,0,0,0,0,0,1,0,0,0,64,0,0,0,0,0,
                  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,131,0,
                  84,27,133,0,76,0,1,110,0,0,0,0,255,239,255,223,255,
                  255,255,255,255,255,255,127,0,0,0,0,0,0,0,0,0,0,0,0,0,
                  0,0,0,104,0,0,0,255,110,0,0,0,0,254,239,255,223,255,
                  255,255,255,255,255,255,127,0,0,0,0,0,0,0,0,0,0,0,0,0,
                  0,0,0,120,0,76,25,120,0,84,0>>}).

No idea how often or what versions that would break on. Maybe rare to never?

Copy link
Member

Choose a reason for hiding this comment

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

tests should catch it I imagine

Copy link
Member Author

Choose a reason for hiding this comment

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

true

parse_pairs([Pair | Rest], Acc) ->
case split(string:trim(Pair)) of
{K, V} ->
case re:run(K, ?KEY_MP) =/= nomatch
Copy link
Member

Choose a reason for hiding this comment

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

given what was pointed out earlier, it may make more sense to pass ?KEY_MP and ?VALUE_MP as parameters to this function so they're compiled once per list, particularly if there's no other caching (pdict or persistent terms) to save the easy re-compiling at each iteration.

@tsloughter tsloughter merged commit 73b4f13 into open-telemetry:main Aug 27, 2023
@tsloughter tsloughter deleted the tracestate-support branch August 27, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants