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

fix(propagator-b3): update extract to check for array #2285

Merged
merged 7 commits into from Jun 30, 2021

Conversation

jordanworner
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Updated the check for the b3 single header to ensure the value is not an empty array.
  • Added a test to check a b3 multi header is extracted if the TextMapGetter returns an array.

@dyladan
Copy link
Member

dyladan commented Jun 16, 2021

What truthy value causes this check to pass which is avoided by checking for .length? To me this is quite unintuitive. I would rather see Array.isArray() or typeof === 'string' or something which makes intent more clear.

@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #2285 (81de1e2) into main (b442048) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 81de1e2 differs from pull request most recent head 7511b48. Consider uploading reports for the commit 7511b48 to get more accurate results

@@            Coverage Diff             @@
##             main    #2285      +/-   ##
==========================================
+ Coverage   92.74%   92.76%   +0.02%     
==========================================
  Files         145      145              
  Lines        5182     5184       +2     
  Branches     1059     1060       +1     
==========================================
+ Hits         4806     4809       +3     
+ Misses        376      375       -1     
Impacted Files Coverage Δ
...es/opentelemetry-propagator-b3/src/B3Propagator.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

@jordanworner
Copy link
Contributor Author

@dyladan I have updated it and normalized the header value so the intention is clear. An empty array evaluates to true so it was passing the check.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan added the enhancement New feature or request label Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GRPC instrumentation doesn't extract from B3 multi propagator
5 participants