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 opentracing header name issue #5840

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

abdolsamad
Copy link
Contributor

Fixes an issue in opentracing baggage propagation.

There is no guarantee that TextMapGetters will send headers(.keys()) as lowercase strings. Some of the TextMapGetters like undertow:1.7 send the headers as they receive them(e.g. Ot-Baggage-Key) but ensure case insensitivity of headers by allowing get() to be called regardless of the header name case. This behaviour breaks the OtTracePropagator because it expects lowercase literal ot-baggage- to be present in header name.

This PR fixes that issue.

@abdolsamad abdolsamad requested a review from a team September 20, 2023 09:52
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
...extension/trace/propagation/OtTracePropagator.java 94.44% <100.00%> (+0.32%) ⬆️

... and 77 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

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

I think this pr presents a valid concern.

Comment on lines 120 to 121
String baggageKey = lowercaseKey.replace(OtTracePropagator.PREFIX_BAGGAGE_HEADER, "");
baggageBuilder.put(baggageKey, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String baggageKey = lowercaseKey.replace(OtTracePropagator.PREFIX_BAGGAGE_HEADER, "");
baggageBuilder.put(baggageKey, value);
baggageBuilder.put(key.substring(OtTracePropagator.PREFIX_BAGGAGE_HEADER.length()), value);

Using substring preserves the case of the baggage key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can not preserve the case of the baggage key anyway. They are carried in http headers and headers are case insensitive. The servlet implementations, and upstream services (sometimes) change it. I have seen several type of changes:

  • ot-baggage-some-key to Ot-Baggage-Some-Key (for example golang http package does this)
  • ot-baggage-Some-Key to ot-baggage-some-key (tomcat does this)
  • ot-baggage-Some-Key to ot-baggage-Some-Key (keeping the case, undertow does this)

I think, since there is no guarantee of baggage name casing, we can at least keep it consistent and lowercase it everywhere (as tomcat does). Of course I have no strong objection with your changes and if you still prefer your changes I can apply them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be better to use substring on the lowercaseKey. Substring is a simpler operation than replace and should be a bit more efficient. Also replace replaces all occurrences of given substring which would have undesired results if the key contained multiple copies of the prefix (which I guess is unlikely to ever happen).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -108,14 +109,16 @@ public <C> Context extract(Context context, @Nullable C carrier, TextMapGetter<C
if (carrier != null) {
BaggageBuilder baggageBuilder = Baggage.builder();
for (String key : getter.keys(carrier)) {
if (!key.startsWith(PREFIX_BAGGAGE_HEADER)) {
String lowercaseKey = key.toLowerCase(Locale.ROOT);
if (!lowercaseKey.startsWith(PREFIX_BAGGAGE_HEADER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively could use

if (!key.regionMatches(true, 0, PREFIX_BAGGAGE_HEADER, 0, PREFIX_BAGGAGE_HEADER.length())) {
  continue;
}

to avoid converting the whole key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decided to keep the case of header, I will also apply this change.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay looking at this @abdolsamad. Can you write a unit test to confirm the new behavior?

@abdolsamad
Copy link
Contributor Author

@jack-berg I added a test for the new behaviour. Please don't hesitate to tell me if more test or changes are required.

@jack-berg
Copy link
Member

@laurit do you still have open questions about this? If not, I'll merge.

Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

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

@jack-berg looks good, left 1 more comment

@abdolsamad
Copy link
Contributor Author

@laurit @jack-berg I changed the replace to substring

@jack-berg jack-berg merged commit ccb2e04 into open-telemetry:main Oct 31, 2023
18 checks passed
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.

3 participants