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

remove baggage propagation from xray propagator #1178

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

remove baggage propagation from xray propagator #1178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d078667
remove baggage propagation from xray propagator
jamesmoessis Feb 1, 2024
4dde536
spotless
jamesmoessis Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 14 additions & 59 deletions ...opagator/src/main/java/io/opentelemetry/contrib/awsxray/propagator/AwsXrayPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@

package io.opentelemetry.contrib.awsxray.propagator;

import io.opentelemetry.api.baggage.Baggage;
import io.opentelemetry.api.baggage.BaggageBuilder;
import io.opentelemetry.api.baggage.BaggageEntry;
import io.opentelemetry.api.internal.StringUtils;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
Expand All @@ -21,7 +18,6 @@
import io.opentelemetry.context.propagation.TextMapSetter;
import java.util.Collections;
import java.util.List;
import java.util.function.BiConsumer;
import java.util.logging.Logger;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -112,49 +108,20 @@ public <C> void inject(Context context, @Nullable C carrier, TextMapSetter<C> se
char samplingFlag = spanContext.isSampled() ? IS_SAMPLED : NOT_SAMPLED;
// TODO: Add OT trace state to the X-Ray trace header

StringBuilder traceHeader = new StringBuilder();
traceHeader
.append(TRACE_ID_KEY)
.append(KV_DELIMITER)
.append(xrayTraceId)
.append(TRACE_HEADER_DELIMITER)
.append(PARENT_ID_KEY)
.append(KV_DELIMITER)
.append(parentId)
.append(TRACE_HEADER_DELIMITER)
.append(SAMPLED_FLAG_KEY)
.append(KV_DELIMITER)
.append(samplingFlag);

Baggage baggage = Baggage.fromContext(context);
// Truncate baggage to 256 chars per X-Ray spec.
Copy link
Member

Choose a reason for hiding this comment

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

I can't seem to find a spec document for the xray spec format. If there is a document, and it does talk about including baggage like this, we should likely follow the spec.

Copy link

Choose a reason for hiding this comment

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

I'm not aware of a formal specification as well, but I usually follow this reference: https://docs.aws.amazon.com/xray/latest/devguide/aws-xray.html#Tracing-header:~:text=sampling%20rules.-,Tracing%20header,-All%20requests%20are

It seems that there is no equivalent concept to baggage, at least in the reference above.

Copy link
Member

Choose a reason for hiding this comment

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

No reference to baggage here, so maybe its right to delete it.

Copy link

Choose a reason for hiding this comment

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

refer to the comment, using key "XRAY_HEADER_ADDITIONAL_FIELDS"

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this comment:

Regarding the length of the X-Ray header, I did not find a definition in X-Ray spec. However, based on some clues, it can be inferred that StepFunction likely has a contract with X-Ray, hence there is length checking in the StepFunction client API. So it is reasonable to truncate the baggage length to (256 - the length of the existing trace context).

My read of this is that including up to 256 baggage bytes is a bug that should be fixed, instead of removing baggage encoding altogether.

Copy link

Choose a reason for hiding this comment

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

Right. #563 is StepFunction receives a xray header over 256. AWS Lambda is working on a new encoding algorithm to reducing the xray header to be less than 256, then we can fix the bug by truncating xray header to be 256, not truncating baggage to 256.

baggage.forEach(
new BiConsumer<String, BaggageEntry>() {

private int baggageWrittenBytes;

@Override
public void accept(String key, BaggageEntry entry) {
if (key.equals(TRACE_ID_KEY)
|| key.equals(PARENT_ID_KEY)
|| key.equals(SAMPLED_FLAG_KEY)) {
return;
}
// Size is key/value pair, excludes delimiter.
int size = key.length() + entry.getValue().length() + 1;
if (baggageWrittenBytes + size > 256) {
Copy link
Member

Choose a reason for hiding this comment

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

The primary incompatibility is that the limit of 256 characters in the Amazon Trace header. This is incompatible with the W3C baggage limit of 8192 bytes. Attempting to truncate the baggage to fit within this limit risks corrupting the entries and should not be attempted.

It looks like the code is only trying to include baggage entires if the entire entry can be included without exceeding 256 bytes.

When you talk about corrupting entries, are you referring to some entries being present while others not? Can you help me understand why this is problematic?

return;
}
traceHeader
.append(TRACE_HEADER_DELIMITER)
.append(key)
.append(KV_DELIMITER)
.append(entry.getValue());
baggageWrittenBytes += size;
}
});

setter.set(carrier, TRACE_HEADER_KEY, traceHeader.toString());
String traceHeader =
TRACE_ID_KEY
+ KV_DELIMITER
+ xrayTraceId
+ TRACE_HEADER_DELIMITER
+ PARENT_ID_KEY
+ KV_DELIMITER
+ parentId
+ TRACE_HEADER_DELIMITER
+ SAMPLED_FLAG_KEY
+ KV_DELIMITER
+ samplingFlag;

setter.set(carrier, TRACE_HEADER_KEY, traceHeader);
}

@Override
Expand All @@ -180,9 +147,6 @@ private static <C> Context getContextFromHeader(
String spanId = SpanId.getInvalid();
Boolean isSampled = false;

BaggageBuilder baggage = null;
int baggageReadBytes = 0;

int pos = 0;
while (pos < traceHeader.length()) {
int delimiterIndex = traceHeader.indexOf(TRACE_HEADER_DELIMITER, pos);
Expand Down Expand Up @@ -210,12 +174,6 @@ private static <C> Context getContextFromHeader(
spanId = parseSpanId(value);
} else if (trimmedPart.startsWith(SAMPLED_FLAG_KEY)) {
isSampled = parseTraceFlag(value);
} else if (baggageReadBytes + trimmedPart.length() <= 256) {
if (baggage == null) {
baggage = Baggage.builder();
}
baggage.put(trimmedPart.substring(0, equalsIndex), value);
baggageReadBytes += trimmedPart.length();
}
}
if (isSampled == null) {
Expand All @@ -241,9 +199,6 @@ private static <C> Context getContextFromHeader(
if (spanContext.isValid()) {
context = context.with(Span.wrap(spanContext));
}
if (baggage != null) {
context = context.with(baggage.build());
Copy link

Choose a reason for hiding this comment

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

Can we use a new key "XRAY_HEADER_ADDITIONAL_FIELDS"

context = context.with(XRAY_HEADER_ADDITIONAL_FIELDS ,baggage.build());

FYI #1178 (comment)

}
return context;
}

Expand Down
106 changes: 0 additions & 106 deletions ...ator/src/test/java/io/opentelemetry/contrib/awsxray/propagator/AwsXrayPropagatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import static io.opentelemetry.contrib.awsxray.propagator.AwsXrayPropagator.TRACE_HEADER_KEY;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.api.baggage.Baggage;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.api.trace.TraceFlags;
Expand All @@ -21,8 +20,6 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -89,63 +86,6 @@ void inject_NotSampledContext() {
"Root=1-8a3c60f7-d188f8fa79d48a391a778fa6;Parent=53995c3f42cd8ad8;Sampled=0");
}

@Test
void inject_WithBaggage() {
Map<String, String> carrier = new LinkedHashMap<>();
subject.inject(
withSpanContext(
SpanContext.create(
TRACE_ID, SPAN_ID, TraceFlags.getDefault(), TraceState.getDefault()),
Context.current())
.with(
Baggage.builder()
.put("cat", "meow")
.put("dog", "bark")
.put("Root", "ignored")
.put("Parent", "ignored")
.put("Sampled", "ignored")
.build()),
carrier,
SETTER);

assertThat(carrier)
.containsEntry(
TRACE_HEADER_KEY,
"Root=1-8a3c60f7-d188f8fa79d48a391a778fa6;Parent=53995c3f42cd8ad8;Sampled=0;"
+ "cat=meow;dog=bark");
}

@Test
void inject_WithBaggage_LimitTruncates() {
Map<String, String> carrier = new LinkedHashMap<>();
// Limit is 256 characters for all baggage. We add a 254-character key/value pair and a
// 3 character key value pair.
String key1 = Stream.generate(() -> "a").limit(252).collect(Collectors.joining());
String value1 = "a"; // 252 + 1 (=) + 1 = 254

String key2 = "b";
String value2 = "b"; // 1 + 1 (=) + 1 = 3

Baggage baggage = Baggage.builder().put(key1, value1).put(key2, value2).build();

subject.inject(
withSpanContext(
SpanContext.create(
TRACE_ID, SPAN_ID, TraceFlags.getDefault(), TraceState.getDefault()),
Context.current())
.with(baggage),
carrier,
SETTER);

assertThat(carrier)
.containsEntry(
TRACE_HEADER_KEY,
"Root=1-8a3c60f7-d188f8fa79d48a391a778fa6;Parent=53995c3f42cd8ad8;Sampled=0;"
+ key1
+ '='
+ value1);
}

@Test
void inject_WithTraceState() {
Map<String, String> carrier = new LinkedHashMap<>();
Expand Down Expand Up @@ -232,52 +172,6 @@ void extract_DifferentPartOrder() {
TRACE_ID, SPAN_ID, TraceFlags.getSampled(), TraceState.getDefault()));
}

@Test
void extract_AdditionalFields() {
Map<String, String> carrier = new LinkedHashMap<>();
carrier.put(
TRACE_HEADER_KEY,
"Root=1-8a3c60f7-d188f8fa79d48a391a778fa6;Parent=53995c3f42cd8ad8;Sampled=1;Foo=Bar");

Context context = subject.extract(Context.current(), carrier, GETTER);
assertThat(getSpanContext(context))
.isEqualTo(
SpanContext.createFromRemoteParent(
TRACE_ID, SPAN_ID, TraceFlags.getSampled(), TraceState.getDefault()));
assertThat(Baggage.fromContext(context).getEntryValue("Foo")).isEqualTo("Bar");
}

@Test
void extract_Baggage_LimitTruncates() {
// Limit is 256 characters for all baggage. We add a 254-character key/value pair and a
// 3 character key value pair.
String key1 = Stream.generate(() -> "a").limit(252).collect(Collectors.joining());
String value1 = "a"; // 252 + 1 (=) + 1 = 254

String key2 = "b";
String value2 = "b"; // 1 + 1 (=) + 1 = 3

Map<String, String> carrier = new LinkedHashMap<>();
carrier.put(
TRACE_HEADER_KEY,
"Root=1-8a3c60f7-d188f8fa79d48a391a778fa6;Parent=53995c3f42cd8ad8;Sampled=1;"
+ key1
+ '='
+ value1
+ ';'
+ key2
+ '='
+ value2);

Context context = subject.extract(Context.current(), carrier, GETTER);
assertThat(getSpanContext(context))
.isEqualTo(
SpanContext.createFromRemoteParent(
TRACE_ID, SPAN_ID, TraceFlags.getSampled(), TraceState.getDefault()));
assertThat(Baggage.fromContext(context).getEntryValue(key1)).isEqualTo(value1);
assertThat(Baggage.fromContext(context).getEntryValue(key2)).isNull();
}

@Test
void extract_EmptyHeaderValue() {
Map<String, String> invalidHeaders = new LinkedHashMap<>();
Expand Down
Loading