Add b3multi option to OTEL_PROPAGATORS

[indrekj]

According to OTEL Spec there are b3 and b3multi. The latter is
supposed to be used when Multi B3 format is used. This is exactly
what b3_propagators() currently is doing.

[codecov]

Codecov Report

Merging #272 (6942f08) into main (f0cf709) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 6942f08 differs from pull request most recent head e98fc2d. Consider uploading reports for the commit e98fc2d to get more accurate results

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   36.50%   36.41%   -0.10%     
==========================================
  Files          41       41              
  Lines        3169     3166       -3     
==========================================
- Hits         1157     1153       -4     
- Misses       2012     2013       +1     

Flag	Coverage Δ
api	63.06% <ø> (-0.32%)	⬇️
elixir	15.98% <ø> (-0.72%)	⬇️
erlang	36.41% <100.00%> (-0.06%)	⬇️
exporter	19.60% <ø> (ø)
sdk	79.13% <100.00%> (-0.15%)	⬇️

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

Impacted Files	Coverage Δ
apps/opentelemetry/src/otel_configuration.erl	78.87% <100.00%> (+0.30%)	⬆️
apps/opentelemetry_api/lib/open_telemetry/span.ex	18.75% <0.00%> (-9.03%)	⬇️
apps/opentelemetry_api/src/otel_span.erl	74.07% <0.00%> (-1.79%)	⬇️
apps/opentelemetry/src/otel_resource_detector.erl	91.42% <0.00%> (-1.43%)	⬇️
apps/opentelemetry_api/src/opentelemetry.erl	74.19% <0.00%> (ø)

---

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 f0cf709...e98fc2d. Read the comment docs.

[tsloughter]

Thanks. I was afraid of this being the case :).

I think this PR should break use of b3 to ensure those using it must update to setting multib3 and we can in the future include the single header b3.

[indrekj]

I think this PR should break use of b3 to ensure those using it must update to setting multib3 and we can in the future include the single header b3.

Currently, if you use OTEL_PROPAGATORS="b3" then there's a warning on application start:

[warn] Ignoring uknown propagator b3 in OS environment variable $OTEL_PROPAGATORS

So basically it doesn't affect existing b3 users as it didn't work before either. This PR just adds support to OTEL_PROPAGATORS="b3multi" without changing anything else.

Do you think that's fine or do you think something should be changed here?

[tsloughter]

Ooh, my bad, I was taking into account my changes in the proapgators rewrite PR which has some extra lines about b3/multib3.

This should be fine.

[indrekj]

@tsloughter Pushed https://github.com/open-telemetry/opentelemetry-erlang/compare/a5b7fccbca09f49b2f6143b69410b38929dbe5ad..b3fd84be88b052041f94ef6e93470b23fe57ddbd which should fix the test coverage check, but workflow has to be approved

[indrekj]

🤦 missed to include the test: https://github.com/open-telemetry/opentelemetry-erlang/compare/b3fd84be88b052041f94ef6e93470b23fe57ddbd..42b17a65b0a6a7c0714be75727d9fc739a0d6535

[indrekj]

It seems that this doesn't work exactly as I expected.

So the propagators are evaluated correctly. However, in src/opentelemetry_app.erl there's:

setup_text_map_propagators(Opts) ->
    Propagators = proplists:get_value(text_map_propagators, Opts, []),
    ...

Which uses text_map_propagators, instead of propagators, so setting the env variable doesn't seem to have any effect.

start(_StartType, _StartArgs) ->
    Opts = otel_configuration:merge_with_os(
             application:get_all_env(opentelemetry)),

basically looks:

    %% [
    %%  {text_map_propagators,[fun otel_baggage:get_text_map_propagators/0,fun otel_tracer_default:w3c_propagators/0]},
    %%  {propagators,[fun otel_tracer_default:b3_propagators/0]},
    %%  {traces_exporter,{opentelemetry_exporter,#{}}}]

I'm not really what's the best way to fix this. Rename propagators to text_map_propagators?

[tsloughter]

Oh, where is propagators used? It should always be text_map_propagators except in that transform function.

[indrekj]

The issue is that it is not used. Basically, propagators sets it to b3 (when using env var) but then that is just ignored. So even though I use b3multi env variable, everything is still propagated using w3c trace context. Currently, it seems that OTEL_PROPAGATORS is basically a no-op. At least that's how I understand this.

[indrekj]

@tsloughter I added a commit that fixes the issue I mentioned: 6942f08 . I have no idea if this is the actual approach you'd want to use but it should be backwards compatible and it does work.

[tsloughter]

I think this is good.

[indrekj]

@tsloughter I rebased against main branch but it says code cov reduced 0.02%. Would it be possible to access codecov to see what exactly it means? I'm getting Unauthorized when accessing https://app.codecov.io/gh/open-telemetry/opentelemetry-erlang/compare/272

[tsloughter]

@indrekj the codecov is really screwed up and needs to be fixed, it is not accurate right now. So I would say don't worry about it, but if you want and have the time you are welcome to try fixing it ;).

It looks like I have to add you to the github team to give you access, that might have to go through someone else since its a cncf project.

[indrekj]

@tsloughter do you think this could be merged in this case? Or is there anything else I need to do?

[tsloughter]

Hey, yea, sorry about that, I thought I'd get the other propagators change in much earlier and thought merging it with the other PR would be easier than the other way around. I'll merge this now since that wasn't the case.