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

[api] Fix Fields in CompositeTextMapPropagator #5745

Conversation

stevejgordon
Copy link
Contributor

@stevejgordon stevejgordon commented Jul 10, 2024

Fixes #2328

Changes

  • Updates CompositeTextMapPropagator.Fields to return a unioned set of fields based on all propagators it wraps. This set is calculated once during instance construction.
  • Updates the ctor to discard null propagators in the IEnumerable<TextMapPropagator>.
  • Removes use of mutable static EmptyFields.
  • Updates tests to ensure expected fields and adds assertions to validate that each wrapped propagator is called for Inject and Extract.
  • Relocates test classes under the correct folder structure (and updates their namespace) to match the OpenTelemetry.Api project.

Reviewer notes:

I'm uncertain whether the Fields property should be considered immutable based on the spec. Most implementations (apart from B3Propagator) return a new HashSet<string> every time they are accessed, resulting in the set always containing the same elements. However, a custom implementation of TextMapPropagator could potentially modify the set each time Fields is accessed. I'm unsure if this needs to be considered. Currently, I've computed the union of fields once in the CompositeTextMapPropagator constructor to avoid recalculating it for each access of the Fields property. If necessary, we can transfer this logic to the getter to accommodate wrapper propagators dynamically changing their fields.

I noticed that the B3Propagator, untouched in the PR, currently uses a static HashSet<string> which Fields returns. This could be a bug since it is then possible for an instance to add items to that set. This is an edge case, but we could also fix it by returning a new HashSet each time so that instances can mutate a shared set.

Test Failures (locally)

I had issues with running the OpenTelemetry.Api.Tests with net462 as it doesn't seem to use the latest code from the project. When debugging, I could see that it pulled in OpenTelemetry.Api from my package cache instead, but it did not include the ctor changes. I'd love some thoughts on why that's happening. I had a quick look but couldn't see anything obvious. I even updated the csproj to explicitly reference the project being tested. Is it just on my machine? Hopefully, the CI run will also show if this is an issue.

[xUnit.net 00:00:01.99]     OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_SingleTextMapPropagator [FAIL]
[xUnit.net 00:00:02.01]     OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_TestPropagator [FAIL]
[xUnit.net 00:00:02.04]     OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_UsingSameTag [FAIL]
  Failed OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_SingleTextMapPropagator [16 ms]
  Error Message:
   Assert.Equal() Failure: HashSets differ
Expected: ["custom-traceparent-1", "custom-tracestate-1"]
Actual:   []
  Stack Trace:
     at OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_SingleTextMapPropagator() in C:\Users\SteveGordon\Code\OTel\opentelemetry-dotnet\test\OpenTelemetry.Api.Tests\Trace\Propagation\CompositePropagatorTest.cs:line 58
  Failed OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_TestPropagator [11 ms]
  Error Message:
   Assert.Equal() Failure: Values differ
Expected: 4
Actual:   0
  Stack Trace:
     at OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_TestPropagator() in C:\Users\SteveGordon\Code\OTel\opentelemetry-dotnet\test\OpenTelemetry.Api.Tests\Trace\Propagation\CompositePropagatorTest.cs:line 68
  Failed OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_UsingSameTag [3 ms]
  Error Message:
   Assert.Equal() Failure: Values differ
Expected: 3
Actual:   0
  Stack Trace:
     at OpenTelemetry.Context.Propagation.Tests.CompositePropagatorTest.CompositePropagator_UsingSameTag() in C:\Users\SteveGordon\Code\OTel\opentelemetry-dotnet\test\OpenTelemetry.Api.Tests\Trace\Propagation\CompositePropagatorTest.cs:line 112
[xUnit.net 00:00:02.90]     OpenTelemetry.Trace.Tests.TracerTest.TracerConcurrencyTest [SKIP]
  Skipped OpenTelemetry.Trace.Tests.TracerTest.TracerConcurrencyTest [1 ms]

Failed!  - Failed:     3, Passed:   216, Skipped:     1, Total:   220, Duration: 2 s - OpenTelemetry.Api.Tests.dll (net462)

Debugging with 'Just my code' disabled, when I enter the ctor, it's coming from an external source:
image

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
    * [ ] Changes in public API reviewed (if applicable)
@stevejgordon stevejgordon requested a review from a team as a code owner July 10, 2024 09:39
@github-actions github-actions bot added the pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package label Jul 10, 2024
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.24%. Comparing base (6250307) to head (ea8963f).
Report is 281 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5745      +/-   ##
==========================================
+ Coverage   83.38%   86.24%   +2.85%     
==========================================
  Files         297      254      -43     
  Lines       12531    11071    -1460     
==========================================
- Hits        10449     9548     -901     
+ Misses       2082     1523     -559     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.24% <100.00%> (?)
unittests-Project-Stable 86.16% <100.00%> (?)

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

Files Coverage Δ
.../Context/Propagation/CompositeTextMapPropagator.cs 100.00% <100.00%> (+16.66%) ⬆️

... and 198 files with indirect coverage changes

@stevejgordon
Copy link
Contributor Author

stevejgordon commented Jul 10, 2024

The net462 tests were fine in CI, so it's likely a local issue for me to figure out. The failures are unrelated and being addressed in #5744.

UPDATE: It's definitely something specific to my main laptop. I've just pulled the code and run the tests on another Windows laptop, and they all pass!

@Kielek
Copy link
Contributor

Kielek commented Jul 15, 2024

@stevejgordon, I can confirm that it is on your machine, executed all tests locally from your branch. All are green (except couple ignored).

@stevejgordon stevejgordon force-pushed the bug/2328/CompositeTextMapPropagator branch from a683035 to bf384ac Compare July 15, 2024 16:02
@CodeBlanch CodeBlanch changed the title Fix Fields in CompositeTextMapPropagator Jul 17, 2024
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@CodeBlanch CodeBlanch merged commit 6a0a934 into open-telemetry:main Jul 17, 2024
40 checks passed
@stevejgordon stevejgordon deleted the bug/2328/CompositeTextMapPropagator branch July 23, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package
3 participants