-
Notifications
You must be signed in to change notification settings - Fork 745
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
[api] Fix Fields
in CompositeTextMapPropagator
#5745
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
The 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! |
@stevejgordon, I can confirm that it is on your machine, executed all tests locally from your branch. All are green (except couple ignored). |
a683035
to
bf384ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #2328
Changes
IEnumerable<TextMapPropagator>
.EmptyFields
.Inject
andExtract
.OpenTelemetry.Api
project.Reviewer notes:
I'm uncertain whether the
Fields
property should be considered immutable based on the spec. Most implementations (apart fromB3Propagator
) return a newHashSet<string>
every time they are accessed, resulting in the set always containing the same elements. However, a custom implementation ofTextMapPropagator
could potentially modify the set each timeFields
is accessed. I'm unsure if this needs to be considered. Currently, I've computed the union of fields once in theCompositeTextMapPropagator
constructor to avoid recalculating it for each access of theFields
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 staticHashSet<string>
whichFields
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 newHashSet
each time so that instances can mutate a shared set.Test Failures (locally)I had issues with running the OpenTelemetry.Api.Tests withnet462
as it doesn't seem to use the latest code from the project. When debugging, I could see that it pulled inOpenTelemetry.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.Debugging with 'Just my code' disabled, when I enter the ctor, it's coming from an external source:Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes* [ ] Changes in public API reviewed (if applicable)