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 Elixir modules for baggage and context #121

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

tsloughter
Copy link
Member

Resolves #120

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #121 into master will increase coverage by 0.10%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   32.01%   32.12%   +0.10%     
==========================================
  Files          61       63       +2     
  Lines        2927     2945      +18     
==========================================
+ Hits          937      946       +9     
- Misses       1990     1999       +9     
Flag Coverage Δ
#api 50.00% <68.42%> (+0.28%) ⬆️
#elixir 16.84% <68.42%> (+3.99%) ⬆️
#erlang 31.85% <25.00%> (-0.18%) ⬇️
#exporter 17.05% <ø> (ø)
#sdk 63.86% <ø> (-0.15%) ⬇️

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

Impacted Files Coverage Δ
apps/opentelemetry_api/src/otel_ctx.erl 83.33% <ø> (-8.34%) ⬇️
...ps/opentelemetry_api/lib/open_telemetry/baggage.ex 60.00% <60.00%> (ø)
apps/opentelemetry_api/lib/open_telemetry/ctx.ex 70.00% <70.00%> (ø)
apps/opentelemetry_api/src/otel_baggage.erl 81.48% <75.00%> (-1.86%) ⬇️
apps/opentelemetry/src/otel_batch_processor.erl 72.41% <0.00%> (-1.15%) ⬇️

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 5b002ce...919d552. Read the comment docs.

@hauleth
Copy link
Member

hauleth commented Oct 10, 2020

Out of interest. Why do we implement such modules that are fully defdelegate. I would understand situation when we need to have some changes to be made to make it usable with Elixir, like defining Elixir macros or changing order of arguments. `in this case I feel that it is a little bit unneeded in most cases, as calling Erlang functions is pretty straightforward.

@tsloughter
Copy link
Member Author

Because modules like Tracer and the metrics modules exist that do do more than just defdelegate. It was discussed at some point that it is better than just having some modules with Elixir counterpart to have all.

The SDK has no Elixir modules because they would all just be defdelegates, but the API feels more consistent if you aren't mixing different types of module calls.

@tsloughter tsloughter merged commit 4c54edf into open-telemetry:master Oct 15, 2020
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.

Add Elixir Context module
3 participants