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(baggage): include baggage metadata when propagating baggage entries #2766

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

chrskrchr
Copy link
Contributor

@chrskrchr chrskrchr commented Feb 6, 2022

Which problem is this PR solving?

Partially fixes #2740

Short description of the changes

As part of the changes to address #2740, the W3CBaggagePropagator needs to be updated to propagate baggage entry metadata. It was previously parsing the metadata on baggage extraction from the inbound request but not including it on propagation.

Type of change

  • New feature (non-breaking change which adds functionality)

This PR might also be considered a "bug fix" since it's adding functionality that was expected to already be in place.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • added unit test assertion
  • verified in local application that baggage metadata is now being propagated

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 6, 2022

CLA Signed

The committers are authorized under a signed CLA.

@vmarchaud
Copy link
Member

@chrskrchr You'll need to sign the CLA so we can accept the PR

@vmarchaud vmarchaud added the bug Something isn't working label Feb 6, 2022
@codecov
Copy link

codecov bot commented Feb 6, 2022

Codecov Report

Merging #2766 (aff56e2) into main (4f8849f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2766      +/-   ##
==========================================
- Coverage   93.29%   93.28%   -0.02%     
==========================================
  Files         159      159              
  Lines        5445     5448       +3     
  Branches     1143     1144       +1     
==========================================
+ Hits         5080     5082       +2     
- Misses        365      366       +1     
Impacted Files Coverage Δ
packages/opentelemetry-core/src/baggage/utils.ts 75.75% <100.00%> (+2.42%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@chrskrchr
Copy link
Contributor Author

@chrskrchr You'll need to sign the CLA so we can accept the PR

Yep! Working on that with my organization now since I'm technically a "corporate contributor". I'll move this PR out of draft mode as soon as I have that sorted.

@chrskrchr chrskrchr marked this pull request as ready for review February 7, 2022 15:17
@chrskrchr chrskrchr requested a review from a team February 7, 2022 15:17
@chrskrchr
Copy link
Contributor Author

@vmarchaud - my CLA has been approved and this PR is ready for review.

I see you added the bug tag to the PR. Would you prefer I include the fix prefer on the PR title so it's released as a patch/fix change?

@vmarchaud
Copy link
Member

Would you prefer I include the fix prefer on the PR title so it's released as a patch/fix change?

I believe its based on the PR tag, but yeah i think its preferable since we should have propagated since in the first place

@chrskrchr chrskrchr changed the title feat(baggage): include baggage metadata when propagating baggage entries fix(baggage): include baggage metadata when propagating baggage entries Feb 7, 2022
@legendecas legendecas merged commit 1dd8a89 into open-telemetry:main Feb 9, 2022
@legendecas
Copy link
Member

Thank you for the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add properties field to the BaggageEntry interface
3 participants