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

Support for Lineage in XRay Trace Header and removing additional Baggage from being added #1671

Merged
merged 6 commits into from
Feb 18, 2025

Conversation

liustve
Copy link
Contributor

@liustve liustve commented Jan 24, 2025

Description:
Bug Fix: Current logic for XRay Propagator uses W3C Baggage to inject baggage information to the XRay Trace Header. However, XRay Tracing has a 256 character limit while W3C Baggage has 8192 character limit. This causes part of the baggage to be truncated if it every exceeds 256, causing malformed key-pairs values in the header.

Feature: XRay propagator will still need to use baggage to store Lineage information but we omit adding any sort of additional baggage data to the xray trace header. Instead Baggage context will remain propagated using W3C Baggage.

We do not have to worry about the 256 character limit currently as Lineage is currently using V2 format which has a maximum length of 18 characters.

Anyone using OTel Instrumentation with Xray will have to have both baggage and xray propagators turned on in that order (baggage first then xray).

Existing Issue(s):

Related PR: #1178

Testing:

Unit tests were written to verify that the extraction and injection logic were correct. Older unit tests were deprecated and deleted.

End-to-end testing was done using APIGW, EC2, and Lambda to verify scenarios with services with different instrumentations turned on to ensure the trace header was propagated correctly.

@liustve liustve requested a review from a team as a code owner January 24, 2025 19:52
Copy link

linux-foundation-easycla bot commented Jan 24, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@liustve
Copy link
Contributor Author

liustve commented Jan 28, 2025

@jack-berg
@jamesmoessis
@rapphil

Need additional reviews on the proposed XrayPropagator changes.

@trask
Copy link
Member

trask commented Jan 28, 2025

hi @liustve, can you sign the CLA? #1671 (comment)

@jack-berg @jamesmoessis @rapphil

Need additional reviews on the proposed XrayPropagator changes.

@srprash and @wangzlei are the owners for this component and have already been assigned as reviewers, thanks


if (xrayLineageHeader.length() < LINEAGE_MIN_LENGTH
|| xrayLineageHeader.length() > LINEAGE_MAX_LENGTH
|| numOfDelimiters != 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here a constant would be better as well

baggage = Baggage.builder();
} else if (trimmedPart.startsWith(LINEAGE_KEY)) {
lineageHeader = parseLineageHeader(value);
if (isValidLineage(lineageHeader)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worth adding logger.fine printing the value in case parsed lineage is invalid and lineage is not added to the baggage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply but comments addressed and implemented.

Copy link

@wangzlei wangzlei left a comment

Choose a reason for hiding this comment

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

The code looks good to me.
With this change, XRayPropagator appends but not overrides the baggage in context, it does not conflict with BaggagePropagator.

liustve added a commit to aws-observability/aws-otel-java-instrumentation that referenced this pull request Feb 17, 2025
*Description of changes:*
Enabling baggage propagator to support Lineage propagation:
open-telemetry/opentelemetry-java-contrib#1671


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@trask trask added this to the v1.44.0 milestone Feb 18, 2025
@trask trask merged commit e0cdc56 into open-telemetry:main Feb 18, 2025
17 checks passed
@liustve liustve deleted the xray_baggage_fix branch February 18, 2025 23:00
XinRanZhAWS pushed a commit to aws-observability/aws-otel-java-instrumentation that referenced this pull request Mar 11, 2025
*Description of changes:*
Enabling baggage propagator to support Lineage propagation:
open-telemetry/opentelemetry-java-contrib#1671


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
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.

5 participants