Repository: open-telemetry/opentelemetry-go-contrib (go)

Issue:
Jaeger propagator doesn't accept trace ids with lengths other than 64 and 128 bit
### Summary
The Jaeger propagator currently doesn't accept trace ids with lengths other than exactly 64 or 128 bit, although the [Jaeger Propagation Format](https://www.jaegertracing.io/docs/1.37/client-libraries/#value) specifies that servers must 0-left-pad received hex-strings shorter than 32 hex characters. This results in an interoperability issue with the Python OpenTracing Jaeger Client, leading to received trace IDs wrongly being discarded.

### Details

We've run into an interoperability issue with a Python gRPC client instrumented with the OpenTracing Jaeger client and a Go gRPC server instrumented with OpenTelemetry: Some of the server traces were not associated with the client traces. Looking into this, I realized that all propagation values with 64-bit trace IDs with a leading `0` were being discarded. Assuming random distribution of the trace ID, every 16th 64-bit trace ID is being dropped (all IDs with a leading hex `0`).

The Python Jaeger Client generally [encodes trace IDs as variable-length integers](https://github.com/jaegertracing/jaeger-client-python/blob/4.8.0/jaeger_client/codecs.py#L206-L220). This may not only lead to the observed single zeroes in the beginning being dropped, but may result in multiple hex zeroes at the beginning of the trace ID being omitted.

Given this observed issue and the Jaeger trace ID specification requiring 0-padding up to 128 bits, I suggest changing the [current implementation](https://github.com/open-telemetry/opentelemetry-go-contrib/blob/v1.9.0/propagators/jaeger/jaeger_propagator.go#L110-L117) to left-pad with a variable numbers of zeros to always reach 128 bits (if the trace ID is shorter than 128 bits). The current implementation only supports adding 0-padding for exactly 64 bits. I may later open a PR changing this.


Pull request description:
Jaeger Propagator: Fix accepting short trace and span IDs
Fixes #2729.

While implementing the fix for trace IDs, i noticed that span IDs have the same requirement, so I fixed them as well.

As an optimization, we could keep the special case for 64-bit tracing IDs with a constant padding. I'm not sure it's worth it, so I haven't implemented this for now.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 79f44798d..9c70a8405 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -32,6 +32,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Rename the `Typ` field of `"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc".InterceptorInfo` to `Type`. (#2688)
 - Use Go 1.19 as the default version for CI testing/linting. (#2675)
 
+### Fixed
+
+- Fix the Jaeger propagator rejecting trace IDs that are both shorter than 128 bits and not exactly 64 bits long (while not being 0).
+  Also fix the propagator rejecting span IDs shorter than 64 bits.
+  This fixes compatibility with Jaeger clients encoding trace and span IDs as variable-length hex strings, [as required by the Jaeger propagation format](https://www.jaegertracing.io/docs/1.37/client-libraries/#value). (#2731)
+
 ## [1.9.0/0.34.0/0.4.0] - 2022-08-02
 
 ### Added
diff --git a/propagators/jaeger/jaeger_propagator.go b/propagators/jaeger/jaeger_propagator.go
index 3d2063823..08b5faa5d 100644
--- a/propagators/jaeger/jaeger_propagator.go
+++ b/propagators/jaeger/jaeger_propagator.go
@@ -28,11 +28,10 @@ import (
 const (
 	jaegerHeader        = "uber-trace-id"
 	separator           = ":"
-	traceID64bitsWidth  = 64 / 4
 	traceID128bitsWidth = 128 / 4
 	spanIDWidth         = 64 / 4
 
-	traceIDPadding = "0000000000000000"
+	idPaddingChar = "0"
 
 	flagsDebug      = 0x02
 	flagsSampled    = 0x01
@@ -108,12 +107,13 @@ func extract(ctx context.Context, headerVal string) (context.Context, trace.Span
 	// extract trace ID
 	if parts[0] != "" {
 		id := parts[0]
-		if len(id) != traceID128bitsWidth && len(id) != traceID64bitsWidth {
+		if len(id) > traceID128bitsWidth {
 			return ctx, empty, errInvalidTraceIDLength
 		}
-		// padding when length is 16
-		if len(id) == traceID64bitsWidth {
-			id = traceIDPadding + id
+		// padding when length is less than 32
+		if len(id) < traceID128bitsWidth {
+			padCharCount := traceID128bitsWidth - len(id)
+			id = strings.Repeat(idPaddingChar, padCharCount) + id
 		}
 		scc.TraceID, err = trace.TraceIDFromHex(id)
 		if err != nil {
@@ -124,9 +124,14 @@ func extract(ctx context.Context, headerVal string) (context.Context, trace.Span
 	// extract span ID
 	if parts[1] != "" {
 		id := parts[1]
-		if len(id) != spanIDWidth {
+		if len(id) > spanIDWidth {
 			return ctx, empty, errInvalidSpanIDLength
 		}
+		// padding when length is less than 16
+		if len(id) < spanIDWidth {
+			padCharCount := spanIDWidth - len(id)
+			id = strings.Repeat(idPaddingChar, padCharCount) + id
+		}
 		scc.SpanID, err = trace.SpanIDFromHex(id)
 		if err != nil {
 			return ctx, empty, errMalformedSpanID


Test patch:
diff --git a/propagators/jaeger/jaeger_data_test.go b/propagators/jaeger/jaeger_data_test.go
index cfd778957..eb752f36a 100644
--- a/propagators/jaeger/jaeger_data_test.go
+++ b/propagators/jaeger/jaeger_data_test.go
@@ -21,6 +21,7 @@ import (
 )
 
 const (
+	traceID15Str = "3ce929d0e0e4736"
 	traceID16Str = "a3ce929d0e0e4736"
 	traceID32Str = "a1ce929d0e0e4736a3ce929d0e0e4736"
 	spanIDStr    = "00f067aa0ba902b7"
@@ -28,6 +29,7 @@ const (
 )
 
 var (
+	traceID15 = trace.TraceID{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xce, 0x92, 0x9d, 0x0e, 0x0e, 0x47, 0x36}
 	traceID16 = trace.TraceID{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xa3, 0xce, 0x92, 0x9d, 0x0e, 0x0e, 0x47, 0x36}
 	traceID32 = trace.TraceID{0xa1, 0xce, 0x92, 0x9d, 0x0e, 0x0e, 0x47, 0x36, 0xa3, 0xce, 0x92, 0x9d, 0x0e, 0x0e, 0x47, 0x36}
 	spanID    = trace.SpanID{0x00, 0xf0, 0x67, 0xaa, 0x0b, 0xa9, 0x02, 0xb7}
@@ -117,6 +119,18 @@ var extractHeaders = []extractTest{
 		},
 		true,
 	},
+	{
+		"left padding 60 bit trace ID",
+		map[string]string{
+			jaegerHeader: fmt.Sprintf("%s:%s:0:1", traceID15Str, spanIDStr),
+		},
+		trace.SpanContextConfig{
+			TraceID:    traceID15,
+			SpanID:     spanID,
+			TraceFlags: trace.FlagsSampled,
+		},
+		false,
+	},
 	{
 		"left padding 64 bit trace ID",
 		map[string]string{
@@ -162,12 +176,6 @@ var invalidExtractHeaders = []extractTest{
 			jaegerHeader: fmt.Sprintf("%s:%s:0:1", traceID32Str+"0000", spanIDStr),
 		},
 	},
-	{
-		name: "trace ID length is not 32 or 16",
-		headers: map[string]string{
-			jaegerHeader: fmt.Sprintf("%s:%s:0:1", "1234567890abcd01234", spanIDStr),
-		},
-	},
 	{
 		name: "span ID length is not 16 or 32",
 		headers: map[string]string{
diff --git a/propagators/jaeger/jaeger_propagator_test.go b/propagators/jaeger/jaeger_propagator_test.go
index 016806c4f..a1a2ee33f 100644
--- a/propagators/jaeger/jaeger_propagator_test.go
+++ b/propagators/jaeger/jaeger_propagator_test.go
@@ -26,13 +26,15 @@ import (
 )
 
 var (
-	traceID        = trace.TraceID{0, 0, 0, 0, 0, 0, 0, 0, 0x7b, 0, 0, 0, 0, 0, 0x1, 0xc8}
-	traceID128Str  = "00000000000000007b000000000001c8"
+	traceID        = trace.TraceID{0, 0, 0, 0, 0, 0, 0, 0, 0xb, 0, 0, 0, 0, 0, 0x1, 0xc8}
+	traceID128Str  = "00000000000000000b000000000001c8"
 	zeroTraceIDStr = "00000000000000000000000000000000"
-	traceID64Str   = "7b000000000001c8"
+	traceID64Str   = "0b000000000001c8"
+	traceID60Str   = "b000000000001c8"
 	spanID         = trace.SpanID{0, 0, 0, 0, 0, 0, 0, 0x7b}
 	zeroSpanIDStr  = "0000000000000000"
-	spanIDStr      = "000000000000007b"
+	spanID64Str    = "000000000000007b"
+	spanID60Str    = "00000000000007b"
 )
 
 func TestJaeger_Extract(t *testing.T) {
@@ -46,7 +48,7 @@ func TestJaeger_Extract(t *testing.T) {
 		debug        bool
 	}{
 		{
-			traceID128Str, spanIDStr, deprecatedParentSpanID, "1",
+			traceID128Str, spanID64Str, deprecatedParentSpanID, "1",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -56,7 +58,7 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			traceID64Str, spanIDStr, deprecatedParentSpanID, "1",
+			traceID64Str, spanID64Str, deprecatedParentSpanID, "1",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -66,7 +68,17 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			traceID128Str, spanIDStr, deprecatedParentSpanID, "3",
+			traceID60Str, spanID60Str, deprecatedParentSpanID, "1",
+			trace.SpanContextConfig{
+				TraceID:    traceID,
+				SpanID:     spanID,
+				TraceFlags: trace.FlagsSampled,
+			},
+			nil,
+			false,
+		},
+		{
+			traceID128Str, spanID64Str, deprecatedParentSpanID, "3",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -77,7 +89,7 @@ func TestJaeger_Extract(t *testing.T) {
 		},
 		{
 			// if we didn't set sampled bit when debug bit is 1, then assuming it's not sampled
-			traceID128Str, spanIDStr, deprecatedParentSpanID, "2",
+			traceID128Str, spanID64Str, deprecatedParentSpanID, "2",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -88,7 +100,7 @@ func TestJaeger_Extract(t *testing.T) {
 		},
 		{
 			// ignore firehose bit since we don't really have this feature in otel span context
-			traceID128Str, spanIDStr, deprecatedParentSpanID, "8",
+			traceID128Str, spanID64Str, deprecatedParentSpanID, "8",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -98,7 +110,7 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			traceID128Str, spanIDStr, deprecatedParentSpanID, "9",
+			traceID128Str, spanID64Str, deprecatedParentSpanID, "9",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -108,7 +120,7 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			traceID128Str, spanIDStr, "wired stuff", "1",
+			traceID128Str, spanID64Str, "wired stuff", "1",
 			trace.SpanContextConfig{
 				TraceID:    traceID,
 				SpanID:     spanID,
@@ -118,13 +130,13 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			fmt.Sprintf("%32s", "This_is_a_string_len_64"), spanIDStr, deprecatedParentSpanID, "1",
+			fmt.Sprintf("%32s", "This_is_a_string_len_64"), spanID64Str, deprecatedParentSpanID, "1",
 			trace.SpanContextConfig{},
 			errMalformedTraceID,
 			false,
 		},
 		{
-			"000000000007b00000000000001c8", spanIDStr, deprecatedParentSpanID, "1",
+			"0000000000000007b00000000000001c8", spanID64Str, deprecatedParentSpanID, "1",
 			trace.SpanContextConfig{},
 			errInvalidTraceIDLength,
 			false,
@@ -136,7 +148,7 @@ func TestJaeger_Extract(t *testing.T) {
 			false,
 		},
 		{
-			traceID128Str, "0000000000010", deprecatedParentSpanID, "1",
+			traceID128Str, "00000000000000010", deprecatedParentSpanID, "1",
 			trace.SpanContextConfig{},
 			errInvalidSpanIDLength,
 			false,