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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Dec 28, 2023
Merged

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

merged 13 commits into from
Dec 28, 2023

Conversation

scorpionknifes
Copy link
Member

@scorpionknifes scorpionknifes commented Dec 21, 2023 •
edited by yurishkuro
Loading

Fixes #4799
Related to #2793

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

@scorpionknifes scorpionknifes mentioned this pull request Dec 21, 2023
Closed
pellared
pellared reviewed Dec 21, 2023
View reviewed changes
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
@scorpionknifes @pellared
Update bridge/opentracing/bridge_test.go
285ad6d 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@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
yurishkuro
yurishkuro reviewed Dec 21, 2023
View reviewed changes
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 mentioned this pull request Dec 27, 2023
Closed
@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
pellared
pellared reviewed Dec 27, 2023
View reviewed changes
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 and others added 2 commits December 28, 2023 16:51
@scorpionknifes @pellared
Update CHANGELOG.md
e689a1f 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@scorpionknifes
Merge branch 'main' into chore-add-bridge-opentracing-baggage-propaga…
2140fd6 
…tion-test
@scorpionknifes
Copy link
Member Author

scorpionknifes commented Dec 28, 2023 •
edited
Loading

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.

@codecov Codecov
Copy link

codecov bot commented Dec 28, 2023 •
edited
Loading

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%) ⬆️

@pellared pellared requested a review from yurishkuro December 28, 2023 09:50
yurishkuro
yurishkuro approved these changes Dec 28, 2023
View reviewed changes
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 😬

@MrAlias MrAlias added this to the v1.22.0 milestone Jan 11, 2024
@MadVikingGod MadVikingGod mentioned this pull request Jan 11, 2024
Merged
@scorpionknifes scorpionknifes mentioned this pull request Apr 24, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.22.0
Development

Successfully merging this pull request may close these issues.

bridge/opentracing: propagated baggage item key is canonicalized
4 participants
@scorpionknifes @yurishkuro @pellared @MrAlias