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

Added B3Propagator for B3 Single Header #691

Merged

Conversation

kishannsangani
Copy link
Member

Missed out implementing B3 Single Header in #680.

Closes #500.

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #691 (00e281e) into main (74eda3d) will increase coverage by 0.38%.
The diff coverage is 99.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #691      +/-   ##
============================================
+ Coverage     80.37%   80.75%   +0.38%     
- Complexity     1766     1805      +39     
============================================
  Files           222      225       +3     
  Lines          4520     4615      +95     
============================================
+ Hits           3633     3727      +94     
- Misses          887      888       +1     
Flag Coverage Δ
7.4 80.75% <99.05%> (+0.38%) ⬆️
8.0 80.79% <99.05%> (+0.38%) ⬆️
8.1 80.79% <99.05%> (+0.38%) ⬆️

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

Impacted Files Coverage Δ
src/Extension/Propagator/B3/B3SinglePropagator.php 98.55% <98.55%> (ø)
...c/API/Trace/Propagation/TraceContextPropagator.php 100.00% <100.00%> (ø)
.../Extension/Propagator/B3/B3DebugFlagContextKey.php 100.00% <100.00%> (ø)
src/Extension/Propagator/B3/B3MultiPropagator.php 100.00% <100.00%> (ø)
src/Extension/Propagator/B3/B3Propagator.php 100.00% <100.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 74eda3d...00e281e. Read the comment docs.

@kishannsangani kishannsangani marked this pull request as ready for review June 29, 2022 13:05
@Nevay
Copy link
Contributor

Nevay commented Jun 29, 2022

Single and multi-header propagation should be implemented as a single propagator with two configurations.

The debug flag has to be propagated (B3 Extract), the specification PR mentioned storing it in the returned context.

MUST preserve a debug trace flag, if received, and propagate it with subsequent requests.

@kishannsangani
Copy link
Member Author

@Nevay Added B3Propagator Class to handle two configurations and stored debug flag in the returned context. However, since we have implemented a singleton class for all the propagators, it is not possible to test the B3Propagator Class since there is no mechanism to reset the singleton static instance.

PS: @tidal @bobstrecansky @brettmc

@Nevay
Copy link
Contributor

Nevay commented Jul 24, 2022

We should allow users to choose between the two modes, either by having a singleton for each mode, or by exposing the constructor with an argument that allows choosing between the two modes.

Configuration Interface:

The SDK MUST provide a programmatic interface for all configuration. This interface SHOULD be written in the language of the SDK itself. All other configuration mechanisms SHOULD be built on top of this interface.

@kishannsangani
Copy link
Member Author

@Nevay so having a third B3Propagator class is not required since we have a singleton for each B3 Single and B3 Multi propagator?

@Nevay
Copy link
Contributor

Nevay commented Jul 25, 2022

No, we still need the combined ::extract() behavior - I meant replacing B3Propagator::getInstance() with a method for each configuration mode, something like ::getB3SingleHeaderInstance() and ::getB3MultiHeaderInstance().

Copy link
Contributor

@Nevay Nevay left a comment

Choose a reason for hiding this comment

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

Should be moved out of the API into an extension package - see Propagators Distribution.

src/API/Trace/Propagation/B3Propagator.php Outdated Show resolved Hide resolved
@kishannsangani
Copy link
Member Author

Should be moved out of the API into an extension package - see Propagators Distribution.

@Nevay do we have any extension packages in opentelemetry-php?

@brettmc
Copy link
Collaborator

brettmc commented Aug 17, 2022

do we have any extension packages in opentelemetry-php?

discussed in SIG, please go ahead and create an extensions dir (this is what otel-java does)

@kishannsangani
Copy link
Member Author

do we have any extension packages in opentelemetry-php?

discussed in SIG, please go ahead and create an extensions dir (this is what otel-java does)

@brettmc otel-java has nested directories under extension. Do we follow the same or a simple hierarchy? Also, they have api and sdk on the root folder whereas we have it under src. Should I place extension under the src?

@brettmc
Copy link
Collaborator

brettmc commented Aug 17, 2022

I think src/Extension/Propagator/B3 or something similar...

@kishannsangani
Copy link
Member Author

I think src/Extension/Propagator/B3 or something similar...

Created the new Extension directory and moved the files under it.

@bobstrecansky
Copy link
Collaborator

@kishannsangani - do we feel this is ready now?

@kishannsangani
Copy link
Member Author

@kishannsangani - do we feel this is ready now?

Yes, I just pushed the final changes for the clarification on specs.

@Nevay @bobstrecansky @brettmc @tidal can you review it?

Copy link
Contributor

@Nevay Nevay left a comment

Choose a reason for hiding this comment

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

Needs a composer.json in either src/Extension/Propagator or src/Extension/Propagator/B3.

src/Extension/Propagator/B3/B3SinglePropagator.php Outdated Show resolved Hide resolved
src/Extension/Propagator/B3/B3Propagator.php Outdated Show resolved Hide resolved
@kishannsangani
Copy link
Member Author

Needs a composer.json in either src/Extension/Propagator or src/Extension/Propagator/B3.

@Nevay added composer.json in src/Extension/Propagator/B3. Can you help me verify the same?

src/Extension/Propagator/B3/composer.json Outdated Show resolved Hide resolved
src/Extension/Propagator/B3/composer.json Outdated Show resolved Hide resolved
@brettmc brettmc merged commit 0f30849 into open-telemetry:main Aug 24, 2022
@kishannsangani kishannsangani deleted the b3_propagator_single_header branch August 24, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create B3 Propagator
4 participants