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

bridge/opentracing: fix baggage item key is canonicalized #4776

Conversation

scorpionknifes
Copy link
Member

@scorpionknifes scorpionknifes commented Dec 21, 2023

Fixes #4799
Related to #2793

Updated to fix propagated baggage item key is canonicalized #4799
A test to prove #2793 is working as expected.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I just made a quick look.

Won't this tests be helpful when working on #3685?

bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
@pellared pellared changed the title bridge/opentracing: add example test to propagate baggage item with carrier bridge/opentracing: add baggage propagation tests Dec 21, 2023
@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 21, 2023
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

The issue I personally experienced with the bridge was when values contained special characters, not these simplistic foo/bar strings

bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
@scorpionknifes scorpionknifes changed the title bridge/opentracing: add baggage propagation tests bridge/opentracing: fix baggage item key is canonicalized in bridgeSpan Dec 27, 2023
@scorpionknifes scorpionknifes changed the title bridge/opentracing: fix baggage item key is canonicalized in bridgeSpan bridge/opentracing: fix baggage item key is canonicalized Dec 27, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

Note - I changed the description from Closes 2793 to Related to - this PR does NOT close the original issue because the implementation still mangles the baggage values.

@yurishkuro
Copy link
Member

In order to close that issue the implementation must satisfy this condition: open-telemetry/opentelemetry-specification#3801

@scorpionknifes
Copy link
Member Author

scorpionknifes commented Dec 28, 2023

Note - I changed the description from Closes 2793 to Related to - this PR does NOT close the original issue because the implementation still mangles the baggage values.

@yurishkuro, We might be referencing different tickets; #2793 is an invalid issue and no changes needed: #2793 (comment)
I think #3685 was the one that shouldn't be closed?

@pellared
Copy link
Member

Note - I changed the description from Closes 2793 to Related to - this PR does NOT close the original issue because the implementation still mangles the baggage values.

I guess @yurishkuro had #3685 in mind.
I plan to look at this issue tomorrow or even today.

Regarding #2793 I would prefer the author to close the issue or close it after e.g. 1 month in case we got no response.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (08b856f) 82.2% compared to head (8776d8b) 82.2%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4776   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        226     226           
  Lines      18383   18381    -2     
=====================================
+ Hits       15126   15127    +1     
+ Misses      2975    2972    -3     
  Partials     282     282           
Files Coverage Δ
bridge/opentracing/bridge.go 64.2% <50.0%> (+0.4%) ⬆️

CHANGELOG.md Outdated Show resolved Hide resolved
bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
@pellared pellared removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 28, 2023
@pellared pellared merged commit 27f70a3 into open-telemetry:main Dec 28, 2023
25 of 26 checks passed
@MrAlias
Copy link
Contributor

MrAlias commented Jan 2, 2024

This seems to have merged without two approvals from the Go Approvers team.

@pellared
Copy link
Member

pellared commented Jan 2, 2024

@MrAlias Sorry, I have missed that @yurishkuro is not an approver 😬

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.

bridge/opentracing: propagated baggage item key is canonicalized
4 participants