Skip to content

Fix otel_propagator_trace_context:extract/1 missing undefined check #290

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

Merged

Conversation

dvic
Copy link
Contributor

@dvic dvic commented Oct 13, 2021

No description provided.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #290 (ec2335b) into main (f58196f) will increase coverage by 8.46%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
+ Coverage   37.33%   45.80%   +8.46%     
==========================================
  Files          46       43       -3     
  Lines        3254     1109    -2145     
==========================================
- Hits         1215      508     -707     
+ Misses       2039      601    -1438     
Flag Coverage Δ
api 14.25% <0.00%> (-51.32%) ⬇️
elixir 14.25% <0.00%> (-0.06%) ⬇️
erlang 77.29% <ø> (+39.95%) ⬆️
exporter ?
sdk 77.29% <ø> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
...elemetry_api/src/otel_propagator_trace_context.erl 0.00% <0.00%> (-52.18%) ⬇️
apps/opentelemetry_api/src/otel_propagator.erl 0.00% <0.00%> (-87.50%) ⬇️
.../opentelemetry_api/src/otel_propagator_b3multi.erl 0.00% <0.00%> (-86.67%) ⬇️
...opentelemetry_api/src/otel_propagator_b3single.erl 0.00% <0.00%> (-85.72%) ⬇️
...pps/opentelemetry_api/src/otel_tracer_provider.erl 0.00% <0.00%> (-69.57%) ⬇️
...opentelemetry_api/src/otel_propagator_text_map.erl 0.00% <0.00%> (-68.58%) ⬇️
...try_api/src/otel_propagator_text_map_composite.erl 0.00% <0.00%> (-66.67%) ⬇️
apps/opentelemetry_api/src/opentelemetry.erl 10.00% <0.00%> (-66.25%) ⬇️
apps/opentelemetry_api/src/otel_propagator_b3.erl 0.00% <0.00%> (-63.64%) ⬇️
.../opentelemetry_api/src/otel_propagator_baggage.erl 0.00% <0.00%> (-63.24%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f58196f...ec2335b. Read the comment docs.

@dvic
Copy link
Contributor Author

dvic commented Oct 13, 2021

Not sure how to properly test this with the try/catch there, also it was hard to figure out what the error was locally in an Elixir project. Is the original error / stacktrace properly logged? I'm not that familiar with the :logger module.

The try/catch I'm talking about is this one: https://github.com/open-telemetry/opentelemetry-erlang/blob/main/apps/opentelemetry_api/src/otel_propagator_text_map_composite.erl#L68

@tsloughter
Copy link
Member

Thanks! You could add a test in otel_propagators_SUITE.erl in the rewrite function where it just calls the trace context propagation module directly.

@dvic
Copy link
Contributor Author

dvic commented Oct 13, 2021

Thanks! You could add a test in otel_propagators_SUITE.erl in the rewrite function where it just calls the trace context propagation module directly.

Ah thanks! Done :)

@dvic dvic force-pushed the fix-extract-span-ctx-from-string branch from 746a487 to 17f9f2d Compare October 13, 2021 18:14
@bryannaegele bryannaegele merged commit b41aee5 into open-telemetry:main Oct 13, 2021
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.

3 participants