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

Separate OpenTelemetry.Api B3 Trace Propagator to a new Nuget Package #3244

Merged
merged 28 commits into from
May 10, 2022

Conversation

dmpe
Copy link
Contributor

@dmpe dmpe commented Apr 30, 2022

part of #1881

Changes

As described in #1881, this MR creates a new Nuget Package (and VS project inside of solution) for B3 tracing propagation. As suggested by @cijothomas, and furher documented here https://opentelemetry.io/docs/reference/specification/context/api-propagators/#propagators-distribution, B3 as well as Jaeger propagators need to be independent, extension packages.

Please, can you approve workflows so that tests etc could be executed. Thanks.

Details

  • 2 new projects for both the main the nuget package as well as the testing project
  • public api (net2.0+net462) remained the same, as I did not adjust the code logic of b3 tracing itself

For significant contributions please make sure you have completed the following items:

@dmpe dmpe requested a review from a team April 30, 2022 10:19
reyang
reyang previously requested changes Apr 30, 2022
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Looks like a breaking change.
I would suggestion the following path:

  1. Do not remove existing B3 APIs, consider adding https://docs.microsoft.com/en-us/dotnet/api/system.obsoleteattribute to these B3 APIs.
  2. Create an issue to track the removal of the B3 APIs in the next MAJOR release (e.g. 2.x).

OpenTelemetry.sln Outdated Show resolved Hide resolved
@dmpe
Copy link
Contributor Author

dmpe commented May 7, 2022

Looks like a breaking change. I would suggestion the following path:

  1. Do not remove existing B3 APIs, consider adding https://docs.microsoft.com/en-us/dotnet/api/system.obsoleteattribute to these B3 APIs.
  2. Create an issue to track the removal of the B3 APIs in the next MAJOR release (e.g. 2.x).

Done. It has been marked as obsolete (just functions, not attributes, was not sure about that) and a new issue #3259 has been created as well.

Also, package has been renamed as suggested (thanks @alanwest). Tests were executed locally, and run fine.

@reyang reyang dismissed their stale review May 9, 2022 02:45

Breaking changes are avoided

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang changed the title draft: Separate OpenTelemetry.Api B3 Trace Propagator to a new Nuget Package Separate OpenTelemetry.Api B3 Trace Propagator to a new Nuget Package May 9, 2022
dmpe and others added 3 commits May 9, 2022 13:45
@dmpe
Copy link
Contributor Author

dmpe commented May 9, 2022

thanks @reyang - I will bring this in the next 2 days to the finish...

@cijothomas
Copy link
Member

@dmpe I;d suggest to break this down into smaller scoped PRs.

  1. Start adding a new project and add B3 propagator (its diff. namespace from existing, but rest of code is exact same as existing one)
  2. Add Readme.md files and tests
  3. Modify all existing code in this repo to switch to new propagator.
  4. Obsolete the current B3Propagator.

@cijothomas
Copy link
Member

Please, can you approve workflows so that tests etc could be executed. Thanks.

Please, can you approve workflows so that tests etc could be executed. Thanks.

^ _ once you make a smaller PR and get it merged, Github will automatically trigger CI checks for your PR. (the restriction is only for 1st time contributions. So you have a good reason to make Github think you are not 1st time contributor!)

@dmpe
Copy link
Contributor Author

dmpe commented May 9, 2022

@dmpe I;d suggest to break this down into smaller scoped PRs.

  1. Start adding a new project and add B3 propagator (its diff. namespace from existing, but rest of code is exact same as existing one)
  2. Add Readme.md files and tests
  3. Modify all existing code in this repo to switch to new propagator.
  4. Obsolete the current B3Propagator.

Thanks.
I have added both test and main project, so this shall be ready for review.

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #3244 (8b6c7c8) into main (72fcbf3) will decrease coverage by 0.03%.
The diff coverage is 85.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3244      +/-   ##
==========================================
- Coverage   85.48%   85.44%   -0.04%     
==========================================
  Files         261      262       +1     
  Lines        9410     9502      +92     
==========================================
+ Hits         8044     8119      +75     
- Misses       1366     1383      +17     
Impacted Files Coverage Δ
...enTelemetry.Extensions.Propagators/B3Propagator.cs 85.86% <85.86%> (ø)
src/OpenTelemetry/BatchExportProcessor.cs 84.21% <0.00%> (-3.16%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 90.94% <0.00%> (-0.87%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️

@cijothomas
Copy link
Member

@dmpe Almost ready to merge this PR. Left a few minor comments, you may address it in a follow up, but make sure the CI is green!

@dmpe
Copy link
Contributor Author

dmpe commented May 10, 2022

The api compatibility test has failed https://github.com/open-telemetry/opentelemetry-dotnet/runs/6371440160?check_suite_focus=true previously but it is not clear to me as to why? Otherwise, fixed as suggested.

@cijothomas cijothomas merged commit 5c168a3 into open-telemetry:main May 10, 2022
@cijothomas
Copy link
Member

The api compatibility test has failed https://github.com/open-telemetry/opentelemetry-dotnet/runs/6371440160?check_suite_focus=true previously but it is not clear to me as to why? Otherwise, fixed as suggested.

Its a flaky test - #2885

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.

4 participants