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

baggage: Add NewMemberRaw and NewKeyValuePropertyRaw #4804

Merged
merged 10 commits into from
Jan 10, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Dec 29, 2023

Towards #3685
Related to open-telemetry/opentelemetry-specification#3801

Add NewMemberRaw and NewKeyValuePropertyRaw which does not require-precent encoding.
Add a notice to NewMember and NewKeyValueProperty about the new functions.
The bridge now uses NewMemberRaw to fix #3685.

The alternative fix (quickfix) for the OpenTracing Bridge would be to precent-encode the value in func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string). However, I decided to add new functions, because of open-telemetry/opentelemetry-specification#3801. I also feel that the new functions are more usable.

Initially, I changed the bevavior of NewMember and NewKeyValuePropertyRaw, but I think that it would be safer and more user-friendly to make the change in a backwards-compatible manner.

This PR contains also breaking behavioral change: Property.Value now returns a vanilla string instead of a percent-encoded value.
This is what Member.Value returns right now so it makes the API consistent.
I would rather say that it is a bugfix than a change, but I prefer to have it listed in "changed" in the changelog.

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

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

Comparison is base (6ead8d8) 82.3% compared to head (517bd54) 82.3%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4804     +/-   ##
=======================================
- Coverage   82.3%   82.3%   -0.1%     
=======================================
  Files        226     226             
  Lines      18392   18401      +9     
=======================================
  Hits       15148   15148             
- Misses      2963    2969      +6     
- Partials     281     284      +3     
Files Coverage Δ
bridge/opentracing/bridge.go 64.2% <100.0%> (ø)
baggage/baggage.go 97.8% <64.0%> (-2.2%) ⬇️

@pellared pellared changed the title baggage: Precent-encoding and decoding is done only by Parse and String baggage: Precent-encoding and decoding is done only by String and Parse Dec 29, 2023
@pellared
Copy link
Member Author

@yurishkuro PTAL

@pellared pellared marked this pull request as ready for review December 29, 2023 10:19
@pellared pellared added pkg:bridges Related to a bridge package area:baggage Part of OpenTelemetry baggage labels Dec 29, 2023
@pellared pellared marked this pull request as draft December 29, 2023 10:33
@pellared pellared changed the title baggage: Precent-encoding and decoding is done only by String and Parse baggage: Precent-encoding and decoding done by String and Parse Dec 29, 2023
@pellared pellared changed the title baggage: Precent-encoding and decoding done by String and Parse baggage: Add NewMemberRaw and NewKeyValuePropertyRaw Dec 29, 2023
@pellared pellared marked this pull request as ready for review December 29, 2023 11:15
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.

I would suggest combining this change with w3c propagator change and moving all encoding responsibilities into the propagator

baggage/baggage.go Show resolved Hide resolved
baggage/baggage.go Outdated Show resolved Hide resolved
@pellared pellared requested review from MrAlias and yurishkuro January 3, 2024 07:46
@yurishkuro
Copy link
Member

Fixes #3685

I don't think it actually fixes the main ticket - a complete fix would be to change all propagators to use the new xxxRaw methods when accessing baggage.

@pellared
Copy link
Member Author

pellared commented Jan 3, 2024

Fixes #3685

I don't think it actually fixes the main ticket - a complete fix would be to change all propagators to use the new xxxRaw methods when accessing baggage.

Is there any other place where the propagators should use the new methods? I have not found any other usages.

Take notice that the baggage propagator uses Parse function and Baggage.String method that output proper output.

@yurishkuro
Copy link
Member

Take notice that the baggage propagator uses Parse function and Baggage.String method that output proper output.

that's precisely the thing that makes no sense to me. W3C baggage format is different from Jaeger baggage format, so encode/decode functions must exist in the propagators, not in the shared baggage layer, and propagators should only read/write raw values, not expect the shared baggage layer to do any encoding for them.

@pellared
Copy link
Member Author

pellared commented Jan 3, 2024

not in the shared baggage layer

That ship has sailed 🤷

We can copy paste the encoding/decoding code, but I see no big sense in doing that as I see no big value in code duplication. Rather the opposite as if we find an improvement or bug in the encoding/decoding code then we would have to apply it in two places.

I want to avoid making breaking changes in baggage package.

@yurishkuro
Copy link
Member

That ship has sailed 🤷

Why? Propagator calling Parse/String is implementation detail of the propagator, it can be changed. Same as with the non-raw accessors the Parse function can be deprecated.

@pellared
Copy link
Member Author

pellared commented Jan 4, 2024

Why? Propagator calling Parse/String is implementation detail of the propagator, it can be changed. Same as with the non-raw accessors the Parse function can be deprecated.

It would lead to code duplication as mentioned before. I think it would be less controversial if this PR only introduces new function as proposed #4804 (comment). The deprecation can be done in a separate PR.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@pellared pellared merged commit 259143a into open-telemetry:main Jan 10, 2024
24 of 25 checks passed
@MrAlias MrAlias added this to the v1.22.0 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:baggage Part of OpenTelemetry baggage pkg:bridges Related to a bridge package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Baggage API is not working, entangled with W3C Propagator
4 participants