From d0a0b514d906cad1592b38b14c340f90a0c5f351 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Mon, 2 Sep 2024 02:02:22 +0530 Subject: [PATCH 01/20] stats: opentelemetry GrpcTraceBinPropagator refactor GRPCTraceBinPropagator to be exported externally Use context instead of metadata in CustomCarrier --- .../internal/grpc_trace_bin_propagator.go | 132 ++++++++++++ .../grpc_trace_bin_propagator_test.go | 116 ++++++++++ .../internal/tracing/custom_carrier.go | 103 +++++++++ .../internal/tracing/custom_carrier_test.go | 202 ++++++++++++++++++ 4 files changed, 553 insertions(+) create mode 100644 stats/opentelemetry/internal/grpc_trace_bin_propagator.go create mode 100644 stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go create mode 100644 stats/opentelemetry/internal/tracing/custom_carrier.go create mode 100644 stats/opentelemetry/internal/tracing/custom_carrier_test.go diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go new file mode 100644 index 000000000000..becd54fd3ed3 --- /dev/null +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -0,0 +1,132 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package internal + +import ( + "context" + "encoding/base64" + + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/trace" + otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" +) + +// TODO: Move out of internal as part of open telemetry API + +// GRPCTraceBinPropagator is TextMapPropagator to propagate cross-cutting +// concerns as both text and binary key-value pairs within a carrier that +// travels in-band across process boundaries. +type GRPCTraceBinPropagator struct{} + +// Inject set cross-cutting concerns from the Context into the carrier. +// +// If carrier is carrier.CustomMapCarrier then SetBinary (fast path) is used, +// otherwise Set (slow path) with encoding is used. +func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier propagation.TextMapCarrier) { + span := trace.SpanFromContext(ctx) + if !span.SpanContext().IsValid() { + return + } + + binaryData := Binary(span.SpanContext()) + if binaryData == nil { + return + } + + if customCarrier, ok := carrier.(otelinternaltracing.CustomCarrier); ok { + customCarrier.SetBinary(binaryData) // fast path: set the binary data without encoding + } else { + carrier.Set(otelinternaltracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(binaryData)) // slow path: set the binary data with encoding + } +} + +// Extract reads cross-cutting concerns from the carrier into a Context. +// +// If carrier is carrier.CustomCarrier then GetBinary (fast path) is used, +// otherwise Get (slow path) with decoding is used. +func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier propagation.TextMapCarrier) context.Context { + var binaryData []byte + + if customCarrier, ok := carrier.(otelinternaltracing.CustomCarrier); ok { + binaryData, _ = customCarrier.GetBinary() + } else { + binaryData, _ = base64.StdEncoding.DecodeString(carrier.Get(otelinternaltracing.GRPCTraceBinHeaderKey)) + } + if binaryData == nil { + return ctx + } + + spanContext, ok := FromBinary([]byte(binaryData)) + if !ok { + return ctx + } + + return trace.ContextWithRemoteSpanContext(ctx, spanContext) +} + +// Fields returns the keys whose values are set with Inject. +// +// GRPCTraceBinPropagator will only have `grpc-trace-bin` field. +func (p GRPCTraceBinPropagator) Fields() []string { + return []string{otelinternaltracing.GRPCTraceBinHeaderKey} +} + +// Binary returns the binary format representation of a SpanContext. +// +// If sc is the zero value, Binary returns nil. +func Binary(sc trace.SpanContext) []byte { + if sc.Equal(trace.SpanContext{}) { + return nil + } + var b [29]byte + traceID := trace.TraceID(sc.TraceID()) + copy(b[2:18], traceID[:]) + b[18] = 1 + spanID := trace.SpanID(sc.SpanID()) + copy(b[19:27], spanID[:]) + b[27] = 2 + b[28] = uint8(trace.TraceFlags(sc.TraceFlags())) + return b[:] +} + +// FromBinary returns the SpanContext represented by b. +// +// If b has an unsupported version ID or contains no TraceID, FromBinary +// returns with ok==false. +func FromBinary(b []byte) (sc trace.SpanContext, ok bool) { + if len(b) == 0 || b[0] != 0 { + return trace.SpanContext{}, false + } + b = b[1:] + + if len(b) >= 17 && b[0] == 0 { + sc = sc.WithTraceID(trace.TraceID(b[1:17])) + b = b[17:] + } else { + return trace.SpanContext{}, false + } + if len(b) >= 9 && b[0] == 1 { + sc = sc.WithSpanID(trace.SpanID(b[1:9])) + b = b[9:] + } + if len(b) >= 2 && b[0] == 2 { + sc = sc.WithTraceFlags(trace.TraceFlags(b[1])) + } + return sc, true +} diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go new file mode 100644 index 000000000000..41b1303fc093 --- /dev/null +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -0,0 +1,116 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package internal + +import ( + "context" + "encoding/base64" + "testing" + + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/stats" + otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" +) + +// TODO: Move out of internal as part of open telemetry API + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +func (s) TestInject(t *testing.T) { + propagator := GRPCTraceBinPropagator{} + spanContext := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, + TraceFlags: trace.FlagsSampled, + }) + traceCtx, traceCancel := context.WithCancel(context.Background()) + traceCtx = trace.ContextWithSpanContext(traceCtx, spanContext) + + t.Run("Fast path with CustomCarrier", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + carrier := otelinternaltracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) + propagator.Inject(traceCtx, carrier) + + got := stats.OutgoingTrace(*carrier.Ctx) + want := Binary(spanContext) + if string(got) != string(want) { + t.Fatalf("got = %v, want %v", got, want) + } + cancel() + }) + + t.Run("Slow path with TextMapCarrier", func(t *testing.T) { + carrier := propagation.MapCarrier{} + propagator.Inject(traceCtx, carrier) + + got := carrier.Get(otelinternaltracing.GRPCTraceBinHeaderKey) + want := base64.StdEncoding.EncodeToString(Binary(spanContext)) + if got != want { + t.Fatalf("got = %v, want %v", got, want) + } + }) + + traceCancel() +} + +func (s) TestExtract(t *testing.T) { + propagator := GRPCTraceBinPropagator{} + spanContext := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, + TraceFlags: trace.FlagsSampled, + Remote: true, + }) + binaryData := Binary(spanContext) + + t.Run("Fast path with CustomCarrier", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + carrier := otelinternaltracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, binaryData)) + traceCtx := propagator.Extract(ctx, carrier) + got := trace.SpanContextFromContext(traceCtx) + + if !got.Equal(spanContext) { + t.Fatalf("got = %v, want %v", got, spanContext) + } + cancel() + }) + + t.Run("Slow path with TextMapCarrier", func(t *testing.T) { + carrier := propagation.MapCarrier{ + otelinternaltracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(binaryData), + } + ctx, cancel := context.WithCancel(context.Background()) + traceCtx := propagator.Extract(ctx, carrier) + got := trace.SpanContextFromContext(traceCtx) + + if !got.Equal(spanContext) { + t.Fatalf("got = %v, want %v", got, spanContext) + } + cancel() + }) +} diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go new file mode 100644 index 000000000000..5768e27487ab --- /dev/null +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -0,0 +1,103 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// Package tracing implements the OpenTelemetry carrier for context propagation +// in gRPC tracing. +package tracing + +import ( + "context" + "errors" + + "go.opentelemetry.io/otel/propagation" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/stats" +) + +// GRPCTraceBinHeaderKey is the gRPC metadata header key `grpc-trace-bin` used +// to propagate trace context in binary format. +const GRPCTraceBinHeaderKey = "grpc-trace-bin" + +// CustomCarrier is a TextMapCarrier that uses gRPC context to store and +// retrieve any propagated key-value pairs in text format along with binary +// format for `grpc-trace-bin` header +type CustomCarrier struct { + propagation.TextMapCarrier + + Ctx *context.Context +} + +// NewCustomCarrier creates a new CustomMapCarrier with +// the given context. +func NewCustomCarrier(ctx context.Context) CustomCarrier { + return CustomCarrier{ + Ctx: &ctx, + } +} + +// Get returns the string value associated with the passed key from the gRPC +// context. +func (c CustomCarrier) Get(key string) string { + md, ok := metadata.FromIncomingContext(*c.Ctx) + if !ok { + return "" + } + values := md.Get(key) + if len(values) == 0 { + return "" + } + return values[0] +} + +// Set stores the key-value pair in string format in the gRPC context. +// If the key already exists, its value will be overwritten. +func (c CustomCarrier) Set(key, value string) { + md, ok := metadata.FromOutgoingContext(*c.Ctx) + if !ok { + md = metadata.MD{} + } + md.Set(key, value) + *c.Ctx = metadata.NewOutgoingContext(*c.Ctx, md) +} + +// GetBinary returns the binary value from the gRPC context in the incoming RPC, +// associated with the header `grpc-trace-bin`. +func (c CustomCarrier) GetBinary() ([]byte, error) { + values := stats.Trace(*c.Ctx) + if len(values) == 0 { + return nil, errors.New("`grpc-trace-bin` header not found") + } + + return values, nil +} + +// SetBinary sets the binary value to the gRPC context, which will be sent in +// the outgoing RPC with the header grpc-trace-bin. +func (c CustomCarrier) SetBinary(value []byte) { + *c.Ctx = stats.SetTrace(*c.Ctx, value) +} + +// Keys lists the keys stored in the gRPC context for the outgoing RPC. +func (c CustomCarrier) Keys() []string { + md, _ := metadata.FromOutgoingContext(*c.Ctx) + keys := make([]string, 0, len(md)) + for k := range md { + keys = append(keys, k) + } + return keys +} diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go new file mode 100644 index 000000000000..932051328703 --- /dev/null +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -0,0 +1,202 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package tracing + +import ( + "context" + "reflect" + "testing" + + "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/stats" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +// sameElements checks if two string slices have the same elements, +// ignoring order. +func sameElements(a, b []string) bool { + if len(a) != len(b) { + return false + } + + countA := make(map[string]int) + countB := make(map[string]int) + for _, s := range a { + countA[s]++ + } + for _, s := range b { + countB[s]++ + } + + for k, v := range countA { + if countB[k] != v { + return false + } + } + return true +} + +func (s) TestGet(t *testing.T) { + tests := []struct { + name string + md metadata.MD + key string + want string + }{ + { + name: "existing key", + md: metadata.Pairs("key1", "value1"), + key: "key1", + want: "value1", + }, + { + name: "non-existing key", + md: metadata.Pairs("key1", "value1"), + key: "key2", + want: "", + }, + { + name: "empty key", + md: metadata.MD{}, + key: "key1", + want: "", + }, + } + + for _, tt := range tests { + ctx, cancel := context.WithCancel(context.Background()) + t.Run(tt.name, func(t *testing.T) { + c := NewCustomCarrier(metadata.NewIncomingContext(ctx, tt.md)) + got := c.Get(tt.key) + if got != tt.want { + t.Fatalf("got %s, want %s", got, tt.want) + } + cancel() + }) + } +} + +func (s) TestSet(t *testing.T) { + tests := []struct { + name string + initialMD metadata.MD // Metadata to initialize the context with + setKey string // Key to set using c.Set() + setValue string // Value to set using c.Set() + wantKeys []string // Expected keys returned by c.Keys() + }{ + { + name: "set new key", + initialMD: metadata.MD{}, + setKey: "key1", + setValue: "value1", + wantKeys: []string{"key1"}, + }, + { + name: "override existing key", + initialMD: metadata.MD{"key1": []string{"oldvalue"}}, + setKey: "key1", + setValue: "newvalue", + wantKeys: []string{"key1"}, + }, + { + name: "set key with existing unrelated key", + initialMD: metadata.MD{"key2": []string{"value2"}}, + setKey: "key1", + setValue: "value1", + wantKeys: []string{"key2", "key1"}, // Order matters here! + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, tt.initialMD)) + + c.Set(tt.setKey, tt.setValue) + + gotKeys := c.Keys() + if !sameElements(gotKeys, tt.wantKeys) { + t.Fatalf("got keys %v, want %v", gotKeys, tt.wantKeys) + } + gotMD, _ := metadata.FromOutgoingContext(*c.Ctx) + if gotMD.Get(tt.setKey)[0] != tt.setValue { + t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(tt.setKey)[0], tt.setValue, tt.setKey) + } + cancel() + }) + } +} + +func (s) TestGetBinary(t *testing.T) { + t.Run("get grpc-trace-bin header", func(t *testing.T) { + want := []byte{0x01, 0x02, 0x03} + ctx, cancel := context.WithCancel(context.Background()) + c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) + got, err := c.GetBinary() + if err != nil { + t.Fatalf("got error %v, want nil", err) + } + if string(got) != string(want) { + t.Fatalf("got %s, want %s", got, want) + } + cancel() + }) + + t.Run("get non grpc-trace-bin header", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + c := NewCustomCarrier(metadata.NewIncomingContext(ctx, metadata.Pairs("non-trace-bin", "\x01\x02\x03"))) + _, err := c.GetBinary() + if err == nil { + t.Fatalf("got nil error, want error") + } + cancel() + }) +} + +func (s) TestSetBinary(t *testing.T) { + t.Run("set grpc-trace-bin header", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + want := []byte{0x01, 0x02, 0x03} + c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) + c.SetBinary(want) + got := stats.OutgoingTrace(*c.Ctx) + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } + cancel() + }) + + t.Run("set non grpc-trace-bin header", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) + got := stats.OutgoingTrace(*c.Ctx) + if got != nil { + t.Fatalf("got %v, want nil", got) + } + cancel() + }) +} From edf16046c610b3b9c05fed7d1800b321fdb2469e Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Sun, 6 Oct 2024 22:22:29 +0530 Subject: [PATCH 02/20] Address style, clarification review comments --- .../internal/grpc_trace_bin_propagator.go | 106 +++++++++--------- .../grpc_trace_bin_propagator_test.go | 34 +++--- .../internal/tracing/custom_carrier.go | 38 ++++--- .../internal/tracing/custom_carrier_test.go | 18 +-- 4 files changed, 104 insertions(+), 92 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index becd54fd3ed3..966c09fbde42 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -22,111 +22,117 @@ import ( "context" "encoding/base64" - "go.opentelemetry.io/otel/propagation" - "go.opentelemetry.io/otel/trace" - otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" + otelpropagation "go.opentelemetry.io/otel/propagation" + oteltrace "go.opentelemetry.io/otel/trace" + internaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) // TODO: Move out of internal as part of open telemetry API -// GRPCTraceBinPropagator is TextMapPropagator to propagate cross-cutting -// concerns as both text and binary key-value pairs within a carrier that -// travels in-band across process boundaries. +// GRPCTraceBinPropagator is an OpenTelemetry TextMapPropagator which is used +// to extract and inject trace context data from and into messages exchanged by +// gRPC applications. It propagates trace data in binary format using the +// 'grpc-trace-bin' header. type GRPCTraceBinPropagator struct{} -// Inject set cross-cutting concerns from the Context into the carrier. +// Inject sets OpenTelemetry trace context information from the Context into +// the carrier. // -// If carrier is carrier.CustomMapCarrier then SetBinary (fast path) is used, -// otherwise Set (slow path) with encoding is used. -func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier propagation.TextMapCarrier) { - span := trace.SpanFromContext(ctx) +// If the carrier is a CustomCarrier, trace data is directly injected in a +// binary format using the 'grpc-trace-bin' header (fast path). Otherwise, +// the trace data is base64 encoded and injected using the same header in +// text format (slow path). +func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { + span := oteltrace.SpanFromContext(ctx) if !span.SpanContext().IsValid() { return } - binaryData := Binary(span.SpanContext()) - if binaryData == nil { + bd := Binary(span.SpanContext()) + if bd == nil { return } - if customCarrier, ok := carrier.(otelinternaltracing.CustomCarrier); ok { - customCarrier.SetBinary(binaryData) // fast path: set the binary data without encoding - } else { - carrier.Set(otelinternaltracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(binaryData)) // slow path: set the binary data with encoding + if cc, ok := carrier.(internaltracing.CustomCarrier); ok { + cc.SetBinary(bd) + return } + carrier.Set(internaltracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(bd)) } -// Extract reads cross-cutting concerns from the carrier into a Context. +// Extract reads OpenTelemetry trace context information from the carrier into a +// Context. // -// If carrier is carrier.CustomCarrier then GetBinary (fast path) is used, -// otherwise Get (slow path) with decoding is used. -func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier propagation.TextMapCarrier) context.Context { - var binaryData []byte - - if customCarrier, ok := carrier.(otelinternaltracing.CustomCarrier); ok { - binaryData, _ = customCarrier.GetBinary() +// If the carrier is a CustomCarrier, trace data is read directly in a binary +// format from the 'grpc-trace-bin' header (fast path). Otherwise, the trace +// data is base64 decoded from the same header in text format (slow path). +func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { + var bd []byte + + if cc, ok := carrier.(internaltracing.CustomCarrier); ok { + bd = cc.GetBinary() } else { - binaryData, _ = base64.StdEncoding.DecodeString(carrier.Get(otelinternaltracing.GRPCTraceBinHeaderKey)) + bd, _ = base64.StdEncoding.DecodeString(carrier.Get(internaltracing.GRPCTraceBinHeaderKey)) } - if binaryData == nil { + if bd == nil { return ctx } - spanContext, ok := FromBinary([]byte(binaryData)) + spanContext, ok := FromBinary([]byte(bd)) if !ok { return ctx } - return trace.ContextWithRemoteSpanContext(ctx, spanContext) + return oteltrace.ContextWithRemoteSpanContext(ctx, spanContext) } -// Fields returns the keys whose values are set with Inject. -// -// GRPCTraceBinPropagator will only have `grpc-trace-bin` field. +// Fields always returns a slice containing only `grpc-trace-bin` header key +// because the GRPCTraceBinPropagator only uses the 'grpc-trace-bin' header for +// propagating trace context. func (p GRPCTraceBinPropagator) Fields() []string { - return []string{otelinternaltracing.GRPCTraceBinHeaderKey} + return []string{internaltracing.GRPCTraceBinHeaderKey} } // Binary returns the binary format representation of a SpanContext. // -// If sc is the zero value, Binary returns nil. -func Binary(sc trace.SpanContext) []byte { - if sc.Equal(trace.SpanContext{}) { +// If sc is the zero value, returns nil. +func Binary(sc oteltrace.SpanContext) []byte { + if sc.Equal(oteltrace.SpanContext{}) { return nil } var b [29]byte - traceID := trace.TraceID(sc.TraceID()) + traceID := oteltrace.TraceID(sc.TraceID()) copy(b[2:18], traceID[:]) b[18] = 1 - spanID := trace.SpanID(sc.SpanID()) + spanID := oteltrace.SpanID(sc.SpanID()) copy(b[19:27], spanID[:]) b[27] = 2 - b[28] = uint8(trace.TraceFlags(sc.TraceFlags())) + b[28] = uint8(oteltrace.TraceFlags(sc.TraceFlags())) return b[:] } // FromBinary returns the SpanContext represented by b. // // If b has an unsupported version ID or contains no TraceID, FromBinary -// returns with ok==false. -func FromBinary(b []byte) (sc trace.SpanContext, ok bool) { +// returns with zero value SpanContext and false. +func FromBinary(b []byte) (oteltrace.SpanContext, bool) { if len(b) == 0 || b[0] != 0 { - return trace.SpanContext{}, false + return oteltrace.SpanContext{}, false } b = b[1:] - - if len(b) >= 17 && b[0] == 0 { - sc = sc.WithTraceID(trace.TraceID(b[1:17])) - b = b[17:] - } else { - return trace.SpanContext{}, false + if len(b) < 17 || b[0] != 0 { + return oteltrace.SpanContext{}, false } + + sc := oteltrace.SpanContext{} + sc = sc.WithTraceID(oteltrace.TraceID(b[1:17])) + b = b[17:] if len(b) >= 9 && b[0] == 1 { - sc = sc.WithSpanID(trace.SpanID(b[1:9])) + sc = sc.WithSpanID(oteltrace.SpanID(b[1:9])) b = b[9:] } if len(b) >= 2 && b[0] == 2 { - sc = sc.WithTraceFlags(trace.TraceFlags(b[1])) + sc = sc.WithTraceFlags(oteltrace.TraceFlags(b[1])) } return sc, true } diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 41b1303fc093..0a2d34c76bd6 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -23,12 +23,12 @@ import ( "encoding/base64" "testing" - "go.opentelemetry.io/otel/propagation" - "go.opentelemetry.io/otel/trace" + otelpropagation "go.opentelemetry.io/otel/propagation" + oteltrace "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" - otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" + internaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) // TODO: Move out of internal as part of open telemetry API @@ -43,20 +43,20 @@ func Test(t *testing.T) { func (s) TestInject(t *testing.T) { propagator := GRPCTraceBinPropagator{} - spanContext := trace.NewSpanContext(trace.SpanContextConfig{ + spanContext := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: trace.FlagsSampled, + TraceFlags: oteltrace.FlagsSampled, }) traceCtx, traceCancel := context.WithCancel(context.Background()) - traceCtx = trace.ContextWithSpanContext(traceCtx, spanContext) + traceCtx = oteltrace.ContextWithSpanContext(traceCtx, spanContext) t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - carrier := otelinternaltracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) + carrier := internaltracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) propagator.Inject(traceCtx, carrier) - got := stats.OutgoingTrace(*carrier.Ctx) + got := stats.OutgoingTrace(*carrier.Context()) want := Binary(spanContext) if string(got) != string(want) { t.Fatalf("got = %v, want %v", got, want) @@ -65,10 +65,10 @@ func (s) TestInject(t *testing.T) { }) t.Run("Slow path with TextMapCarrier", func(t *testing.T) { - carrier := propagation.MapCarrier{} + carrier := otelpropagation.MapCarrier{} propagator.Inject(traceCtx, carrier) - got := carrier.Get(otelinternaltracing.GRPCTraceBinHeaderKey) + got := carrier.Get(internaltracing.GRPCTraceBinHeaderKey) want := base64.StdEncoding.EncodeToString(Binary(spanContext)) if got != want { t.Fatalf("got = %v, want %v", got, want) @@ -80,19 +80,19 @@ func (s) TestInject(t *testing.T) { func (s) TestExtract(t *testing.T) { propagator := GRPCTraceBinPropagator{} - spanContext := trace.NewSpanContext(trace.SpanContextConfig{ + spanContext := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: trace.FlagsSampled, + TraceFlags: oteltrace.FlagsSampled, Remote: true, }) binaryData := Binary(spanContext) t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - carrier := otelinternaltracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, binaryData)) + carrier := internaltracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, binaryData)) traceCtx := propagator.Extract(ctx, carrier) - got := trace.SpanContextFromContext(traceCtx) + got := oteltrace.SpanContextFromContext(traceCtx) if !got.Equal(spanContext) { t.Fatalf("got = %v, want %v", got, spanContext) @@ -101,12 +101,12 @@ func (s) TestExtract(t *testing.T) { }) t.Run("Slow path with TextMapCarrier", func(t *testing.T) { - carrier := propagation.MapCarrier{ - otelinternaltracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(binaryData), + carrier := otelpropagation.MapCarrier{ + internaltracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(binaryData), } ctx, cancel := context.WithCancel(context.Background()) traceCtx := propagator.Extract(ctx, carrier) - got := trace.SpanContextFromContext(traceCtx) + got := oteltrace.SpanContextFromContext(traceCtx) if !got.Equal(spanContext) { t.Fatalf("got = %v, want %v", got, spanContext) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index 5768e27487ab..ef7878085c63 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -22,9 +22,8 @@ package tracing import ( "context" - "errors" - "go.opentelemetry.io/otel/propagation" + otelpropagation "go.opentelemetry.io/otel/propagation" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" ) @@ -37,23 +36,24 @@ const GRPCTraceBinHeaderKey = "grpc-trace-bin" // retrieve any propagated key-value pairs in text format along with binary // format for `grpc-trace-bin` header type CustomCarrier struct { - propagation.TextMapCarrier + otelpropagation.TextMapCarrier - Ctx *context.Context + ctx *context.Context } // NewCustomCarrier creates a new CustomMapCarrier with // the given context. func NewCustomCarrier(ctx context.Context) CustomCarrier { return CustomCarrier{ - Ctx: &ctx, + ctx: &ctx, } } // Get returns the string value associated with the passed key from the gRPC +// context. It returns an empty string if the key is not present in the // context. func (c CustomCarrier) Get(key string) string { - md, ok := metadata.FromIncomingContext(*c.Ctx) + md, ok := metadata.FromIncomingContext(*c.ctx) if !ok { return "" } @@ -67,37 +67,43 @@ func (c CustomCarrier) Get(key string) string { // Set stores the key-value pair in string format in the gRPC context. // If the key already exists, its value will be overwritten. func (c CustomCarrier) Set(key, value string) { - md, ok := metadata.FromOutgoingContext(*c.Ctx) + md, ok := metadata.FromOutgoingContext(*c.ctx) if !ok { md = metadata.MD{} } md.Set(key, value) - *c.Ctx = metadata.NewOutgoingContext(*c.Ctx, md) + *c.ctx = metadata.NewOutgoingContext(*c.ctx, md) } // GetBinary returns the binary value from the gRPC context in the incoming RPC, // associated with the header `grpc-trace-bin`. -func (c CustomCarrier) GetBinary() ([]byte, error) { - values := stats.Trace(*c.Ctx) +func (c CustomCarrier) GetBinary() []byte { + values := stats.Trace(*c.ctx) if len(values) == 0 { - return nil, errors.New("`grpc-trace-bin` header not found") + return nil } - return values, nil + return values } // SetBinary sets the binary value to the gRPC context, which will be sent in -// the outgoing RPC with the header grpc-trace-bin. +// the outgoing RPC with the header `grpc-trace-bin`. func (c CustomCarrier) SetBinary(value []byte) { - *c.Ctx = stats.SetTrace(*c.Ctx, value) + *c.ctx = stats.SetTrace(*c.ctx, value) } -// Keys lists the keys stored in the gRPC context for the outgoing RPC. +// Keys returns the keys stored in the gRPC context for the outgoing RPC. func (c CustomCarrier) Keys() []string { - md, _ := metadata.FromOutgoingContext(*c.Ctx) + md, _ := metadata.FromOutgoingContext(*c.ctx) keys := make([]string, 0, len(md)) for k := range md { keys = append(keys, k) } return keys } + +// Context returns the underlying *context.Context associated with the +// CustomCarrier. +func (c CustomCarrier) Context() *context.Context { + return c.ctx +} diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 932051328703..24a29bbad5f3 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -142,7 +142,7 @@ func (s) TestSet(t *testing.T) { if !sameElements(gotKeys, tt.wantKeys) { t.Fatalf("got keys %v, want %v", gotKeys, tt.wantKeys) } - gotMD, _ := metadata.FromOutgoingContext(*c.Ctx) + gotMD, _ := metadata.FromOutgoingContext(*c.Context()) if gotMD.Get(tt.setKey)[0] != tt.setValue { t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(tt.setKey)[0], tt.setValue, tt.setKey) } @@ -156,9 +156,9 @@ func (s) TestGetBinary(t *testing.T) { want := []byte{0x01, 0x02, 0x03} ctx, cancel := context.WithCancel(context.Background()) c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) - got, err := c.GetBinary() - if err != nil { - t.Fatalf("got error %v, want nil", err) + got := c.GetBinary() + if got == nil { + t.Fatalf("got nil, want %v", got) } if string(got) != string(want) { t.Fatalf("got %s, want %s", got, want) @@ -169,9 +169,9 @@ func (s) TestGetBinary(t *testing.T) { t.Run("get non grpc-trace-bin header", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) c := NewCustomCarrier(metadata.NewIncomingContext(ctx, metadata.Pairs("non-trace-bin", "\x01\x02\x03"))) - _, err := c.GetBinary() - if err == nil { - t.Fatalf("got nil error, want error") + got := c.GetBinary() + if got != nil { + t.Fatalf("got %v, want nil", got) } cancel() }) @@ -183,7 +183,7 @@ func (s) TestSetBinary(t *testing.T) { want := []byte{0x01, 0x02, 0x03} c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) c.SetBinary(want) - got := stats.OutgoingTrace(*c.Ctx) + got := stats.OutgoingTrace(*c.Context()) if !reflect.DeepEqual(got, want) { t.Fatalf("got %v, want %v", got, want) } @@ -193,7 +193,7 @@ func (s) TestSetBinary(t *testing.T) { t.Run("set non grpc-trace-bin header", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) - got := stats.OutgoingTrace(*c.Ctx) + got := stats.OutgoingTrace(*c.Context()) if got != nil { t.Fatalf("got %v, want nil", got) } From 5709a4333aa6b85cdfdb25eabc244fd275c11845 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 9 Oct 2024 14:08:35 +0530 Subject: [PATCH 03/20] Make *CustomCarrier implement carrier interface --- .../internal/grpc_trace_bin_propagator.go | 4 +-- .../grpc_trace_bin_propagator_test.go | 2 +- .../internal/tracing/custom_carrier.go | 30 +++++++++---------- .../internal/tracing/custom_carrier_test.go | 6 ++-- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index 966c09fbde42..745d7e350fbd 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -53,7 +53,7 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat return } - if cc, ok := carrier.(internaltracing.CustomCarrier); ok { + if cc, ok := carrier.(*internaltracing.CustomCarrier); ok { cc.SetBinary(bd) return } @@ -69,7 +69,7 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { var bd []byte - if cc, ok := carrier.(internaltracing.CustomCarrier); ok { + if cc, ok := carrier.(*internaltracing.CustomCarrier); ok { bd = cc.GetBinary() } else { bd, _ = base64.StdEncoding.DecodeString(carrier.Get(internaltracing.GRPCTraceBinHeaderKey)) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 0a2d34c76bd6..b5cce71a429a 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -56,7 +56,7 @@ func (s) TestInject(t *testing.T) { carrier := internaltracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) propagator.Inject(traceCtx, carrier) - got := stats.OutgoingTrace(*carrier.Context()) + got := stats.OutgoingTrace(carrier.Context()) want := Binary(spanContext) if string(got) != string(want) { t.Fatalf("got = %v, want %v", got, want) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index ef7878085c63..65a4a0762875 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -38,22 +38,22 @@ const GRPCTraceBinHeaderKey = "grpc-trace-bin" type CustomCarrier struct { otelpropagation.TextMapCarrier - ctx *context.Context + ctx context.Context } // NewCustomCarrier creates a new CustomMapCarrier with // the given context. -func NewCustomCarrier(ctx context.Context) CustomCarrier { - return CustomCarrier{ - ctx: &ctx, +func NewCustomCarrier(ctx context.Context) *CustomCarrier { + return &CustomCarrier{ + ctx: ctx, } } // Get returns the string value associated with the passed key from the gRPC // context. It returns an empty string if the key is not present in the // context. -func (c CustomCarrier) Get(key string) string { - md, ok := metadata.FromIncomingContext(*c.ctx) +func (c *CustomCarrier) Get(key string) string { + md, ok := metadata.FromIncomingContext(c.ctx) if !ok { return "" } @@ -66,19 +66,19 @@ func (c CustomCarrier) Get(key string) string { // Set stores the key-value pair in string format in the gRPC context. // If the key already exists, its value will be overwritten. -func (c CustomCarrier) Set(key, value string) { - md, ok := metadata.FromOutgoingContext(*c.ctx) +func (c *CustomCarrier) Set(key, value string) { + md, ok := metadata.FromOutgoingContext(c.ctx) if !ok { md = metadata.MD{} } md.Set(key, value) - *c.ctx = metadata.NewOutgoingContext(*c.ctx, md) + c.ctx = metadata.NewOutgoingContext(c.ctx, md) } // GetBinary returns the binary value from the gRPC context in the incoming RPC, // associated with the header `grpc-trace-bin`. func (c CustomCarrier) GetBinary() []byte { - values := stats.Trace(*c.ctx) + values := stats.Trace(c.ctx) if len(values) == 0 { return nil } @@ -88,13 +88,13 @@ func (c CustomCarrier) GetBinary() []byte { // SetBinary sets the binary value to the gRPC context, which will be sent in // the outgoing RPC with the header `grpc-trace-bin`. -func (c CustomCarrier) SetBinary(value []byte) { - *c.ctx = stats.SetTrace(*c.ctx, value) +func (c *CustomCarrier) SetBinary(value []byte) { + c.ctx = stats.SetTrace(c.ctx, value) } // Keys returns the keys stored in the gRPC context for the outgoing RPC. -func (c CustomCarrier) Keys() []string { - md, _ := metadata.FromOutgoingContext(*c.ctx) +func (c *CustomCarrier) Keys() []string { + md, _ := metadata.FromOutgoingContext(c.ctx) keys := make([]string, 0, len(md)) for k := range md { keys = append(keys, k) @@ -104,6 +104,6 @@ func (c CustomCarrier) Keys() []string { // Context returns the underlying *context.Context associated with the // CustomCarrier. -func (c CustomCarrier) Context() *context.Context { +func (c *CustomCarrier) Context() context.Context { return c.ctx } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 24a29bbad5f3..2a7622287f04 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -142,7 +142,7 @@ func (s) TestSet(t *testing.T) { if !sameElements(gotKeys, tt.wantKeys) { t.Fatalf("got keys %v, want %v", gotKeys, tt.wantKeys) } - gotMD, _ := metadata.FromOutgoingContext(*c.Context()) + gotMD, _ := metadata.FromOutgoingContext(c.Context()) if gotMD.Get(tt.setKey)[0] != tt.setValue { t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(tt.setKey)[0], tt.setValue, tt.setKey) } @@ -183,7 +183,7 @@ func (s) TestSetBinary(t *testing.T) { want := []byte{0x01, 0x02, 0x03} c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) c.SetBinary(want) - got := stats.OutgoingTrace(*c.Context()) + got := stats.OutgoingTrace(c.Context()) if !reflect.DeepEqual(got, want) { t.Fatalf("got %v, want %v", got, want) } @@ -193,7 +193,7 @@ func (s) TestSetBinary(t *testing.T) { t.Run("set non grpc-trace-bin header", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) - got := stats.OutgoingTrace(*c.Context()) + got := stats.OutgoingTrace(c.Context()) if got != nil { t.Fatalf("got %v, want nil", got) } From c97cbb51ab79b1d0abc12f3b0ed934d73f034461 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 9 Oct 2024 21:02:14 +0530 Subject: [PATCH 04/20] Address 2nd round of style, docstring comments --- .../internal/grpc_trace_bin_propagator.go | 22 +++--- .../grpc_trace_bin_propagator_test.go | 74 +++++++++++-------- .../internal/tracing/custom_carrier_test.go | 32 ++++---- 3 files changed, 71 insertions(+), 57 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index 745d7e350fbd..3fdf7a0e7314 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -24,7 +24,7 @@ import ( otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" - internaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" + itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) // TODO: Move out of internal as part of open telemetry API @@ -53,11 +53,11 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat return } - if cc, ok := carrier.(*internaltracing.CustomCarrier); ok { + if cc, ok := carrier.(*itracing.CustomCarrier); ok { cc.SetBinary(bd) return } - carrier.Set(internaltracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(bd)) + carrier.Set(itracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(bd)) } // Extract reads OpenTelemetry trace context information from the carrier into a @@ -69,28 +69,30 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { var bd []byte - if cc, ok := carrier.(*internaltracing.CustomCarrier); ok { + if cc, ok := carrier.(*itracing.CustomCarrier); ok { bd = cc.GetBinary() } else { - bd, _ = base64.StdEncoding.DecodeString(carrier.Get(internaltracing.GRPCTraceBinHeaderKey)) + bd, _ = base64.StdEncoding.DecodeString(carrier.Get(itracing.GRPCTraceBinHeaderKey)) } if bd == nil { return ctx } - spanContext, ok := FromBinary([]byte(bd)) + sc, ok := FromBinary([]byte(bd)) if !ok { return ctx } - return oteltrace.ContextWithRemoteSpanContext(ctx, spanContext) + return oteltrace.ContextWithRemoteSpanContext(ctx, sc) } -// Fields always returns a slice containing only `grpc-trace-bin` header key -// because the GRPCTraceBinPropagator only uses the 'grpc-trace-bin' header for +// Fields returns the keys whose values are set with Inject. +// +// GRPCTraceBinPropagator always returns a slice containing only +// `grpc-trace-bin` key because it only sets the 'grpc-trace-bin' header for // propagating trace context. func (p GRPCTraceBinPropagator) Fields() []string { - return []string{internaltracing.GRPCTraceBinHeaderKey} + return []string{itracing.GRPCTraceBinHeaderKey} } // Binary returns the binary format representation of a SpanContext. diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index b5cce71a429a..cf8f04c744d1 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -28,7 +28,7 @@ import ( "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" - internaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" + itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) // TODO: Move out of internal as part of open telemetry API @@ -41,75 +41,87 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } +// TestInject verifies that the GRPCTraceBinPropagator correctly injects +// OpenTelemetry trace context data. It tests both the fast path (using a +// CustomCarrier) and the slow path (using any other TextMapCarrier). +// +// The fast path injects the trace context directly in binary format where as +// the slow path base64 encodes the trace context before injecting it. func (s) TestInject(t *testing.T) { - propagator := GRPCTraceBinPropagator{} - spanContext := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ + p := GRPCTraceBinPropagator{} + sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, TraceFlags: oteltrace.FlagsSampled, }) - traceCtx, traceCancel := context.WithCancel(context.Background()) - traceCtx = oteltrace.ContextWithSpanContext(traceCtx, spanContext) + tCtx, tCancel := context.WithCancel(context.Background()) + tCtx = oteltrace.ContextWithSpanContext(tCtx, sc) + defer tCancel() t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - carrier := internaltracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) - propagator.Inject(traceCtx, carrier) + c := itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) + p.Inject(tCtx, c) - got := stats.OutgoingTrace(carrier.Context()) - want := Binary(spanContext) + got := stats.OutgoingTrace(c.Context()) + want := Binary(sc) if string(got) != string(want) { t.Fatalf("got = %v, want %v", got, want) } cancel() }) - t.Run("Slow path with TextMapCarrier", func(t *testing.T) { - carrier := otelpropagation.MapCarrier{} - propagator.Inject(traceCtx, carrier) + t.Run("Slow path with any Text Carrier", func(t *testing.T) { + c := otelpropagation.MapCarrier{} + p.Inject(tCtx, c) - got := carrier.Get(internaltracing.GRPCTraceBinHeaderKey) - want := base64.StdEncoding.EncodeToString(Binary(spanContext)) + got := c.Get(itracing.GRPCTraceBinHeaderKey) + want := base64.StdEncoding.EncodeToString(Binary(sc)) if got != want { t.Fatalf("got = %v, want %v", got, want) } }) - - traceCancel() } +// TestExtract verifies that the GRPCTraceBinPropagator correctly extracts +// OpenTelemetry trace context data. It tests both the fast path (using a +// CustomCarrier) and the slow path (using any other TextMapCarrier). +// +// The fast path extracts the trace context directly from the binary format +// where as the slow path base64 decodes the trace context before extracting +// it. func (s) TestExtract(t *testing.T) { - propagator := GRPCTraceBinPropagator{} - spanContext := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ + p := GRPCTraceBinPropagator{} + sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, TraceFlags: oteltrace.FlagsSampled, Remote: true, }) - binaryData := Binary(spanContext) + bd := Binary(sc) t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - carrier := internaltracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, binaryData)) - traceCtx := propagator.Extract(ctx, carrier) - got := oteltrace.SpanContextFromContext(traceCtx) + c := itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) + tCtx := p.Extract(ctx, c) + got := oteltrace.SpanContextFromContext(tCtx) - if !got.Equal(spanContext) { - t.Fatalf("got = %v, want %v", got, spanContext) + if !got.Equal(sc) { + t.Fatalf("got = %v, want %v", got, sc) } cancel() }) - t.Run("Slow path with TextMapCarrier", func(t *testing.T) { - carrier := otelpropagation.MapCarrier{ - internaltracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(binaryData), + t.Run("Slow path with any Text Carrier", func(t *testing.T) { + c := otelpropagation.MapCarrier{ + itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), } ctx, cancel := context.WithCancel(context.Background()) - traceCtx := propagator.Extract(ctx, carrier) - got := oteltrace.SpanContextFromContext(traceCtx) + tCtx := p.Extract(ctx, c) + got := oteltrace.SpanContextFromContext(tCtx) - if !got.Equal(spanContext) { - t.Fatalf("got = %v, want %v", got, spanContext) + if !got.Equal(sc) { + t.Fatalf("got = %v, want %v", got, sc) } cancel() }) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 2a7622287f04..0b7e34f1f241 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -6,7 +6,7 @@ * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * htestp://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, @@ -87,13 +87,13 @@ func (s) TestGet(t *testing.T) { }, } - for _, tt := range tests { + for _, test := range tests { ctx, cancel := context.WithCancel(context.Background()) - t.Run(tt.name, func(t *testing.T) { - c := NewCustomCarrier(metadata.NewIncomingContext(ctx, tt.md)) - got := c.Get(tt.key) - if got != tt.want { - t.Fatalf("got %s, want %s", got, tt.want) + t.Run(test.name, func(t *testing.T) { + c := NewCustomCarrier(metadata.NewIncomingContext(ctx, test.md)) + got := c.Get(test.key) + if got != test.want { + t.Fatalf("got %s, want %s", got, test.want) } cancel() }) @@ -127,24 +127,24 @@ func (s) TestSet(t *testing.T) { initialMD: metadata.MD{"key2": []string{"value2"}}, setKey: "key1", setValue: "value1", - wantKeys: []string{"key2", "key1"}, // Order matters here! + wantKeys: []string{"key2", "key1"}, // Order matesters here! }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, tt.initialMD)) + c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, test.initialMD)) - c.Set(tt.setKey, tt.setValue) + c.Set(test.setKey, test.setValue) gotKeys := c.Keys() - if !sameElements(gotKeys, tt.wantKeys) { - t.Fatalf("got keys %v, want %v", gotKeys, tt.wantKeys) + if !sameElements(gotKeys, test.wantKeys) { + t.Fatalf("got keys %v, want %v", gotKeys, test.wantKeys) } gotMD, _ := metadata.FromOutgoingContext(c.Context()) - if gotMD.Get(tt.setKey)[0] != tt.setValue { - t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(tt.setKey)[0], tt.setValue, tt.setKey) + if gotMD.Get(test.setKey)[0] != test.setValue { + t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(test.setKey)[0], test.setValue, test.setKey) } cancel() }) From 4144464543dc0977f0e8d5ca0ef3789b89473696 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Sun, 13 Oct 2024 15:00:50 +0530 Subject: [PATCH 05/20] use cmp.equal with cmpopts.SortSlices for equating keys slices --- .../grpc_trace_bin_propagator_test.go | 6 ++-- .../internal/tracing/custom_carrier_test.go | 31 ++++--------------- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index cf8f04c744d1..9dd31d951673 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -60,6 +60,7 @@ func (s) TestInject(t *testing.T) { t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) + defer cancel() c := itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) p.Inject(tCtx, c) @@ -68,7 +69,6 @@ func (s) TestInject(t *testing.T) { if string(got) != string(want) { t.Fatalf("got = %v, want %v", got, want) } - cancel() }) t.Run("Slow path with any Text Carrier", func(t *testing.T) { @@ -102,6 +102,7 @@ func (s) TestExtract(t *testing.T) { t.Run("Fast path with CustomCarrier", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) + defer cancel() c := itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) tCtx := p.Extract(ctx, c) got := oteltrace.SpanContextFromContext(tCtx) @@ -109,7 +110,6 @@ func (s) TestExtract(t *testing.T) { if !got.Equal(sc) { t.Fatalf("got = %v, want %v", got, sc) } - cancel() }) t.Run("Slow path with any Text Carrier", func(t *testing.T) { @@ -117,12 +117,12 @@ func (s) TestExtract(t *testing.T) { itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), } ctx, cancel := context.WithCancel(context.Background()) + defer cancel() tCtx := p.Extract(ctx, c) got := oteltrace.SpanContextFromContext(tCtx) if !got.Equal(sc) { t.Fatalf("got = %v, want %v", got, sc) } - cancel() }) } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 0b7e34f1f241..11cd1400abd2 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -23,6 +23,8 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" @@ -36,30 +38,6 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -// sameElements checks if two string slices have the same elements, -// ignoring order. -func sameElements(a, b []string) bool { - if len(a) != len(b) { - return false - } - - countA := make(map[string]int) - countB := make(map[string]int) - for _, s := range a { - countA[s]++ - } - for _, s := range b { - countB[s]++ - } - - for k, v := range countA { - if countB[k] != v { - return false - } - } - return true -} - func (s) TestGet(t *testing.T) { tests := []struct { name string @@ -139,7 +117,10 @@ func (s) TestSet(t *testing.T) { c.Set(test.setKey, test.setValue) gotKeys := c.Keys() - if !sameElements(gotKeys, test.wantKeys) { + equalKeys := cmp.Equal(test.wantKeys, gotKeys, cmpopts.SortSlices(func(a, b string) bool { + return a < b + })) + if !equalKeys { t.Fatalf("got keys %v, want %v", gotKeys, test.wantKeys) } gotMD, _ := metadata.FromOutgoingContext(c.Context()) From 4f49b4f3febb72aa66f254109088e025de437db8 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 16 Oct 2024 19:03:51 +0530 Subject: [PATCH 06/20] separate fast path and slow path tests --- .../grpc_trace_bin_propagator_test.go | 132 ++++++++++-------- 1 file changed, 77 insertions(+), 55 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 9dd31d951673..cf6ceeee09fb 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -41,13 +41,12 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -// TestInject verifies that the GRPCTraceBinPropagator correctly injects -// OpenTelemetry trace context data. It tests both the fast path (using a -// CustomCarrier) and the slow path (using any other TextMapCarrier). +// TestInject_FastPath verifies that the GRPCTraceBinPropagator correctly +// injects OpenTelemetry trace context data using the CustomCarrier. // -// The fast path injects the trace context directly in binary format where as -// the slow path base64 encodes the trace context before injecting it. -func (s) TestInject(t *testing.T) { +// It is called the fast path because it injects the trace context directly in +// binary format. +func (s) TestInject_FastPath(t *testing.T) { p := GRPCTraceBinPropagator{} sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, @@ -58,39 +57,50 @@ func (s) TestInject(t *testing.T) { tCtx = oteltrace.ContextWithSpanContext(tCtx, sc) defer tCancel() - t.Run("Fast path with CustomCarrier", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) - p.Inject(tCtx, c) - - got := stats.OutgoingTrace(c.Context()) - want := Binary(sc) - if string(got) != string(want) { - t.Fatalf("got = %v, want %v", got, want) - } - }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) + p.Inject(tCtx, c) - t.Run("Slow path with any Text Carrier", func(t *testing.T) { - c := otelpropagation.MapCarrier{} - p.Inject(tCtx, c) + got := stats.OutgoingTrace(c.Context()) + want := Binary(sc) + if string(got) != string(want) { + t.Fatalf("got = %v, want %v", got, want) + } +} - got := c.Get(itracing.GRPCTraceBinHeaderKey) - want := base64.StdEncoding.EncodeToString(Binary(sc)) - if got != want { - t.Fatalf("got = %v, want %v", got, want) - } +// TestInject_SlowPath verifies that the GRPCTraceBinPropagator correctly +// injects OpenTelemetry trace context data using any other text based carrier. +// +// It is called the slow path because it base64 encodes the binary trace +// context before injecting it. +func (s) TestInject_SlowPath(t *testing.T) { + p := GRPCTraceBinPropagator{} + sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ + TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, + TraceFlags: oteltrace.FlagsSampled, }) + tCtx, tCancel := context.WithCancel(context.Background()) + tCtx = oteltrace.ContextWithSpanContext(tCtx, sc) + defer tCancel() + + c := otelpropagation.MapCarrier{} + p.Inject(tCtx, c) + + got := c.Get(itracing.GRPCTraceBinHeaderKey) + want := base64.StdEncoding.EncodeToString(Binary(sc)) + if got != want { + t.Fatalf("got = %v, want %v", got, want) + } } -// TestExtract verifies that the GRPCTraceBinPropagator correctly extracts -// OpenTelemetry trace context data. It tests both the fast path (using a -// CustomCarrier) and the slow path (using any other TextMapCarrier). +// TestExtract_FastPath verifies that the GRPCTraceBinPropagator correctly +// extracts OpenTelemetry trace context data using the CustomCarrier. // -// The fast path extracts the trace context directly from the binary format -// where as the slow path base64 decodes the trace context before extracting -// it. -func (s) TestExtract(t *testing.T) { +// It is called the fast path because it extracts the trace context directly +// in the binary format. +func (s) TestExtract_FastPath(t *testing.T) { p := GRPCTraceBinPropagator{} sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, @@ -100,29 +110,41 @@ func (s) TestExtract(t *testing.T) { }) bd := Binary(sc) - t.Run("Fast path with CustomCarrier", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) - tCtx := p.Extract(ctx, c) - got := oteltrace.SpanContextFromContext(tCtx) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) + tCtx := p.Extract(ctx, c) + got := oteltrace.SpanContextFromContext(tCtx) - if !got.Equal(sc) { - t.Fatalf("got = %v, want %v", got, sc) - } - }) + if !got.Equal(sc) { + t.Fatalf("got = %v, want %v", got, sc) + } +} - t.Run("Slow path with any Text Carrier", func(t *testing.T) { - c := otelpropagation.MapCarrier{ - itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), - } - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - tCtx := p.Extract(ctx, c) - got := oteltrace.SpanContextFromContext(tCtx) - - if !got.Equal(sc) { - t.Fatalf("got = %v, want %v", got, sc) - } +// TestExtract_SlowPath verifies that the GRPCTraceBinPropagator correctly +// extracts OpenTelemetry trace context data using any other text based carrier. +// +// It is called the slow path because it base64 decodes the binary trace +// context before extracting it. +func (s) TestExtract_SlowPath(t *testing.T) { + p := GRPCTraceBinPropagator{} + sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ + TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, + TraceFlags: oteltrace.FlagsSampled, + Remote: true, }) + bd := Binary(sc) + + c := otelpropagation.MapCarrier{ + itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + tCtx := p.Extract(ctx, c) + got := oteltrace.SpanContextFromContext(tCtx) + + if !got.Equal(sc) { + t.Fatalf("got = %v, want %v", got, sc) + } } From 96abe67c648d43694b313ba0a293a9086c09527c Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Fri, 18 Oct 2024 15:14:31 +0530 Subject: [PATCH 07/20] Addressing documentation comments --- .../internal/grpc_trace_bin_propagator.go | 23 ++++++++------ .../internal/tracing/custom_carrier.go | 30 ++++++++----------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index 3fdf7a0e7314..b48e5cd3df64 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -30,18 +30,19 @@ import ( // TODO: Move out of internal as part of open telemetry API // GRPCTraceBinPropagator is an OpenTelemetry TextMapPropagator which is used -// to extract and inject trace context data from and into messages exchanged by +// to extract and inject trace context data from and into headers exchanged by // gRPC applications. It propagates trace data in binary format using the -// 'grpc-trace-bin' header. +// `grpc-trace-bin` header. type GRPCTraceBinPropagator struct{} // Inject sets OpenTelemetry trace context information from the Context into // the carrier. // // If the carrier is a CustomCarrier, trace data is directly injected in a -// binary format using the 'grpc-trace-bin' header (fast path). Otherwise, +// binary format using the `grpc-trace-bin` header (fast path). Otherwise, // the trace data is base64 encoded and injected using the same header in -// text format (slow path). +// text format (slow path). If span context is not valid or emptu, no data is +// injected. func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { span := oteltrace.SpanFromContext(ctx) if !span.SpanContext().IsValid() { @@ -64,11 +65,16 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat // Context. // // If the carrier is a CustomCarrier, trace data is read directly in a binary -// format from the 'grpc-trace-bin' header (fast path). Otherwise, the trace +// format from the `grpc-trace-bin` header (fast path). Otherwise, the trace // data is base64 decoded from the same header in text format (slow path). +// +// If a valid trace context is found, this function returns a new context +// derived from the input `ctx` containing the extracted span context. The +// extracted span context is marked as "remote", indicating that the trace +// originated from a different process or service. If trace context is invalid +// or not present, input `ctx` is returned as is. func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { var bd []byte - if cc, ok := carrier.(*itracing.CustomCarrier); ok { bd = cc.GetBinary() } else { @@ -82,14 +88,13 @@ func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropaga if !ok { return ctx } - return oteltrace.ContextWithRemoteSpanContext(ctx, sc) } // Fields returns the keys whose values are set with Inject. // // GRPCTraceBinPropagator always returns a slice containing only -// `grpc-trace-bin` key because it only sets the 'grpc-trace-bin' header for +// `grpc-trace-bin` key because it only sets the `grpc-trace-bin` header for // propagating trace context. func (p GRPCTraceBinPropagator) Fields() []string { return []string{itracing.GRPCTraceBinHeaderKey} @@ -109,7 +114,7 @@ func Binary(sc oteltrace.SpanContext) []byte { spanID := oteltrace.SpanID(sc.SpanID()) copy(b[19:27], spanID[:]) b[27] = 2 - b[28] = uint8(oteltrace.TraceFlags(sc.TraceFlags())) + b[28] = byte(oteltrace.TraceFlags(sc.TraceFlags())) return b[:] } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index 65a4a0762875..2efe0733a868 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -23,7 +23,6 @@ package tracing import ( "context" - otelpropagation "go.opentelemetry.io/otel/propagation" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" ) @@ -34,24 +33,20 @@ const GRPCTraceBinHeaderKey = "grpc-trace-bin" // CustomCarrier is a TextMapCarrier that uses gRPC context to store and // retrieve any propagated key-value pairs in text format along with binary -// format for `grpc-trace-bin` header +// format for `grpc-trace-bin` header. type CustomCarrier struct { - otelpropagation.TextMapCarrier - ctx context.Context } // NewCustomCarrier creates a new CustomMapCarrier with // the given context. func NewCustomCarrier(ctx context.Context) *CustomCarrier { - return &CustomCarrier{ - ctx: ctx, - } + return &CustomCarrier{ctx: ctx} } // Get returns the string value associated with the passed key from the gRPC // context. It returns an empty string if the key is not present in the -// context. +// context or if the value associated with the key is empty. func (c *CustomCarrier) Get(key string) string { md, ok := metadata.FromIncomingContext(c.ctx) if !ok { @@ -64,8 +59,8 @@ func (c *CustomCarrier) Get(key string) string { return values[0] } -// Set stores the key-value pair in string format in the gRPC context. -// If the key already exists, its value will be overwritten. +// Set stores the key-value pair in string format in the gRPC context. If the +// key already exists, its value will be overwritten. func (c *CustomCarrier) Set(key, value string) { md, ok := metadata.FromOutgoingContext(c.ctx) if !ok { @@ -75,24 +70,24 @@ func (c *CustomCarrier) Set(key, value string) { c.ctx = metadata.NewOutgoingContext(c.ctx, md) } -// GetBinary returns the binary value from the gRPC context in the incoming RPC, -// associated with the header `grpc-trace-bin`. +// GetBinary returns the binary value from the gRPC context in the incoming +// RPC, associated with the header `grpc-trace-bin`. If header is not found or +// is empty, it returns nil. func (c CustomCarrier) GetBinary() []byte { values := stats.Trace(c.ctx) if len(values) == 0 { return nil } - return values } -// SetBinary sets the binary value to the gRPC context, which will be sent in -// the outgoing RPC with the header `grpc-trace-bin`. +// SetBinary sets the binary value in the carrier's context, which will be sent +// in the outgoing RPC with the header `grpc-trace-bin`. func (c *CustomCarrier) SetBinary(value []byte) { c.ctx = stats.SetTrace(c.ctx, value) } -// Keys returns the keys stored in the gRPC context for the outgoing RPC. +// Keys returns the keys stored in the carrier's context. func (c *CustomCarrier) Keys() []string { md, _ := metadata.FromOutgoingContext(c.ctx) keys := make([]string, 0, len(md)) @@ -102,8 +97,7 @@ func (c *CustomCarrier) Keys() []string { return keys } -// Context returns the underlying *context.Context associated with the -// CustomCarrier. +// Context returns the underlying context associated with the CustomCarrier. func (c *CustomCarrier) Context() context.Context { return c.ctx } From 4839c45a06e823d0e289b0baf42d05cd672c5153 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Mon, 21 Oct 2024 00:32:29 +0530 Subject: [PATCH 08/20] rewrite FromBinary as C core and add unit tests --- .../internal/grpc_trace_bin_propagator.go | 43 ++++--- .../grpc_trace_bin_propagator_test.go | 110 +++++++++++++++++- 2 files changed, 126 insertions(+), 27 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index b48e5cd3df64..b32757c53a78 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -49,7 +49,7 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat return } - bd := Binary(span.SpanContext()) + bd := binary(span.SpanContext()) if bd == nil { return } @@ -84,7 +84,7 @@ func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropaga return ctx } - sc, ok := FromBinary([]byte(bd)) + sc, ok := fromBinary([]byte(bd)) if !ok { return ctx } @@ -103,7 +103,7 @@ func (p GRPCTraceBinPropagator) Fields() []string { // Binary returns the binary format representation of a SpanContext. // // If sc is the zero value, returns nil. -func Binary(sc oteltrace.SpanContext) []byte { +func binary(sc oteltrace.SpanContext) []byte { if sc.Equal(oteltrace.SpanContext{}) { return nil } @@ -118,28 +118,25 @@ func Binary(sc oteltrace.SpanContext) []byte { return b[:] } -// FromBinary returns the SpanContext represented by b. +// FromBinary returns the SpanContext represented by b withRemote set to true. // -// If b has an unsupported version ID or contains no TraceID, FromBinary -// returns with zero value SpanContext and false. -func FromBinary(b []byte) (oteltrace.SpanContext, bool) { - if len(b) == 0 || b[0] != 0 { - return oteltrace.SpanContext{}, false - } - b = b[1:] - if len(b) < 17 || b[0] != 0 { +// It returns with zero value SpanContext and false, if any of the +// below condition is not satisfied: +// - Valid header: len(b) = 29 +// - Valid version: b[0] = 0 +// - Valid traceID prefixed with 0: b[1] = 0 +// - Valid spanID prefixed with 1: b[18] = 1 +// - Valid traceFlags prefixed with 2: b[27] = 2 +func fromBinary(b []byte) (oteltrace.SpanContext, bool) { + if len(b) != 29 || b[0] != 0 || b[1] != 0 || b[18] != 1 || b[27] != 2 { return oteltrace.SpanContext{}, false } - sc := oteltrace.SpanContext{} - sc = sc.WithTraceID(oteltrace.TraceID(b[1:17])) - b = b[17:] - if len(b) >= 9 && b[0] == 1 { - sc = sc.WithSpanID(oteltrace.SpanID(b[1:9])) - b = b[9:] - } - if len(b) >= 2 && b[0] == 2 { - sc = sc.WithTraceFlags(oteltrace.TraceFlags(b[1])) - } - return sc, true + return oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID(b[2:18]), + ).WithSpanID( + oteltrace.SpanID(b[19:27]), + ).WithTraceFlags( + oteltrace.TraceFlags(b[28]), + ).WithRemote(true), true } diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index cf6ceeee09fb..17f5a3a1f524 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -21,6 +21,7 @@ package internal import ( "context" "encoding/base64" + "reflect" "testing" otelpropagation "go.opentelemetry.io/otel/propagation" @@ -63,7 +64,7 @@ func (s) TestInject_FastPath(t *testing.T) { p.Inject(tCtx, c) got := stats.OutgoingTrace(c.Context()) - want := Binary(sc) + want := binary(sc) if string(got) != string(want) { t.Fatalf("got = %v, want %v", got, want) } @@ -89,7 +90,7 @@ func (s) TestInject_SlowPath(t *testing.T) { p.Inject(tCtx, c) got := c.Get(itracing.GRPCTraceBinHeaderKey) - want := base64.StdEncoding.EncodeToString(Binary(sc)) + want := base64.StdEncoding.EncodeToString(binary(sc)) if got != want { t.Fatalf("got = %v, want %v", got, want) } @@ -108,7 +109,7 @@ func (s) TestExtract_FastPath(t *testing.T) { TraceFlags: oteltrace.FlagsSampled, Remote: true, }) - bd := Binary(sc) + bd := binary(sc) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -134,7 +135,7 @@ func (s) TestExtract_SlowPath(t *testing.T) { TraceFlags: oteltrace.FlagsSampled, Remote: true, }) - bd := Binary(sc) + bd := binary(sc) c := otelpropagation.MapCarrier{ itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), @@ -148,3 +149,104 @@ func (s) TestExtract_SlowPath(t *testing.T) { t.Fatalf("got = %v, want %v", got, sc) } } + +// TestBinary verifies that the binary() function correctly serializes a valid +// OpenTelemetry SpanContext into its binary format representation. +func (s) TestBinary(t *testing.T) { + tests := []struct { + name string + sc oteltrace.SpanContext + want []byte + }{ + { + name: "valid", + sc: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + ).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}, + ).WithTraceFlags( + oteltrace.TraceFlags(1), + ), + want: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + }, + { + name: "zero value", + sc: oteltrace.SpanContext{}, + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := binary(tt.sc); !reflect.DeepEqual(got, tt.want) { + t.Errorf("binary() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestFromBinary verifies that the fromBinary function correctly deserializes +// a binary format representation of a valid OpenTelemetry SpanContext into its +// corresponding SpanContext. +func (s) TestFromBinary(t *testing.T) { + tests := []struct { + name string + b []byte + want oteltrace.SpanContext + ok bool + }{ + { + name: "valid", + b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + want: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + ).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}, + ).WithTraceFlags( + oteltrace.TraceFlags(1), + ).WithRemote(true), + ok: true, + }, + { + name: "invalid length", + b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2}, + want: oteltrace.SpanContext{}, + ok: false, + }, + { + name: "invalid version", + b: []byte{1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + want: oteltrace.SpanContext{}, + ok: false, + }, + { + name: "invalid traceID field ID", + b: []byte{0, 1, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + want: oteltrace.SpanContext{}, + ok: false, + }, + { + name: "invalid spanID field ID", + b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 0, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + want: oteltrace.SpanContext{}, + ok: false, + }, + { + name: "invalid traceFlags field ID", + b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 1, 1}, + want: oteltrace.SpanContext{}, + ok: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := fromBinary(tt.b) + if ok != tt.ok { + t.Errorf("fromBinary() ok = %v, want %v", ok, tt.ok) + return + } + if !got.Equal(tt.want) { + t.Errorf("fromBinary() got = %v, want %v", got, tt.want) + } + }) + } +} From ce6cbd40af92624a6f6f3d4b40aaa96931a05647 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Mon, 21 Oct 2024 01:44:50 +0530 Subject: [PATCH 09/20] handle grpc-trace-bin keys for regular get and set --- .../internal/tracing/custom_carrier.go | 34 ++++- .../internal/tracing/custom_carrier_test.go | 117 +++++++++++------- 2 files changed, 103 insertions(+), 48 deletions(-) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index 2efe0733a868..0a19221ca48a 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -22,7 +22,10 @@ package tracing import ( "context" + "encoding/base64" + "strings" + "google.golang.org/grpc/grpclog" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" ) @@ -31,6 +34,8 @@ import ( // to propagate trace context in binary format. const GRPCTraceBinHeaderKey = "grpc-trace-bin" +var logger = grpclog.Component("otel-plugin") + // CustomCarrier is a TextMapCarrier that uses gRPC context to store and // retrieve any propagated key-value pairs in text format along with binary // format for `grpc-trace-bin` header. @@ -47,7 +52,18 @@ func NewCustomCarrier(ctx context.Context) *CustomCarrier { // Get returns the string value associated with the passed key from the gRPC // context. It returns an empty string if the key is not present in the // context or if the value associated with the key is empty. +// +// If the key is `grpc-trace-bin`, it retrieves the binary value using +// `c.GetBinary()` and then base64 encodes it before returning. For all other +// keys, it retrieves the value from the context's metadata. func (c *CustomCarrier) Get(key string) string { + if key == GRPCTraceBinHeaderKey { + return base64.StdEncoding.EncodeToString(c.GetBinary()) + } + if strings.HasSuffix(key, "bin") && key != GRPCTraceBinHeaderKey { + logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) + } + md, ok := metadata.FromIncomingContext(c.ctx) if !ok { return "" @@ -59,9 +75,23 @@ func (c *CustomCarrier) Get(key string) string { return values[0] } -// Set stores the key-value pair in string format in the gRPC context. If the -// key already exists, its value will be overwritten. +// Set stores the key-value pair in the gRPC context. If the key already +// exists, its value will be overwritten. +// +// If the key is `grpc-trace-bin`, it base64 decodes the `value` and stores the +// resulting binary data using `c.SetBinary()`. For all other keys, it stores +// the key-value pair in the string format in context's metadata. func (c *CustomCarrier) Set(key, value string) { + if key == GRPCTraceBinHeaderKey { + decodedValue, err := base64.StdEncoding.DecodeString(value) + if err != nil { + logger.Errorf("encountered error in decoding %s value", GRPCTraceBinHeaderKey) + return + } + c.SetBinary(decodedValue) + return + } + md, ok := metadata.FromOutgoingContext(c.ctx) if !ok { md = metadata.MD{} diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 11cd1400abd2..3f23b17cc64f 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -20,6 +20,7 @@ package tracing import ( "context" + "encoding/base64" "reflect" "testing" @@ -132,52 +133,76 @@ func (s) TestSet(t *testing.T) { } } -func (s) TestGetBinary(t *testing.T) { - t.Run("get grpc-trace-bin header", func(t *testing.T) { - want := []byte{0x01, 0x02, 0x03} - ctx, cancel := context.WithCancel(context.Background()) - c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) - got := c.GetBinary() - if got == nil { - t.Fatalf("got nil, want %v", got) - } - if string(got) != string(want) { - t.Fatalf("got %s, want %s", got, want) - } - cancel() - }) - - t.Run("get non grpc-trace-bin header", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - c := NewCustomCarrier(metadata.NewIncomingContext(ctx, metadata.Pairs("non-trace-bin", "\x01\x02\x03"))) - got := c.GetBinary() - if got != nil { - t.Fatalf("got %v, want nil", got) - } - cancel() - }) +func (s) TestGetBinary_GRPCTraceBinHeaderKey(t *testing.T) { + want := []byte{0x01, 0x02, 0x03} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) + + got := c.GetBinary() + if got == nil { + t.Fatalf("GetBinary() = nil, want %v", got) + } + if string(got) != string(want) { + t.Fatalf("GetBinary() = %s, want %s", got, want) + } + + // Regular Get() should return base64 encoded binary string + gotStr := c.Get(GRPCTraceBinHeaderKey) + wantStr := base64.StdEncoding.EncodeToString(want) + if gotStr != wantStr { + t.Fatalf("Get() = %s, want %s", gotStr, wantStr) + } } -func (s) TestSetBinary(t *testing.T) { - t.Run("set grpc-trace-bin header", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - want := []byte{0x01, 0x02, 0x03} - c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) - c.SetBinary(want) - got := stats.OutgoingTrace(c.Context()) - if !reflect.DeepEqual(got, want) { - t.Fatalf("got %v, want %v", got, want) - } - cancel() - }) - - t.Run("set non grpc-trace-bin header", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) - got := stats.OutgoingTrace(c.Context()) - if got != nil { - t.Fatalf("got %v, want nil", got) - } - cancel() - }) +func (s) TestGetBinary_NonGRPCTraceBinHeaderKey(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := NewCustomCarrier(metadata.NewIncomingContext(ctx, metadata.Pairs("non-trace-bin", "\x01\x02\x03"))) + + got := c.GetBinary() + if got != nil { + t.Fatalf("GetBinary() = %v, want nil", got) + } + + gotStr := c.Get(GRPCTraceBinHeaderKey) // regular Get() should return empty string for GRPCTraceBinHeaderKey + if gotStr != "" { + t.Fatalf("Get() = %s, want empty", gotStr) + } + gotStr = c.Get("non-trace-bin") // regular Get() should return the string value for non GRPCTraceBinHeaderKey + wantStr := "\x01\x02\x03" + if gotStr != wantStr { + t.Fatalf("Get() = %s, want %s", gotStr, wantStr) + } +} + +func (s) TestSetBinary_GRPCTraceBinHeaderKey(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + want := []byte{0x01, 0x02, 0x03} + c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) + + c.SetBinary(want) + got := stats.OutgoingTrace(c.Context()) + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } + + c = NewCustomCarrier(ctx) + c.Set(GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(want)) // regular Set() should decode base64 encoded binary string and call SetBinary() + got = stats.OutgoingTrace(c.Context()) + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} + +func (s) TestSetBinary_NonGRPCTraceBinHeaderKey(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) + + got := stats.OutgoingTrace(c.Context()) + if got != nil { + t.Fatalf("got %v, want nil", got) + } } From 59b87e7d23558163979c8839ca310f8b65fbfbe9 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Tue, 22 Oct 2024 23:16:14 +0530 Subject: [PATCH 10/20] Address nits --- .../internal/grpc_trace_bin_propagator.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index b32757c53a78..53b2114daa98 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -118,7 +118,7 @@ func binary(sc oteltrace.SpanContext) []byte { return b[:] } -// FromBinary returns the SpanContext represented by b withRemote set to true. +// FromBinary returns the SpanContext represented by b with Remote set to true. // // It returns with zero value SpanContext and false, if any of the // below condition is not satisfied: @@ -133,10 +133,7 @@ func fromBinary(b []byte) (oteltrace.SpanContext, bool) { } return oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID(b[2:18]), - ).WithSpanID( - oteltrace.SpanID(b[19:27]), - ).WithTraceFlags( - oteltrace.TraceFlags(b[28]), - ).WithRemote(true), true + oteltrace.TraceID(b[2:18])).WithSpanID( + oteltrace.SpanID(b[19:27])).WithTraceFlags( + oteltrace.TraceFlags(b[28])).WithRemote(true), true } From 26ee1b0732633a722b87797a4126dcaee0f84033 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Thu, 24 Oct 2024 16:49:53 +0530 Subject: [PATCH 11/20] don't allow any other binary header except grpc-trace-bin --- .../internal/tracing/custom_carrier.go | 9 +- .../internal/tracing/custom_carrier_test.go | 82 +++++++++---------- 2 files changed, 47 insertions(+), 44 deletions(-) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index 0a19221ca48a..a77fd2495b67 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -62,6 +62,7 @@ func (c *CustomCarrier) Get(key string) string { } if strings.HasSuffix(key, "bin") && key != GRPCTraceBinHeaderKey { logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) + return "" } md, ok := metadata.FromIncomingContext(c.ctx) @@ -83,12 +84,16 @@ func (c *CustomCarrier) Get(key string) string { // the key-value pair in the string format in context's metadata. func (c *CustomCarrier) Set(key, value string) { if key == GRPCTraceBinHeaderKey { - decodedValue, err := base64.StdEncoding.DecodeString(value) + b, err := base64.StdEncoding.DecodeString(value) if err != nil { logger.Errorf("encountered error in decoding %s value", GRPCTraceBinHeaderKey) return } - c.SetBinary(decodedValue) + c.SetBinary(b) + return + } + if strings.HasSuffix(key, "bin") && key != GRPCTraceBinHeaderKey { + logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) return } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 3f23b17cc64f..7c5c93935f0c 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -64,6 +64,12 @@ func (s) TestGet(t *testing.T) { key: "key1", want: "", }, + { + name: "non-grpc-trace-bin key", + md: metadata.Pairs("non-trace-bin", "\x01\x02\x03"), + key: "non-trace-bin", + want: "", + }, } for _, test := range tests { @@ -72,7 +78,7 @@ func (s) TestGet(t *testing.T) { c := NewCustomCarrier(metadata.NewIncomingContext(ctx, test.md)) got := c.Get(test.key) if got != test.want { - t.Fatalf("got %s, want %s", got, test.want) + t.Fatalf("Get() = %s, want %s", got, test.want) } cancel() }) @@ -82,16 +88,18 @@ func (s) TestGet(t *testing.T) { func (s) TestSet(t *testing.T) { tests := []struct { name string - initialMD metadata.MD // Metadata to initialize the context with - setKey string // Key to set using c.Set() - setValue string // Value to set using c.Set() - wantKeys []string // Expected keys returned by c.Keys() + initialMD metadata.MD + setKey string + setValue string + wantValue string // expected value of the set key + wantKeys []string }{ { name: "set new key", initialMD: metadata.MD{}, setKey: "key1", setValue: "value1", + wantValue: "value1", wantKeys: []string{"key1"}, }, { @@ -99,14 +107,24 @@ func (s) TestSet(t *testing.T) { initialMD: metadata.MD{"key1": []string{"oldvalue"}}, setKey: "key1", setValue: "newvalue", + wantValue: "newvalue", wantKeys: []string{"key1"}, }, { - name: "set key with existing unrelated key", + name: "set new key with different existing key", initialMD: metadata.MD{"key2": []string{"value2"}}, setKey: "key1", setValue: "value1", - wantKeys: []string{"key2", "key1"}, // Order matesters here! + wantValue: "value1", + wantKeys: []string{"key2", "key1"}, + }, + { + name: "set non-grcp-trace-bin binary key", + initialMD: metadata.MD{"key1": []string{"value1"}}, + setKey: "non-grcp-trace-bin", + setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + wantValue: "", // non-grcp-trace-bin key should not be set + wantKeys: []string{"key1"}, }, } @@ -125,7 +143,7 @@ func (s) TestSet(t *testing.T) { t.Fatalf("got keys %v, want %v", gotKeys, test.wantKeys) } gotMD, _ := metadata.FromOutgoingContext(c.Context()) - if gotMD.Get(test.setKey)[0] != test.setValue { + if test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(test.setKey)[0], test.setValue, test.setKey) } cancel() @@ -146,33 +164,18 @@ func (s) TestGetBinary_GRPCTraceBinHeaderKey(t *testing.T) { if string(got) != string(want) { t.Fatalf("GetBinary() = %s, want %s", got, want) } - - // Regular Get() should return base64 encoded binary string - gotStr := c.Get(GRPCTraceBinHeaderKey) - wantStr := base64.StdEncoding.EncodeToString(want) - if gotStr != wantStr { - t.Fatalf("Get() = %s, want %s", gotStr, wantStr) - } } -func (s) TestGetBinary_NonGRPCTraceBinHeaderKey(t *testing.T) { +func TestGet_GRPCTraceBinHeaderKey(t *testing.T) { + tc := []byte{0x01, 0x02, 0x03} ctx, cancel := context.WithCancel(context.Background()) defer cancel() - c := NewCustomCarrier(metadata.NewIncomingContext(ctx, metadata.Pairs("non-trace-bin", "\x01\x02\x03"))) - - got := c.GetBinary() - if got != nil { - t.Fatalf("GetBinary() = %v, want nil", got) - } + c := NewCustomCarrier(stats.SetIncomingTrace(ctx, tc)) - gotStr := c.Get(GRPCTraceBinHeaderKey) // regular Get() should return empty string for GRPCTraceBinHeaderKey - if gotStr != "" { - t.Fatalf("Get() = %s, want empty", gotStr) - } - gotStr = c.Get("non-trace-bin") // regular Get() should return the string value for non GRPCTraceBinHeaderKey - wantStr := "\x01\x02\x03" - if gotStr != wantStr { - t.Fatalf("Get() = %s, want %s", gotStr, wantStr) + got := c.Get(GRPCTraceBinHeaderKey) + want := base64.StdEncoding.EncodeToString(tc) + if got != want { + t.Fatalf("Get() = %s, want %s", got, want) } } @@ -180,29 +183,24 @@ func (s) TestSetBinary_GRPCTraceBinHeaderKey(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() want := []byte{0x01, 0x02, 0x03} - c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) + c := NewCustomCarrier(ctx) c.SetBinary(want) got := stats.OutgoingTrace(c.Context()) if !reflect.DeepEqual(got, want) { t.Fatalf("got %v, want %v", got, want) } - - c = NewCustomCarrier(ctx) - c.Set(GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(want)) // regular Set() should decode base64 encoded binary string and call SetBinary() - got = stats.OutgoingTrace(c.Context()) - if !reflect.DeepEqual(got, want) { - t.Fatalf("got %v, want %v", got, want) - } } -func (s) TestSetBinary_NonGRPCTraceBinHeaderKey(t *testing.T) { +func (s) TestSet_GRPCTraceBinHeaderKey(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{"non-trace-bin": []string{"value"}})) + want := []byte{0x01, 0x02, 0x03} + c := NewCustomCarrier(ctx) + c.Set(GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(want)) got := stats.OutgoingTrace(c.Context()) - if got != nil { - t.Fatalf("got %v, want nil", got) + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) } } From c5afe543c76ef8c070e91a75f328897897791070 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Fri, 25 Oct 2024 09:19:32 +0530 Subject: [PATCH 12/20] Suffix -bin instead of bin --- .../internal/tracing/custom_carrier.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index a77fd2495b67..60449dfc2c50 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -50,17 +50,19 @@ func NewCustomCarrier(ctx context.Context) *CustomCarrier { } // Get returns the string value associated with the passed key from the gRPC -// context. It returns an empty string if the key is not present in the -// context or if the value associated with the key is empty. +// context. It returns an empty string in any of the following cases: +// - the key is not present in the context +// - the value associated with the key is empty +// - the key ends with "-bin" and is not `grpc-trace-bin` // // If the key is `grpc-trace-bin`, it retrieves the binary value using // `c.GetBinary()` and then base64 encodes it before returning. For all other -// keys, it retrieves the value from the context's metadata. +// string keys, it retrieves the value from the context's metadata. func (c *CustomCarrier) Get(key string) string { if key == GRPCTraceBinHeaderKey { return base64.StdEncoding.EncodeToString(c.GetBinary()) } - if strings.HasSuffix(key, "bin") && key != GRPCTraceBinHeaderKey { + if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) return "" } @@ -77,7 +79,8 @@ func (c *CustomCarrier) Get(key string) string { } // Set stores the key-value pair in the gRPC context. If the key already -// exists, its value will be overwritten. +// exists, its value will be overwritten. If the key ends with "-bin" and is +// not `grpc-trace-bin`, the key-value pair is not stored. // // If the key is `grpc-trace-bin`, it base64 decodes the `value` and stores the // resulting binary data using `c.SetBinary()`. For all other keys, it stores @@ -92,7 +95,7 @@ func (c *CustomCarrier) Set(key, value string) { c.SetBinary(b) return } - if strings.HasSuffix(key, "bin") && key != GRPCTraceBinHeaderKey { + if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) return } @@ -108,7 +111,7 @@ func (c *CustomCarrier) Set(key, value string) { // GetBinary returns the binary value from the gRPC context in the incoming // RPC, associated with the header `grpc-trace-bin`. If header is not found or // is empty, it returns nil. -func (c CustomCarrier) GetBinary() []byte { +func (c *CustomCarrier) GetBinary() []byte { values := stats.Trace(c.ctx) if len(values) == 0 { return nil From 309dbf6efb83ad294c7b26315e961303bd3aba98 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Sat, 26 Oct 2024 23:54:24 +0530 Subject: [PATCH 13/20] address testing comments of merging in t-test and updating top level comments --- .../grpc_trace_bin_propagator_test.go | 283 ++++++++++-------- .../internal/tracing/custom_carrier.go | 13 +- .../internal/tracing/custom_carrier_test.go | 125 ++++---- 3 files changed, 244 insertions(+), 177 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 17f5a3a1f524..9ae96299efda 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -42,116 +42,157 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -// TestInject_FastPath verifies that the GRPCTraceBinPropagator correctly -// injects OpenTelemetry trace context data using the CustomCarrier. +// TestInject verifies that the GRPCTraceBinPropagator correctly injects +// OpenTelemetry span context as `grpc-trace-bin` header in the provided +// carrier's context, for both fast and slow path, if span context is valid. If +// span context is invalid, carrier's context is not modified which is verified +// by retrieving zero value span context from carrier's context. // -// It is called the fast path because it injects the trace context directly in -// binary format. -func (s) TestInject_FastPath(t *testing.T) { - p := GRPCTraceBinPropagator{} - sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ - TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: oteltrace.FlagsSampled, - }) - tCtx, tCancel := context.WithCancel(context.Background()) - tCtx = oteltrace.ContextWithSpanContext(tCtx, sc) - defer tCancel() - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) - p.Inject(tCtx, c) - - got := stats.OutgoingTrace(c.Context()) - want := binary(sc) - if string(got) != string(want) { - t.Fatalf("got = %v, want %v", got, want) - } -} - -// TestInject_SlowPath verifies that the GRPCTraceBinPropagator correctly -// injects OpenTelemetry trace context data using any other text based carrier. +// For fast path, it passes `CustomCarrier` as carrier to `Inject()` which +// injects the span context using `CustomCarrier.SetBinary()`. It then +// retrieves the injected span context using `stats.OutgoingTrace()` for +// verification because `SetBinary()` does injection directly in binary format. // -// It is called the slow path because it base64 encodes the binary trace -// context before injecting it. -func (s) TestInject_SlowPath(t *testing.T) { - p := GRPCTraceBinPropagator{} - sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ - TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: oteltrace.FlagsSampled, - }) - tCtx, tCancel := context.WithCancel(context.Background()) - tCtx = oteltrace.ContextWithSpanContext(tCtx, sc) - defer tCancel() - - c := otelpropagation.MapCarrier{} - p.Inject(tCtx, c) - - got := c.Get(itracing.GRPCTraceBinHeaderKey) - want := base64.StdEncoding.EncodeToString(binary(sc)) - if got != want { - t.Fatalf("got = %v, want %v", got, want) +// For slow path, it passes `otel.MapCarrier` as carrier to `Inject()` which +// injects the span context after base64 encoding as string using +// `carrier.Set()`. It then retrieves the injected span context using +// `carrier.Get()` for verification because `Set()` does injection in string +// format. +func (s) TestInject(t *testing.T) { + tests := []struct { + name string + sc oteltrace.SpanContext + fast bool + }{ + { + name: "fast path, valid context", + sc: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)), + fast: true, + }, + { + name: "fast path, invalid context", + sc: oteltrace.SpanContext{}, + fast: true, + }, + { + name: "slow path, valid context", + sc: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)), + fast: false, + }, + { + name: "slow path, invalid context", + sc: oteltrace.SpanContext{}, + fast: false, + }, } -} -// TestExtract_FastPath verifies that the GRPCTraceBinPropagator correctly -// extracts OpenTelemetry trace context data using the CustomCarrier. -// -// It is called the fast path because it extracts the trace context directly -// in the binary format. -func (s) TestExtract_FastPath(t *testing.T) { - p := GRPCTraceBinPropagator{} - sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ - TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: oteltrace.FlagsSampled, - Remote: true, - }) - bd := binary(sc) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) - tCtx := p.Extract(ctx, c) - got := oteltrace.SpanContextFromContext(tCtx) - - if !got.Equal(sc) { - t.Fatalf("got = %v, want %v", got, sc) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + p := GRPCTraceBinPropagator{} + tCtx, tCancel := context.WithCancel(context.Background()) + tCtx = oteltrace.ContextWithSpanContext(tCtx, test.sc) + defer tCancel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var c otelpropagation.TextMapCarrier + if test.fast { + c = itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) + } else { + c = otelpropagation.MapCarrier{} + } + p.Inject(tCtx, c) + var got oteltrace.SpanContext + if test.fast { + got, _ = fromBinary(stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())) + } else { + b, _ := base64.StdEncoding.DecodeString(c.Get(itracing.GRPCTraceBinHeaderKey)) + got, _ = fromBinary(b) + } + if test.sc.TraceID() != got.TraceID() && test.sc.SpanID() != got.SpanID() && test.sc.TraceFlags() != got.TraceFlags() { + t.Fatalf("got = %v, want %v", got, test.sc) + } + }) } } -// TestExtract_SlowPath verifies that the GRPCTraceBinPropagator correctly -// extracts OpenTelemetry trace context data using any other text based carrier. +// TestExtract verifies that the GRPCTraceBinPropagator correctly extracts +// OpenTelemetry span context data for fast and slow path, if valid span +// context was injected. If invalid context was injected, it verifies that a +// zero value span context was retrieved. +// +// For fast path, it uses the CustomCarrier and sets the span context in the +// binary format using `stats.SetIncomingTrace` in its context and then +// verifies that same span context is extracted directly in binary foramt. // -// It is called the slow path because it base64 decodes the binary trace -// context before extracting it. -func (s) TestExtract_SlowPath(t *testing.T) { - p := GRPCTraceBinPropagator{} - sc := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ - TraceID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - SpanID: [8]byte{17, 18, 19, 20, 21, 22, 23, 24}, - TraceFlags: oteltrace.FlagsSampled, - Remote: true, - }) - bd := binary(sc) - - c := otelpropagation.MapCarrier{ - itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd), +// For slow path, it uses a MapCarrier and sets base64 encoded span context in +// its context and then verifies that same span context is extracted after +// base64 decoding. +func (s) TestExtract(t *testing.T) { + tests := []struct { + name string + sc oteltrace.SpanContext + fast bool + }{ + { + name: "fast path, valid context", + sc: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)).WithRemote(true), + fast: true, + }, + { + name: "fast path, invalid context", + sc: oteltrace.SpanContext{}, + fast: true, + }, + { + name: "slow path, valid context", + sc: oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)).WithRemote(true), + fast: false, + }, + { + name: "slow path, invalid context", + sc: oteltrace.SpanContext{}, + fast: false, + }, } - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - tCtx := p.Extract(ctx, c) - got := oteltrace.SpanContextFromContext(tCtx) - if !got.Equal(sc) { - t.Fatalf("got = %v, want %v", got, sc) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + p := GRPCTraceBinPropagator{} + bd := binary(test.sc) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var c otelpropagation.TextMapCarrier + if test.fast { + c = itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) + } else { + c = otelpropagation.MapCarrier{itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd)} + } + tCtx := p.Extract(ctx, c) + got := oteltrace.SpanContextFromContext(tCtx) + if !got.Equal(test.sc) { + t.Fatalf("got = %v, want %v", got, test.sc) + } + }) } } // TestBinary verifies that the binary() function correctly serializes a valid -// OpenTelemetry SpanContext into its binary format representation. +// OpenTelemetry span context into its binary format representation. If span +// context is invalid, it verifies that serialization is nil. func (s) TestBinary(t *testing.T) { tests := []struct { name string @@ -159,34 +200,37 @@ func (s) TestBinary(t *testing.T) { want []byte }{ { - name: "valid", + name: "valid context", sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - ).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}, - ).WithTraceFlags( - oteltrace.TraceFlags(1), - ), - want: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)), + want: binary(oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1))), }, { - name: "zero value", + name: "zero value context", sc: oteltrace.SpanContext{}, want: nil, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := binary(tt.sc); !reflect.DeepEqual(got, tt.want) { - t.Errorf("binary() = %v, want %v", got, tt.want) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := binary(test.sc); !reflect.DeepEqual(got, test.want) { + t.Errorf("binary() = %v, want %v", got, test.want) } }) } } -// TestFromBinary verifies that the fromBinary function correctly deserializes -// a binary format representation of a valid OpenTelemetry SpanContext into its -// corresponding SpanContext. +// TestFromBinary verifies that the fromBinary() function correctly +// deserializes a binary format representation of a valid OpenTelemetry span +// context into its corresponding span context format. If span context's binary +// representation is invalid, it verifies that deserialization is zero value +// span context. func (s) TestFromBinary(t *testing.T) { tests := []struct { name string @@ -237,15 +281,16 @@ func (s) TestFromBinary(t *testing.T) { ok: false, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, ok := fromBinary(tt.b) - if ok != tt.ok { - t.Errorf("fromBinary() ok = %v, want %v", ok, tt.ok) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, ok := fromBinary(test.b) + if ok != test.ok { + t.Errorf("fromBinary() ok = %v, want %v", ok, test.ok) return } - if !got.Equal(tt.want) { - t.Errorf("fromBinary() got = %v, want %v", got, tt.want) + if !got.Equal(test.want) { + t.Errorf("fromBinary() got = %v, want %v", got, test.want) } }) } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index 60449dfc2c50..b45382de4a23 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -56,11 +56,11 @@ func NewCustomCarrier(ctx context.Context) *CustomCarrier { // - the key ends with "-bin" and is not `grpc-trace-bin` // // If the key is `grpc-trace-bin`, it retrieves the binary value using -// `c.GetBinary()` and then base64 encodes it before returning. For all other +// `stats.Trace()` and then base64 encodes it before returning. For all other // string keys, it retrieves the value from the context's metadata. func (c *CustomCarrier) Get(key string) string { if key == GRPCTraceBinHeaderKey { - return base64.StdEncoding.EncodeToString(c.GetBinary()) + return base64.StdEncoding.EncodeToString(stats.Trace(c.ctx)) } if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) @@ -83,7 +83,7 @@ func (c *CustomCarrier) Get(key string) string { // not `grpc-trace-bin`, the key-value pair is not stored. // // If the key is `grpc-trace-bin`, it base64 decodes the `value` and stores the -// resulting binary data using `c.SetBinary()`. For all other keys, it stores +// resulting binary data using `stats.SetTrace()`. For all other keys, it stores // the key-value pair in the string format in context's metadata. func (c *CustomCarrier) Set(key, value string) { if key == GRPCTraceBinHeaderKey { @@ -92,7 +92,7 @@ func (c *CustomCarrier) Set(key, value string) { logger.Errorf("encountered error in decoding %s value", GRPCTraceBinHeaderKey) return } - c.SetBinary(b) + c.ctx = stats.SetTrace(c.ctx, b) return } if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { @@ -128,10 +128,13 @@ func (c *CustomCarrier) SetBinary(value []byte) { // Keys returns the keys stored in the carrier's context. func (c *CustomCarrier) Keys() []string { md, _ := metadata.FromOutgoingContext(c.ctx) - keys := make([]string, 0, len(md)) + keys := make([]string, 0, len(md)+1) // +1 for grpc-trace-bin (if present) for k := range md { keys = append(keys, k) } + if stats.OutgoingTrace(c.ctx) != nil { + keys = append(keys, GRPCTraceBinHeaderKey) // add grpc-trace-bin key if grpc-trace-bin is present in the context + } return keys } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 7c5c93935f0c..87de92692a6b 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -22,6 +22,7 @@ import ( "context" "encoding/base64" "reflect" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -39,6 +40,24 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } +func verifyOutgoingTraceGRPCTraceBinHeader(ctx context.Context, t *testing.T, wantB []byte) { + gotB := stats.OutgoingTrace(ctx) + if len(wantB) == 0 && gotB != nil { + t.Fatalf("stats.OutgoingTrace() is non-nil, want nil") + } + if gotB != nil && !reflect.DeepEqual(gotB, wantB) { + t.Fatalf("stats.OutgoingTrace() = %v, want %v", gotB, wantB) + } +} + +// TestGet verifies that `CustomCarrier.Get()` returns correct value for the +// corresponding key in the carrier's context, if key is present. If key is not +// present, it verifies that empty string is returned. +// +// If key ends with `-bin`, it verifies that a correct base64 encoded +// string value is returned for the `grpc-trace-bin` header's binary value, if +// present. For any other `-bin` key, it verifies that an empty string is +// returned. func (s) TestGet(t *testing.T) { tests := []struct { name string @@ -64,27 +83,48 @@ func (s) TestGet(t *testing.T) { key: "key1", want: "", }, + { + name: "grpc-trace-bin key", + md: metadata.MD{}, + key: "grpc-trace-bin", + want: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + }, { name: "non-grpc-trace-bin key", - md: metadata.Pairs("non-trace-bin", "\x01\x02\x03"), + md: metadata.Pairs("non-trace-bin", base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03})), key: "non-trace-bin", - want: "", + want: "", // -bin key that isn't grpc-trace-bin should always get empty string }, } for _, test := range tests { - ctx, cancel := context.WithCancel(context.Background()) t.Run(test.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if strings.HasSuffix(test.key, "-bin") { // for binary headers set `grpc-trace-bin` using `stats.SetIncomingTrace()` + b, _ := base64.StdEncoding.DecodeString(test.want) + ctx = stats.SetIncomingTrace(ctx, b) + } c := NewCustomCarrier(metadata.NewIncomingContext(ctx, test.md)) got := c.Get(test.key) if got != test.want { t.Fatalf("Get() = %s, want %s", got, test.want) } - cancel() }) } } +// TestSet verifies that a key-value pair is set in carrier's context using +// `CustomCarrier.Set()`. If key is not present, it verifies that key-value +// pair is insterted. If key is already present, it verifies that value is +// overwritten. +// +// If key ends with `-bin`, it verifies that a binary value is set for +// `grpc-trace-bin` header, if key is `grpc-trace-bin` and value is base64 +// encoded. For any other `-bin` key, it verifies that no binary value is set. +// +// It also verifies that both existing and newly inserted keys are present in +// the carrier's context using `CustomCarrier.Keys()`. func (s) TestSet(t *testing.T) { tests := []struct { name string @@ -119,11 +159,19 @@ func (s) TestSet(t *testing.T) { wantKeys: []string{"key2", "key1"}, }, { - name: "set non-grcp-trace-bin binary key", + name: "set grpc-trace-bin binary key", + initialMD: metadata.MD{"key1": []string{"value1"}}, + setKey: "grpc-trace-bin", + setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + wantValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + wantKeys: []string{"key1", "grpc-trace-bin"}, + }, + { + name: "set non-grpc-trace-bin binary key", initialMD: metadata.MD{"key1": []string{"value1"}}, - setKey: "non-grcp-trace-bin", + setKey: "non-grpc-trace-bin", setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), - wantValue: "", // non-grcp-trace-bin key should not be set + wantValue: "", // -bin key that isn't grpc-trace-bin should never be set wantKeys: []string{"key1"}, }, } @@ -131,76 +179,47 @@ func (s) TestSet(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) + defer cancel() c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, test.initialMD)) - c.Set(test.setKey, test.setValue) - gotKeys := c.Keys() - equalKeys := cmp.Equal(test.wantKeys, gotKeys, cmpopts.SortSlices(func(a, b string) bool { - return a < b - })) + equalKeys := cmp.Equal(test.wantKeys, gotKeys, cmpopts.SortSlices(func(a, b string) bool { return a < b })) if !equalKeys { - t.Fatalf("got keys %v, want %v", gotKeys, test.wantKeys) + t.Fatalf("c.Keys() = keys %v, want %v", gotKeys, test.wantKeys) + } + // for binary headers verify `grpc-trace-bin` binary value for outgoing trace in carrier's context + if strings.HasSuffix(test.setKey, "-bin") { + wantB, _ := base64.StdEncoding.DecodeString(test.wantValue) + verifyOutgoingTraceGRPCTraceBinHeader(c.ctx, t, wantB) + return } - gotMD, _ := metadata.FromOutgoingContext(c.Context()) - if test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { + // for non -bin headers verify string value in carrier's context metadata + if gotMD, _ := metadata.FromOutgoingContext(c.Context()); test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(test.setKey)[0], test.setValue, test.setKey) } - cancel() }) } } -func (s) TestGetBinary_GRPCTraceBinHeaderKey(t *testing.T) { +func (s) TestGetBinary(t *testing.T) { want := []byte{0x01, 0x02, 0x03} ctx, cancel := context.WithCancel(context.Background()) defer cancel() c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) - got := c.GetBinary() if got == nil { - t.Fatalf("GetBinary() = nil, want %v", got) + t.Fatalf("c.GetBinary() = nil, want %v", got) } - if string(got) != string(want) { - t.Fatalf("GetBinary() = %s, want %s", got, want) + if !reflect.DeepEqual(want, got) { + t.Fatalf("c.GetBinary() = %s, want %s", got, want) } } -func TestGet_GRPCTraceBinHeaderKey(t *testing.T) { - tc := []byte{0x01, 0x02, 0x03} - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := NewCustomCarrier(stats.SetIncomingTrace(ctx, tc)) - - got := c.Get(GRPCTraceBinHeaderKey) - want := base64.StdEncoding.EncodeToString(tc) - if got != want { - t.Fatalf("Get() = %s, want %s", got, want) - } -} - -func (s) TestSetBinary_GRPCTraceBinHeaderKey(t *testing.T) { +func (s) TestSetBinary(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() want := []byte{0x01, 0x02, 0x03} c := NewCustomCarrier(ctx) - c.SetBinary(want) - got := stats.OutgoingTrace(c.Context()) - if !reflect.DeepEqual(got, want) { - t.Fatalf("got %v, want %v", got, want) - } -} - -func (s) TestSet_GRPCTraceBinHeaderKey(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - want := []byte{0x01, 0x02, 0x03} - c := NewCustomCarrier(ctx) - - c.Set(GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(want)) - got := stats.OutgoingTrace(c.Context()) - if !reflect.DeepEqual(got, want) { - t.Fatalf("got %v, want %v", got, want) - } + verifyOutgoingTraceGRPCTraceBinHeader(c.Context(), t, want) } From 760330c4b31ec66d16d375fac63860fb3149be41 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Tue, 5 Nov 2024 17:14:23 +0530 Subject: [PATCH 14/20] error and naming suggestions for tests --- .../grpc_trace_bin_propagator_test.go | 52 ++++++------------- .../internal/tracing/custom_carrier_test.go | 24 ++++++--- 2 files changed, 35 insertions(+), 41 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 9ae96299efda..88638bbfeb0b 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -34,6 +34,12 @@ import ( // TODO: Move out of internal as part of open telemetry API +// validSpanContext is a valid OpenTelemetry span context. +var validSpanContext = oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)) + type s struct { grpctest.Tester } @@ -66,10 +72,7 @@ func (s) TestInject(t *testing.T) { }{ { name: "fast path, valid context", - sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)), + sc: validSpanContext, fast: true, }, { @@ -79,10 +82,7 @@ func (s) TestInject(t *testing.T) { }, { name: "slow path, valid context", - sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)), + sc: validSpanContext, fast: false, }, { @@ -142,10 +142,7 @@ func (s) TestExtract(t *testing.T) { }{ { name: "fast path, valid context", - sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)).WithRemote(true), + sc: validSpanContext.WithRemote(true), fast: true, }, { @@ -155,10 +152,7 @@ func (s) TestExtract(t *testing.T) { }, { name: "slow path, valid context", - sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)).WithRemote(true), + sc: validSpanContext.WithRemote(true), fast: false, }, { @@ -201,14 +195,8 @@ func (s) TestBinary(t *testing.T) { }{ { name: "valid context", - sc: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)), - want: binary(oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1))), + sc: validSpanContext, + want: binary(validSpanContext), }, { name: "zero value context", @@ -220,7 +208,7 @@ func (s) TestBinary(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { if got := binary(test.sc); !reflect.DeepEqual(got, test.want) { - t.Errorf("binary() = %v, want %v", got, test.want) + t.Fatalf("binary() = %v, want %v", got, test.want) } }) } @@ -241,14 +229,8 @@ func (s) TestFromBinary(t *testing.T) { { name: "valid", b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, - want: oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - ).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}, - ).WithTraceFlags( - oteltrace.TraceFlags(1), - ).WithRemote(true), - ok: true, + want: validSpanContext.WithRemote(true), + ok: true, }, { name: "invalid length", @@ -286,11 +268,11 @@ func (s) TestFromBinary(t *testing.T) { t.Run(test.name, func(t *testing.T) { got, ok := fromBinary(test.b) if ok != test.ok { - t.Errorf("fromBinary() ok = %v, want %v", ok, test.ok) + t.Fatalf("fromBinary() ok = %v, want %v", ok, test.ok) return } if !got.Equal(test.want) { - t.Errorf("fromBinary() got = %v, want %v", got, test.want) + t.Fatalf("fromBinary() got = %v, want %v", got, test.want) } }) } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 87de92692a6b..c5faa2c8bb19 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -89,6 +89,12 @@ func (s) TestGet(t *testing.T) { key: "grpc-trace-bin", want: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), }, + { + name: "grpc-trace-bin key with another string key", + md: metadata.Pairs("key1", "value1"), + key: "grpc-trace-bin", + want: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + }, { name: "non-grpc-trace-bin key", md: metadata.Pairs("non-trace-bin", base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03})), @@ -102,7 +108,10 @@ func (s) TestGet(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() if strings.HasSuffix(test.key, "-bin") { // for binary headers set `grpc-trace-bin` using `stats.SetIncomingTrace()` - b, _ := base64.StdEncoding.DecodeString(test.want) + b, err := base64.StdEncoding.DecodeString(test.want) + if err != nil { + t.Fatalf("failed to decode want %s as base64 string: %v", test.want, err) + } ctx = stats.SetIncomingTrace(ctx, b) } c := NewCustomCarrier(metadata.NewIncomingContext(ctx, test.md)) @@ -135,7 +144,7 @@ func (s) TestSet(t *testing.T) { wantKeys []string }{ { - name: "set new key", + name: "new key", initialMD: metadata.MD{}, setKey: "key1", setValue: "value1", @@ -151,7 +160,7 @@ func (s) TestSet(t *testing.T) { wantKeys: []string{"key1"}, }, { - name: "set new key with different existing key", + name: "new key with different existing key", initialMD: metadata.MD{"key2": []string{"value2"}}, setKey: "key1", setValue: "value1", @@ -159,7 +168,7 @@ func (s) TestSet(t *testing.T) { wantKeys: []string{"key2", "key1"}, }, { - name: "set grpc-trace-bin binary key", + name: "grpc-trace-bin binary key", initialMD: metadata.MD{"key1": []string{"value1"}}, setKey: "grpc-trace-bin", setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), @@ -167,7 +176,7 @@ func (s) TestSet(t *testing.T) { wantKeys: []string{"key1", "grpc-trace-bin"}, }, { - name: "set non-grpc-trace-bin binary key", + name: "non-grpc-trace-bin binary key", initialMD: metadata.MD{"key1": []string{"value1"}}, setKey: "non-grpc-trace-bin", setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), @@ -189,7 +198,10 @@ func (s) TestSet(t *testing.T) { } // for binary headers verify `grpc-trace-bin` binary value for outgoing trace in carrier's context if strings.HasSuffix(test.setKey, "-bin") { - wantB, _ := base64.StdEncoding.DecodeString(test.wantValue) + wantB, err := base64.StdEncoding.DecodeString(test.wantValue) + if err != nil { + t.Fatalf("failed to decode want %s as base64 string: %v", test.wantValue, err) + } verifyOutgoingTraceGRPCTraceBinHeader(c.ctx, t, wantB) return } From 38cde19365ecc619f35c35b73b26aed4ac1d6e96 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Tue, 5 Nov 2024 18:16:59 +0530 Subject: [PATCH 15/20] handle success bool and err separately in tests --- .../grpc_trace_bin_propagator_test.go | 56 ++++++++++++------- .../internal/tracing/custom_carrier_test.go | 18 +++++- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index 88638bbfeb0b..b1528b4b4234 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -66,29 +66,34 @@ func Test(t *testing.T) { // format. func (s) TestInject(t *testing.T) { tests := []struct { - name string - sc oteltrace.SpanContext - fast bool + name string + sc oteltrace.SpanContext + fast bool + validSC bool }{ { - name: "fast path, valid context", - sc: validSpanContext, - fast: true, + name: "fast path, valid context", + sc: validSpanContext, + fast: true, + validSC: true, }, { - name: "fast path, invalid context", - sc: oteltrace.SpanContext{}, - fast: true, + name: "fast path, invalid context", + sc: oteltrace.SpanContext{}, + fast: true, + validSC: false, }, { - name: "slow path, valid context", - sc: validSpanContext, - fast: false, + name: "slow path, valid context", + sc: validSpanContext, + fast: false, + validSC: true, }, { - name: "slow path, invalid context", - sc: oteltrace.SpanContext{}, - fast: false, + name: "slow path, invalid context", + sc: oteltrace.SpanContext{}, + fast: false, + validSC: false, }, } @@ -108,15 +113,24 @@ func (s) TestInject(t *testing.T) { c = otelpropagation.MapCarrier{} } p.Inject(tCtx, c) - var got oteltrace.SpanContext + + var gotSC oteltrace.SpanContext + var gotValidSC bool if test.fast { - got, _ = fromBinary(stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())) + if gotSC, gotValidSC = fromBinary(stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())); test.validSC != gotValidSC { + t.Fatalf("got invalid span context in CustomCarrier's context from grpc-trace-bin header: %v, want valid span context", stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())) + } } else { - b, _ := base64.StdEncoding.DecodeString(c.Get(itracing.GRPCTraceBinHeaderKey)) - got, _ = fromBinary(b) + b, err := base64.StdEncoding.DecodeString(c.Get(itracing.GRPCTraceBinHeaderKey)) + if err != nil { + t.Fatalf("failed to decode MapCarrier's grpc-trace-bin base64 string header %s to binary: %v", c.Get(itracing.GRPCTraceBinHeaderKey), err) + } + if gotSC, gotValidSC = fromBinary(b); test.validSC != gotValidSC { + t.Fatalf("got invalid span context in MapCarrier's context from grpc-trace-bin header: %v, want valid span context", b) + } } - if test.sc.TraceID() != got.TraceID() && test.sc.SpanID() != got.SpanID() && test.sc.TraceFlags() != got.TraceFlags() { - t.Fatalf("got = %v, want %v", got, test.sc) + if test.sc.TraceID() != gotSC.TraceID() && test.sc.SpanID() != gotSC.SpanID() && test.sc.TraceFlags() != gotSC.TraceFlags() { + t.Fatalf("got span context = %v, want span contexts %v", gotSC, test.sc) } }) } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index c5faa2c8bb19..b2a0769f9234 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -83,6 +83,18 @@ func (s) TestGet(t *testing.T) { key: "key1", want: "", }, + { + name: "more than one key/value pair", + md: metadata.MD{"key1": []string{"value1"}, "key2": []string{"value2"}}, + key: "key2", + want: "value2", + }, + { + name: "more than one value for a key", + md: metadata.MD{"key1": []string{"value1", "value2"}}, + key: "key1", + want: "value1", + }, { name: "grpc-trace-bin key", md: metadata.MD{}, @@ -110,7 +122,7 @@ func (s) TestGet(t *testing.T) { if strings.HasSuffix(test.key, "-bin") { // for binary headers set `grpc-trace-bin` using `stats.SetIncomingTrace()` b, err := base64.StdEncoding.DecodeString(test.want) if err != nil { - t.Fatalf("failed to decode want %s as base64 string: %v", test.want, err) + t.Fatalf("failed to decode want %s base64 string to binary: %v", test.want, err) } ctx = stats.SetIncomingTrace(ctx, b) } @@ -200,13 +212,13 @@ func (s) TestSet(t *testing.T) { if strings.HasSuffix(test.setKey, "-bin") { wantB, err := base64.StdEncoding.DecodeString(test.wantValue) if err != nil { - t.Fatalf("failed to decode want %s as base64 string: %v", test.wantValue, err) + t.Fatalf("failed to decode want %s base64 string to binary: %v", test.wantValue, err) } verifyOutgoingTraceGRPCTraceBinHeader(c.ctx, t, wantB) return } // for non -bin headers verify string value in carrier's context metadata - if gotMD, _ := metadata.FromOutgoingContext(c.Context()); test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { + if gotMD, ok := metadata.FromOutgoingContext(c.Context()); ok && test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(test.setKey)[0], test.setValue, test.setKey) } }) From 309386adf790b3e9ed7d0e30391e81068a7c39f3 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 6 Nov 2024 15:30:59 +0530 Subject: [PATCH 16/20] first pass from doug --- .../internal/grpc_trace_bin_propagator.go | 6 +++--- .../internal/grpc_trace_bin_propagator_test.go | 10 +++++----- stats/opentelemetry/internal/tracing/custom_carrier.go | 8 ++------ .../internal/tracing/custom_carrier_test.go | 7 ++++--- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go index 53b2114daa98..21e23271ca10 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator.go @@ -43,7 +43,7 @@ type GRPCTraceBinPropagator struct{} // the trace data is base64 encoded and injected using the same header in // text format (slow path). If span context is not valid or emptu, no data is // injected. -func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { +func (GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { span := oteltrace.SpanFromContext(ctx) if !span.SpanContext().IsValid() { return @@ -73,7 +73,7 @@ func (p GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagat // extracted span context is marked as "remote", indicating that the trace // originated from a different process or service. If trace context is invalid // or not present, input `ctx` is returned as is. -func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { +func (GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { var bd []byte if cc, ok := carrier.(*itracing.CustomCarrier); ok { bd = cc.GetBinary() @@ -96,7 +96,7 @@ func (p GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropaga // GRPCTraceBinPropagator always returns a slice containing only // `grpc-trace-bin` key because it only sets the `grpc-trace-bin` header for // propagating trace context. -func (p GRPCTraceBinPropagator) Fields() []string { +func (GRPCTraceBinPropagator) Fields() []string { return []string{itracing.GRPCTraceBinHeaderKey} } diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go index b1528b4b4234..b6a225456df2 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go @@ -21,9 +21,9 @@ package internal import ( "context" "encoding/base64" - "reflect" "testing" + "github.com/google/go-cmp/cmp" otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/internal/grpctest" @@ -68,8 +68,8 @@ func (s) TestInject(t *testing.T) { tests := []struct { name string sc oteltrace.SpanContext - fast bool - validSC bool + fast bool // to indicate whether to follow fast path or slow path for injection verification + validSC bool // to indicate whether to expect a valid span context or not }{ { name: "fast path, valid context", @@ -152,7 +152,7 @@ func (s) TestExtract(t *testing.T) { tests := []struct { name string sc oteltrace.SpanContext - fast bool + fast bool // to indicate whether to follow fast path or slow path for extraction verification }{ { name: "fast path, valid context", @@ -221,7 +221,7 @@ func (s) TestBinary(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if got := binary(test.sc); !reflect.DeepEqual(got, test.want) { + if got := binary(test.sc); !cmp.Equal(got, test.want) { t.Fatalf("binary() = %v, want %v", got, test.want) } }) diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index b45382de4a23..fa20eba93582 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -110,13 +110,9 @@ func (c *CustomCarrier) Set(key, value string) { // GetBinary returns the binary value from the gRPC context in the incoming // RPC, associated with the header `grpc-trace-bin`. If header is not found or -// is empty, it returns nil. +// is empty, it returns empty slice. func (c *CustomCarrier) GetBinary() []byte { - values := stats.Trace(c.ctx) - if len(values) == 0 { - return nil - } - return values + return stats.Trace(c.ctx) } // SetBinary sets the binary value in the carrier's context, which will be sent diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index b2a0769f9234..4df4bcdd3e14 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -21,7 +21,6 @@ package tracing import ( "context" "encoding/base64" - "reflect" "strings" "testing" @@ -41,11 +40,13 @@ func Test(t *testing.T) { } func verifyOutgoingTraceGRPCTraceBinHeader(ctx context.Context, t *testing.T, wantB []byte) { + t.Helper() + gotB := stats.OutgoingTrace(ctx) if len(wantB) == 0 && gotB != nil { t.Fatalf("stats.OutgoingTrace() is non-nil, want nil") } - if gotB != nil && !reflect.DeepEqual(gotB, wantB) { + if gotB != nil && !cmp.Equal(gotB, wantB) { t.Fatalf("stats.OutgoingTrace() = %v, want %v", gotB, wantB) } } @@ -234,7 +235,7 @@ func (s) TestGetBinary(t *testing.T) { if got == nil { t.Fatalf("c.GetBinary() = nil, want %v", got) } - if !reflect.DeepEqual(want, got) { + if !cmp.Equal(want, got) { t.Fatalf("c.GetBinary() = %s, want %s", got, want) } } From 23c9a5862330ae3565baff6f39a1135e40e3a627 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 6 Nov 2024 23:12:48 +0530 Subject: [PATCH 17/20] Move grpcTraceBinPropagator under opentelemtry package --- go.mod | 1 - .../{internal => }/grpc_trace_bin_propagator.go | 4 +--- .../grpc_trace_bin_propagator_test.go | 13 +------------ 3 files changed, 2 insertions(+), 16 deletions(-) rename stats/opentelemetry/{internal => }/grpc_trace_bin_propagator.go (98%) rename stats/opentelemetry/{internal => }/grpc_trace_bin_propagator_test.go (97%) diff --git a/go.mod b/go.mod index 91008eeb7a42..58a1cbb46bbd 100644 --- a/go.mod +++ b/go.mod @@ -32,7 +32,6 @@ require ( github.com/go-logr/logr v1.4.2 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect - go.opentelemetry.io/otel/trace v1.31.0 // indirect golang.org/x/text v0.19.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20241015192408-796eee8c2d53 // indirect ) diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go b/stats/opentelemetry/grpc_trace_bin_propagator.go similarity index 98% rename from stats/opentelemetry/internal/grpc_trace_bin_propagator.go rename to stats/opentelemetry/grpc_trace_bin_propagator.go index 21e23271ca10..d1c0a46d1f7a 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator.go @@ -16,7 +16,7 @@ * */ -package internal +package opentelemetry import ( "context" @@ -27,8 +27,6 @@ import ( itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) -// TODO: Move out of internal as part of open telemetry API - // GRPCTraceBinPropagator is an OpenTelemetry TextMapPropagator which is used // to extract and inject trace context data from and into headers exchanged by // gRPC applications. It propagates trace data in binary format using the diff --git a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/grpc_trace_bin_propagator_test.go similarity index 97% rename from stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go rename to stats/opentelemetry/grpc_trace_bin_propagator_test.go index b6a225456df2..a2cf657de56f 100644 --- a/stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator_test.go @@ -16,7 +16,7 @@ * */ -package internal +package opentelemetry import ( "context" @@ -26,28 +26,17 @@ import ( "github.com/google/go-cmp/cmp" otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" - "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) -// TODO: Move out of internal as part of open telemetry API - // validSpanContext is a valid OpenTelemetry span context. var validSpanContext = oteltrace.SpanContext{}.WithTraceID( oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( oteltrace.TraceFlags(1)) -type s struct { - grpctest.Tester -} - -func Test(t *testing.T) { - grpctest.RunSubTests(t, s{}) -} - // TestInject verifies that the GRPCTraceBinPropagator correctly injects // OpenTelemetry span context as `grpc-trace-bin` header in the provided // carrier's context, for both fast and slow path, if span context is valid. If From 3c8389b70057faac68352131863cae758a5f3158 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 6 Nov 2024 23:20:04 +0530 Subject: [PATCH 18/20] update mod.go for otel/trace --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index 58a1cbb46bbd..da3125422c93 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( go.opentelemetry.io/otel/metric v1.31.0 go.opentelemetry.io/otel/sdk v1.31.0 go.opentelemetry.io/otel/sdk/metric v1.31.0 + go.opentelemetry.io/otel/trace v1.31.0 golang.org/x/net v0.30.0 golang.org/x/oauth2 v0.23.0 golang.org/x/sync v0.8.0 From 062e769be8aba00c8c9988bc0cf1b4dc1f1bf487 Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Tue, 12 Nov 2024 14:33:10 +0530 Subject: [PATCH 19/20] Changed context propagation to deal binary directly using metadata and remove base64 usages --- .../grpc_trace_bin_propagator.go | 78 +++---- .../grpc_trace_bin_propagator_test.go | 198 ++++++++---------- .../internal/tracing/custom_carrier.go | 113 ++-------- .../internal/tracing/custom_carrier_test.go | 108 ++-------- 4 files changed, 174 insertions(+), 323 deletions(-) diff --git a/stats/opentelemetry/grpc_trace_bin_propagator.go b/stats/opentelemetry/grpc_trace_bin_propagator.go index d1c0a46d1f7a..5e92114970e7 100644 --- a/stats/opentelemetry/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator.go @@ -20,13 +20,16 @@ package opentelemetry import ( "context" - "encoding/base64" otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" - itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" + "google.golang.org/grpc/stats" ) +// GRPCTraceBinHeaderKey is the gRPC metadata header key `grpc-trace-bin` used +// to propagate trace context in binary format. +const GRPCTraceBinHeaderKey = "grpc-trace-bin" + // GRPCTraceBinPropagator is an OpenTelemetry TextMapPropagator which is used // to extract and inject trace context data from and into headers exchanged by // gRPC applications. It propagates trace data in binary format using the @@ -36,53 +39,58 @@ type GRPCTraceBinPropagator struct{} // Inject sets OpenTelemetry trace context information from the Context into // the carrier. // -// If the carrier is a CustomCarrier, trace data is directly injected in a -// binary format using the `grpc-trace-bin` header (fast path). Otherwise, -// the trace data is base64 encoded and injected using the same header in -// text format (slow path). If span context is not valid or emptu, no data is -// injected. +// It first attempts to retrieve any existing binary trace data from the +// provided context using `stats.Trace()`. If found, it means that a trace data +// was injected by a system using gRPC OpenCensus plugin. Hence, we inject this +// trace data into the carrier, allowing trace from system using gRPC +// OpenCensus plugin to propagate downstream. However, we set the value in +// string format against `grpc-trace-bin` key so that downstream systems which +// are using gRPC OpenTelemetry plugin are able to extract it using +// `GRPCTraceBinPropagator`. +// +// It then attempts to retrieve an OpenTelemetry span context from the provided +// context. If not found, that means either there is no trace context or previous +// system had injected it using gRPC OpenCensus plugin. Therefore, it returns +// early without doing anymore modification to carrier. If found, that means +// previous system had injected using OpenTelemetry plugin so it converts the +// span context to binary and set in carrier in string format against +// `grpc-trace-bin` key. func (GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { - span := oteltrace.SpanFromContext(ctx) - if !span.SpanContext().IsValid() { - return + bd := stats.Trace(ctx) + if bd != nil { + carrier.Set(GRPCTraceBinHeaderKey, string(bd)) } - bd := binary(span.SpanContext()) - if bd == nil { + sc := oteltrace.SpanFromContext(ctx) + if !sc.SpanContext().IsValid() { return } - if cc, ok := carrier.(*itracing.CustomCarrier); ok { - cc.SetBinary(bd) - return - } - carrier.Set(itracing.GRPCTraceBinHeaderKey, base64.StdEncoding.EncodeToString(bd)) + bd = binary(sc.SpanContext()) + carrier.Set(GRPCTraceBinHeaderKey, string(bd)) } // Extract reads OpenTelemetry trace context information from the carrier into a // Context. // -// If the carrier is a CustomCarrier, trace data is read directly in a binary -// format from the `grpc-trace-bin` header (fast path). Otherwise, the trace -// data is base64 decoded from the same header in text format (slow path). +// It first attempts to read `grpc-trace-bin` header value from carrier. If +// found, that means the trace data was injected using gRPC OpenTelemetry +// plugin using `GRPCTraceBinPropagator`. It then set the trace data into +// context using `stats.SetTrace` for downstream systems still using gRPC +// OpenCensus plugin to be able to use this context. // -// If a valid trace context is found, this function returns a new context -// derived from the input `ctx` containing the extracted span context. The -// extracted span context is marked as "remote", indicating that the trace -// originated from a different process or service. If trace context is invalid -// or not present, input `ctx` is returned as is. +// It then also extracts the OpenTelemetry span context from binary header +// value. If span context is not valid, it just returns the context as parent. +// If span context is valid, it creates a new context containing the extracted +// OpenTelemetry span context marked as remote so that downstream systems using +// OpenTelemetry are able use this context. func (GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { - var bd []byte - if cc, ok := carrier.(*itracing.CustomCarrier); ok { - bd = cc.GetBinary() - } else { - bd, _ = base64.StdEncoding.DecodeString(carrier.Get(itracing.GRPCTraceBinHeaderKey)) - } - if bd == nil { - return ctx + h := carrier.Get(GRPCTraceBinHeaderKey) + if h != "" { + ctx = stats.SetTrace(ctx, []byte(h)) } - sc, ok := fromBinary([]byte(bd)) + sc, ok := fromBinary([]byte(h)) if !ok { return ctx } @@ -95,7 +103,7 @@ func (GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagati // `grpc-trace-bin` key because it only sets the `grpc-trace-bin` header for // propagating trace context. func (GRPCTraceBinPropagator) Fields() []string { - return []string{itracing.GRPCTraceBinHeaderKey} + return []string{GRPCTraceBinHeaderKey} } // Binary returns the binary format representation of a SpanContext. diff --git a/stats/opentelemetry/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/grpc_trace_bin_propagator_test.go index a2cf657de56f..27eb71095df8 100644 --- a/stats/opentelemetry/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator_test.go @@ -20,168 +20,156 @@ package opentelemetry import ( "context" - "encoding/base64" "testing" "github.com/google/go-cmp/cmp" - otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) -// validSpanContext is a valid OpenTelemetry span context. -var validSpanContext = oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)) +// Valid OpenTelemetry span contexts for testing. +var ( + validSpanContext1 = oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)) + validSpanContext2 = oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}).WithSpanID( + oteltrace.SpanID{33, 34, 35, 36, 37, 38, 39, 40}).WithTraceFlags( + oteltrace.TraceFlags(1)) +) // TestInject verifies that the GRPCTraceBinPropagator correctly injects -// OpenTelemetry span context as `grpc-trace-bin` header in the provided -// carrier's context, for both fast and slow path, if span context is valid. If -// span context is invalid, carrier's context is not modified which is verified -// by retrieving zero value span context from carrier's context. +// existing binary trace data or OpenTelemetry span context as `grpc-trace-bin` +// header in the provided carrier's metadata. +// +// For existing binary traced data, it maintains a test field `scExists` which +// if contains a valid span context, it set it as binary trace data using +// `stats.SetIncomingTrace` to mimick scenario of previous system setting trace +// context using gRPC OpenCensus plugin. It then verifies that if valid +// OpenTelemetry span context is not present to inject, the carrier's metadata +// should have binary equivalent of `scExists`. // -// For fast path, it passes `CustomCarrier` as carrier to `Inject()` which -// injects the span context using `CustomCarrier.SetBinary()`. It then -// retrieves the injected span context using `stats.OutgoingTrace()` for -// verification because `SetBinary()` does injection directly in binary format. +// For OpenTelemetry span context, it maintains a test field `scToInject` which +// if contains a valid span context, it creates a context using that span +// context. It then verifies that irrespective of whether existing trace data +// is present or not, if `scToInject` is valid span context, carrier's metadata +// should have binary equivalent of `scToInject`. // -// For slow path, it passes `otel.MapCarrier` as carrier to `Inject()` which -// injects the span context after base64 encoding as string using -// `carrier.Set()`. It then retrieves the injected span context using -// `carrier.Get()` for verification because `Set()` does injection in string -// format. +// If both `scToInject` and `scExists` are invalid span contexts, it verifies +// that `grpc-trace-bin` header is not set in the carrier's metadata. func (s) TestInject(t *testing.T) { tests := []struct { - name string - sc oteltrace.SpanContext - fast bool // to indicate whether to follow fast path or slow path for injection verification - validSC bool // to indicate whether to expect a valid span context or not + name string + scToInject oteltrace.SpanContext // span context to inject from the context to carrier + scExists oteltrace.SpanContext // existing trace data in the context to set using `stats.SetIncomingTrace` to mimick scenario of previous system setting trace context using gRPC OpenCensus plugin. + wantSC oteltrace.SpanContext // expected span context from carrier after injection }{ { - name: "fast path, valid context", - sc: validSpanContext, - fast: true, - validSC: true, + name: "inject valid span context, no existing trace data", + scToInject: validSpanContext1, + scExists: oteltrace.SpanContext{}, + wantSC: validSpanContext1, }, { - name: "fast path, invalid context", - sc: oteltrace.SpanContext{}, - fast: true, - validSC: false, + name: "invalid span context to inject, existing trace data present", + scToInject: oteltrace.SpanContext{}, + scExists: validSpanContext2, + wantSC: validSpanContext2, }, { - name: "slow path, valid context", - sc: validSpanContext, - fast: false, - validSC: true, + name: "valid span context to inject, existing trace data present", + scToInject: validSpanContext1, + scExists: validSpanContext2, + wantSC: validSpanContext1, // if new valid OpenTelemetry span context is present, it overrides existing trace data }, { - name: "slow path, invalid context", - sc: oteltrace.SpanContext{}, - fast: false, - validSC: false, + name: "invalid span context to inject, no trace data present", + scToInject: oteltrace.SpanContext{}, + scExists: oteltrace.SpanContext{}, + wantSC: oteltrace.SpanContext{}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { p := GRPCTraceBinPropagator{} - tCtx, tCancel := context.WithCancel(context.Background()) - tCtx = oteltrace.ContextWithSpanContext(tCtx, test.sc) - defer tCancel() ctx, cancel := context.WithCancel(context.Background()) defer cancel() - - var c otelpropagation.TextMapCarrier - if test.fast { - c = itracing.NewCustomCarrier(metadata.NewOutgoingContext(ctx, metadata.MD{})) - } else { - c = otelpropagation.MapCarrier{} + if test.scExists.IsValid() { + ctx = stats.SetIncomingTrace(ctx, binary(test.scExists)) + } + if test.scToInject.IsValid() { + ctx = oteltrace.ContextWithSpanContext(ctx, test.scToInject) } - p.Inject(tCtx, c) - var gotSC oteltrace.SpanContext - var gotValidSC bool - if test.fast { - if gotSC, gotValidSC = fromBinary(stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())); test.validSC != gotValidSC { - t.Fatalf("got invalid span context in CustomCarrier's context from grpc-trace-bin header: %v, want valid span context", stats.OutgoingTrace(c.(*itracing.CustomCarrier).Context())) - } - } else { - b, err := base64.StdEncoding.DecodeString(c.Get(itracing.GRPCTraceBinHeaderKey)) - if err != nil { - t.Fatalf("failed to decode MapCarrier's grpc-trace-bin base64 string header %s to binary: %v", c.Get(itracing.GRPCTraceBinHeaderKey), err) - } - if gotSC, gotValidSC = fromBinary(b); test.validSC != gotValidSC { - t.Fatalf("got invalid span context in MapCarrier's context from grpc-trace-bin header: %v, want valid span context", b) + c := itracing.NewCustomCarrier(&metadata.MD{}) + p.Inject(ctx, c) + gotH := c.Get(GRPCTraceBinHeaderKey) + if !test.wantSC.IsValid() { + if gotH != "" { + t.Fatalf("got non-empty value from CustomCarrier's metadata grpc-trace-bin header, want empty") } + return + } + if gotH == "" { + t.Fatalf("got empty value from CustomCarrier's metadata grpc-trace-bin header, want valid span context: %v", test.wantSC) } - if test.sc.TraceID() != gotSC.TraceID() && test.sc.SpanID() != gotSC.SpanID() && test.sc.TraceFlags() != gotSC.TraceFlags() { - t.Fatalf("got span context = %v, want span contexts %v", gotSC, test.sc) + gotSC, ok := fromBinary([]byte(gotH)) + if !ok { + t.Fatalf("got invalid span context from CustomCarrier's metadata grpc-trace-bin header, want valid span context: %v", test.wantSC) + } + if test.wantSC.TraceID() != gotSC.TraceID() && test.wantSC.SpanID() != gotSC.SpanID() && test.wantSC.TraceFlags() != gotSC.TraceFlags() { + t.Fatalf("got span context = %v, want span contexts %v", gotSC, test.wantSC) } }) } } // TestExtract verifies that the GRPCTraceBinPropagator correctly extracts -// OpenTelemetry span context data for fast and slow path, if valid span -// context was injected. If invalid context was injected, it verifies that a -// zero value span context was retrieved. +// OpenTelemetry span context data from the provided context using carrier. // -// For fast path, it uses the CustomCarrier and sets the span context in the -// binary format using `stats.SetIncomingTrace` in its context and then -// verifies that same span context is extracted directly in binary foramt. +// If a valid span context was injected, it verifies same trace span context +// is extracted from carrier's metadata for `grpc-trace-bin` header key. It +// also verifies that the binary value of `grpc-trace-bin` header is set +// correcttly using `stats.SetTrace` by verifying the outgoing trace to make +// sure trace context propagation is backward compatible. // -// For slow path, it uses a MapCarrier and sets base64 encoded span context in -// its context and then verifies that same span context is extracted after -// base64 decoding. +// If invalid span context was injected, it verifies that valid trace span +// context is not extracted. func (s) TestExtract(t *testing.T) { tests := []struct { - name string - sc oteltrace.SpanContext - fast bool // to indicate whether to follow fast path or slow path for extraction verification + name string + wantSC oteltrace.SpanContext // expected span context from carrier }{ { - name: "fast path, valid context", - sc: validSpanContext.WithRemote(true), - fast: true, - }, - { - name: "fast path, invalid context", - sc: oteltrace.SpanContext{}, - fast: true, - }, - { - name: "slow path, valid context", - sc: validSpanContext.WithRemote(true), - fast: false, + name: "valid OpenTelemetry span context", + wantSC: validSpanContext1.WithRemote(true), }, { - name: "slow path, invalid context", - sc: oteltrace.SpanContext{}, - fast: false, + name: "invalid OpenTelemetry span context", + wantSC: oteltrace.SpanContext{}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { p := GRPCTraceBinPropagator{} - bd := binary(test.sc) + bd := binary(test.wantSC) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - var c otelpropagation.TextMapCarrier - if test.fast { - c = itracing.NewCustomCarrier(stats.SetIncomingTrace(ctx, bd)) - } else { - c = otelpropagation.MapCarrier{itracing.GRPCTraceBinHeaderKey: base64.StdEncoding.EncodeToString(bd)} - } + c := itracing.NewCustomCarrier(&metadata.MD{GRPCTraceBinHeaderKey: []string{string(bd)}}) + tCtx := p.Extract(ctx, c) got := oteltrace.SpanContextFromContext(tCtx) - if !got.Equal(test.sc) { - t.Fatalf("got = %v, want %v", got, test.sc) + if !got.Equal(test.wantSC) { + t.Fatalf("got span context: %v, want span context: %v", got, test.wantSC) + } + if bd != nil && cmp.Equal(bd, stats.OutgoingTrace(ctx)) { + t.Fatalf("stats.OutgoingTrace(ctx) = %v, want %v", stats.OutgoingTrace(ctx), bd) } }) } @@ -198,8 +186,8 @@ func (s) TestBinary(t *testing.T) { }{ { name: "valid context", - sc: validSpanContext, - want: binary(validSpanContext), + sc: validSpanContext1, + want: binary(validSpanContext1), }, { name: "zero value context", @@ -232,7 +220,7 @@ func (s) TestFromBinary(t *testing.T) { { name: "valid", b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, - want: validSpanContext.WithRemote(true), + want: validSpanContext1.WithRemote(true), ok: true, }, { diff --git a/stats/opentelemetry/internal/tracing/custom_carrier.go b/stats/opentelemetry/internal/tracing/custom_carrier.go index fa20eba93582..96c6efb1f124 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier.go @@ -21,120 +21,51 @@ package tracing import ( - "context" - "encoding/base64" - "strings" - - "google.golang.org/grpc/grpclog" "google.golang.org/grpc/metadata" - "google.golang.org/grpc/stats" ) -// GRPCTraceBinHeaderKey is the gRPC metadata header key `grpc-trace-bin` used -// to propagate trace context in binary format. -const GRPCTraceBinHeaderKey = "grpc-trace-bin" - -var logger = grpclog.Component("otel-plugin") - -// CustomCarrier is a TextMapCarrier that uses gRPC context to store and -// retrieve any propagated key-value pairs in text format along with binary -// format for `grpc-trace-bin` header. +// CustomCarrier is a TextMapCarrier that uses `*metadata.MD` to store and +// retrieve any propagated key-value pairs in text format. type CustomCarrier struct { - ctx context.Context + md *metadata.MD } -// NewCustomCarrier creates a new CustomMapCarrier with -// the given context. -func NewCustomCarrier(ctx context.Context) *CustomCarrier { - return &CustomCarrier{ctx: ctx} +// NewCustomCarrier creates a new CustomCarrier with +// the given metadata. +func NewCustomCarrier(md *metadata.MD) *CustomCarrier { + return &CustomCarrier{md: md} } -// Get returns the string value associated with the passed key from the gRPC -// context. It returns an empty string in any of the following cases: -// - the key is not present in the context -// - the value associated with the key is empty -// - the key ends with "-bin" and is not `grpc-trace-bin` +// Get returns the string value associated with the passed key from the +// carrier's metadata. // -// If the key is `grpc-trace-bin`, it retrieves the binary value using -// `stats.Trace()` and then base64 encodes it before returning. For all other -// string keys, it retrieves the value from the context's metadata. +// It returns an empty string if the key is not present in the carrier's +// metadata or if the value associated with the key is empty. func (c *CustomCarrier) Get(key string) string { - if key == GRPCTraceBinHeaderKey { - return base64.StdEncoding.EncodeToString(stats.Trace(c.ctx)) - } - if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { - logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) - return "" - } - - md, ok := metadata.FromIncomingContext(c.ctx) - if !ok { - return "" - } - values := md.Get(key) + values := c.md.Get(key) if len(values) == 0 { return "" } return values[0] } -// Set stores the key-value pair in the gRPC context. If the key already -// exists, its value will be overwritten. If the key ends with "-bin" and is -// not `grpc-trace-bin`, the key-value pair is not stored. +// Set stores the key-value pair in the carrier's metadata. // -// If the key is `grpc-trace-bin`, it base64 decodes the `value` and stores the -// resulting binary data using `stats.SetTrace()`. For all other keys, it stores -// the key-value pair in the string format in context's metadata. +// If the key already exists, its value is overwritten. func (c *CustomCarrier) Set(key, value string) { - if key == GRPCTraceBinHeaderKey { - b, err := base64.StdEncoding.DecodeString(value) - if err != nil { - logger.Errorf("encountered error in decoding %s value", GRPCTraceBinHeaderKey) - return - } - c.ctx = stats.SetTrace(c.ctx, b) - return - } - if strings.HasSuffix(key, "-bin") && key != GRPCTraceBinHeaderKey { - logger.Warningf("encountered a binary header %s which is not: %s", key, GRPCTraceBinHeaderKey) - return - } - - md, ok := metadata.FromOutgoingContext(c.ctx) - if !ok { - md = metadata.MD{} - } - md.Set(key, value) - c.ctx = metadata.NewOutgoingContext(c.ctx, md) + c.md.Set(key, value) } -// GetBinary returns the binary value from the gRPC context in the incoming -// RPC, associated with the header `grpc-trace-bin`. If header is not found or -// is empty, it returns empty slice. -func (c *CustomCarrier) GetBinary() []byte { - return stats.Trace(c.ctx) -} - -// SetBinary sets the binary value in the carrier's context, which will be sent -// in the outgoing RPC with the header `grpc-trace-bin`. -func (c *CustomCarrier) SetBinary(value []byte) { - c.ctx = stats.SetTrace(c.ctx, value) -} - -// Keys returns the keys stored in the carrier's context. +// Keys returns the keys stored in the carrier's metadata. func (c *CustomCarrier) Keys() []string { - md, _ := metadata.FromOutgoingContext(c.ctx) - keys := make([]string, 0, len(md)+1) // +1 for grpc-trace-bin (if present) - for k := range md { - keys = append(keys, k) - } - if stats.OutgoingTrace(c.ctx) != nil { - keys = append(keys, GRPCTraceBinHeaderKey) // add grpc-trace-bin key if grpc-trace-bin is present in the context + keys := make([]string, 0, len(*c.md)) + for key := range *c.md { + keys = append(keys, key) } return keys } -// Context returns the underlying context associated with the CustomCarrier. -func (c *CustomCarrier) Context() context.Context { - return c.ctx +// Metadata returns the underlying metadata associated with the CustomCarrier. +func (c *CustomCarrier) Metadata() *metadata.MD { + return c.md } diff --git a/stats/opentelemetry/internal/tracing/custom_carrier_test.go b/stats/opentelemetry/internal/tracing/custom_carrier_test.go index 4df4bcdd3e14..01ac78736d8f 100644 --- a/stats/opentelemetry/internal/tracing/custom_carrier_test.go +++ b/stats/opentelemetry/internal/tracing/custom_carrier_test.go @@ -19,16 +19,12 @@ package tracing import ( - "context" - "encoding/base64" - "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" - "google.golang.org/grpc/stats" ) type s struct { @@ -39,26 +35,12 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -func verifyOutgoingTraceGRPCTraceBinHeader(ctx context.Context, t *testing.T, wantB []byte) { - t.Helper() - - gotB := stats.OutgoingTrace(ctx) - if len(wantB) == 0 && gotB != nil { - t.Fatalf("stats.OutgoingTrace() is non-nil, want nil") - } - if gotB != nil && !cmp.Equal(gotB, wantB) { - t.Fatalf("stats.OutgoingTrace() = %v, want %v", gotB, wantB) - } -} - // TestGet verifies that `CustomCarrier.Get()` returns correct value for the -// corresponding key in the carrier's context, if key is present. If key is not +// corresponding key in the carrier's metadata, if key is present. If key is not // present, it verifies that empty string is returned. // -// If key ends with `-bin`, it verifies that a correct base64 encoded -// string value is returned for the `grpc-trace-bin` header's binary value, if -// present. For any other `-bin` key, it verifies that an empty string is -// returned. +// If key ends with `-bin`, it verifies that a correct binary value is returned +// in the string format for the binary header. func (s) TestGet(t *testing.T) { tests := []struct { name string @@ -98,36 +80,21 @@ func (s) TestGet(t *testing.T) { }, { name: "grpc-trace-bin key", - md: metadata.MD{}, + md: metadata.Pairs("grpc-trace-bin", string([]byte{0x01, 0x02, 0x03})), key: "grpc-trace-bin", - want: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + want: string([]byte{0x01, 0x02, 0x03}), }, { name: "grpc-trace-bin key with another string key", - md: metadata.Pairs("key1", "value1"), + md: metadata.MD{"key1": []string{"value1"}, "grpc-trace-bin": []string{string([]byte{0x01, 0x02, 0x03})}}, key: "grpc-trace-bin", - want: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), - }, - { - name: "non-grpc-trace-bin key", - md: metadata.Pairs("non-trace-bin", base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03})), - key: "non-trace-bin", - want: "", // -bin key that isn't grpc-trace-bin should always get empty string + want: string([]byte{0x01, 0x02, 0x03}), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - if strings.HasSuffix(test.key, "-bin") { // for binary headers set `grpc-trace-bin` using `stats.SetIncomingTrace()` - b, err := base64.StdEncoding.DecodeString(test.want) - if err != nil { - t.Fatalf("failed to decode want %s base64 string to binary: %v", test.want, err) - } - ctx = stats.SetIncomingTrace(ctx, b) - } - c := NewCustomCarrier(metadata.NewIncomingContext(ctx, test.md)) + c := NewCustomCarrier(&test.md) got := c.Get(test.key) if got != test.want { t.Fatalf("Get() = %s, want %s", got, test.want) @@ -136,14 +103,13 @@ func (s) TestGet(t *testing.T) { } } -// TestSet verifies that a key-value pair is set in carrier's context using +// TestSet verifies that a key-value pair is set in carrier's metadata using // `CustomCarrier.Set()`. If key is not present, it verifies that key-value // pair is insterted. If key is already present, it verifies that value is // overwritten. // // If key ends with `-bin`, it verifies that a binary value is set for -// `grpc-trace-bin` header, if key is `grpc-trace-bin` and value is base64 -// encoded. For any other `-bin` key, it verifies that no binary value is set. +// `-bin` header in string format. // // It also verifies that both existing and newly inserted keys are present in // the carrier's context using `CustomCarrier.Keys()`. @@ -184,67 +150,25 @@ func (s) TestSet(t *testing.T) { name: "grpc-trace-bin binary key", initialMD: metadata.MD{"key1": []string{"value1"}}, setKey: "grpc-trace-bin", - setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), - wantValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), + setValue: string([]byte{0x01, 0x02, 0x03}), + wantValue: string([]byte{0x01, 0x02, 0x03}), wantKeys: []string{"key1", "grpc-trace-bin"}, }, - { - name: "non-grpc-trace-bin binary key", - initialMD: metadata.MD{"key1": []string{"value1"}}, - setKey: "non-grpc-trace-bin", - setValue: base64.StdEncoding.EncodeToString([]byte{0x01, 0x02, 0x03}), - wantValue: "", // -bin key that isn't grpc-trace-bin should never be set - wantKeys: []string{"key1"}, - }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := NewCustomCarrier(metadata.NewOutgoingContext(ctx, test.initialMD)) + c := NewCustomCarrier(&test.initialMD) c.Set(test.setKey, test.setValue) gotKeys := c.Keys() equalKeys := cmp.Equal(test.wantKeys, gotKeys, cmpopts.SortSlices(func(a, b string) bool { return a < b })) if !equalKeys { t.Fatalf("c.Keys() = keys %v, want %v", gotKeys, test.wantKeys) } - // for binary headers verify `grpc-trace-bin` binary value for outgoing trace in carrier's context - if strings.HasSuffix(test.setKey, "-bin") { - wantB, err := base64.StdEncoding.DecodeString(test.wantValue) - if err != nil { - t.Fatalf("failed to decode want %s base64 string to binary: %v", test.wantValue, err) - } - verifyOutgoingTraceGRPCTraceBinHeader(c.ctx, t, wantB) - return - } - // for non -bin headers verify string value in carrier's context metadata - if gotMD, ok := metadata.FromOutgoingContext(c.Context()); ok && test.wantValue != "" && gotMD.Get(test.setKey)[0] != test.setValue { - t.Fatalf("got value %s, want %s, for key %s", gotMD.Get(test.setKey)[0], test.setValue, test.setKey) + got := c.Get(test.setKey) + if got != test.setValue { + t.Fatalf("got value %s, want %s, for key %s", got, test.setValue, test.setKey) } }) } } - -func (s) TestGetBinary(t *testing.T) { - want := []byte{0x01, 0x02, 0x03} - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - c := NewCustomCarrier(stats.SetIncomingTrace(ctx, want)) - got := c.GetBinary() - if got == nil { - t.Fatalf("c.GetBinary() = nil, want %v", got) - } - if !cmp.Equal(want, got) { - t.Fatalf("c.GetBinary() = %s, want %s", got, want) - } -} - -func (s) TestSetBinary(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - want := []byte{0x01, 0x02, 0x03} - c := NewCustomCarrier(ctx) - c.SetBinary(want) - verifyOutgoingTraceGRPCTraceBinHeader(c.Context(), t, want) -} From ee2869c04b7d120196dea0c7e93392aab947e45b Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Wed, 13 Nov 2024 01:12:19 +0530 Subject: [PATCH 20/20] remove stats.Trace usage --- .../grpc_trace_bin_propagator.go | 52 +++-------- .../grpc_trace_bin_propagator_test.go | 90 +++++-------------- 2 files changed, 36 insertions(+), 106 deletions(-) diff --git a/stats/opentelemetry/grpc_trace_bin_propagator.go b/stats/opentelemetry/grpc_trace_bin_propagator.go index 5e92114970e7..df687e235031 100644 --- a/stats/opentelemetry/grpc_trace_bin_propagator.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator.go @@ -23,7 +23,6 @@ import ( otelpropagation "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" - "google.golang.org/grpc/stats" ) // GRPCTraceBinHeaderKey is the gRPC metadata header key `grpc-trace-bin` used @@ -36,58 +35,33 @@ const GRPCTraceBinHeaderKey = "grpc-trace-bin" // `grpc-trace-bin` header. type GRPCTraceBinPropagator struct{} -// Inject sets OpenTelemetry trace context information from the Context into -// the carrier. +// Inject sets OpenTelemetry span context from the Context into the carrier as +// a `grpc-trace-bin` header if span context is valid. // -// It first attempts to retrieve any existing binary trace data from the -// provided context using `stats.Trace()`. If found, it means that a trace data -// was injected by a system using gRPC OpenCensus plugin. Hence, we inject this -// trace data into the carrier, allowing trace from system using gRPC -// OpenCensus plugin to propagate downstream. However, we set the value in -// string format against `grpc-trace-bin` key so that downstream systems which -// are using gRPC OpenTelemetry plugin are able to extract it using -// `GRPCTraceBinPropagator`. -// -// It then attempts to retrieve an OpenTelemetry span context from the provided -// context. If not found, that means either there is no trace context or previous -// system had injected it using gRPC OpenCensus plugin. Therefore, it returns -// early without doing anymore modification to carrier. If found, that means -// previous system had injected using OpenTelemetry plugin so it converts the -// span context to binary and set in carrier in string format against -// `grpc-trace-bin` key. +// If span context is not valid, it ruturns without setting `grpc-trace-bin` +// header. func (GRPCTraceBinPropagator) Inject(ctx context.Context, carrier otelpropagation.TextMapCarrier) { - bd := stats.Trace(ctx) - if bd != nil { - carrier.Set(GRPCTraceBinHeaderKey, string(bd)) - } - sc := oteltrace.SpanFromContext(ctx) if !sc.SpanContext().IsValid() { return } - bd = binary(sc.SpanContext()) + bd := binary(sc.SpanContext()) carrier.Set(GRPCTraceBinHeaderKey, string(bd)) } -// Extract reads OpenTelemetry trace context information from the carrier into a -// Context. +// Extract reads OpenTelemetry span context from the `grpc-trace-bin` header of +// carrier into the provided context, if present. // -// It first attempts to read `grpc-trace-bin` header value from carrier. If -// found, that means the trace data was injected using gRPC OpenTelemetry -// plugin using `GRPCTraceBinPropagator`. It then set the trace data into -// context using `stats.SetTrace` for downstream systems still using gRPC -// OpenCensus plugin to be able to use this context. +// If a valid span context is retrieved from `grpc-trace-bin`, it returns a new +// context containing the extracted OpenTelemetry span context marked as +// remote. // -// It then also extracts the OpenTelemetry span context from binary header -// value. If span context is not valid, it just returns the context as parent. -// If span context is valid, it creates a new context containing the extracted -// OpenTelemetry span context marked as remote so that downstream systems using -// OpenTelemetry are able use this context. +// If `grpc-trace-bin` header is not present, it returns the context as is. func (GRPCTraceBinPropagator) Extract(ctx context.Context, carrier otelpropagation.TextMapCarrier) context.Context { h := carrier.Get(GRPCTraceBinHeaderKey) - if h != "" { - ctx = stats.SetTrace(ctx, []byte(h)) + if h == "" { + return ctx } sc, ok := fromBinary([]byte(h)) diff --git a/stats/opentelemetry/grpc_trace_bin_propagator_test.go b/stats/opentelemetry/grpc_trace_bin_propagator_test.go index 27eb71095df8..90527d10e3e8 100644 --- a/stats/opentelemetry/grpc_trace_bin_propagator_test.go +++ b/stats/opentelemetry/grpc_trace_bin_propagator_test.go @@ -25,71 +25,38 @@ import ( "github.com/google/go-cmp/cmp" oteltrace "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/metadata" - "google.golang.org/grpc/stats" itracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing" ) -// Valid OpenTelemetry span contexts for testing. -var ( - validSpanContext1 = oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( - oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( - oteltrace.TraceFlags(1)) - validSpanContext2 = oteltrace.SpanContext{}.WithTraceID( - oteltrace.TraceID{17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}).WithSpanID( - oteltrace.SpanID{33, 34, 35, 36, 37, 38, 39, 40}).WithTraceFlags( - oteltrace.TraceFlags(1)) -) +var validSpanContext = oteltrace.SpanContext{}.WithTraceID( + oteltrace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}).WithSpanID( + oteltrace.SpanID{17, 18, 19, 20, 21, 22, 23, 24}).WithTraceFlags( + oteltrace.TraceFlags(1)) // TestInject verifies that the GRPCTraceBinPropagator correctly injects // existing binary trace data or OpenTelemetry span context as `grpc-trace-bin` // header in the provided carrier's metadata. // -// For existing binary traced data, it maintains a test field `scExists` which -// if contains a valid span context, it set it as binary trace data using -// `stats.SetIncomingTrace` to mimick scenario of previous system setting trace -// context using gRPC OpenCensus plugin. It then verifies that if valid -// OpenTelemetry span context is not present to inject, the carrier's metadata -// should have binary equivalent of `scExists`. -// -// For OpenTelemetry span context, it maintains a test field `scToInject` which -// if contains a valid span context, it creates a context using that span -// context. It then verifies that irrespective of whether existing trace data -// is present or not, if `scToInject` is valid span context, carrier's metadata -// should have binary equivalent of `scToInject`. +// It verifies that if a valid span context is injected, same span context can +// can be retreived from the carrier's metadata. // -// If both `scToInject` and `scExists` are invalid span contexts, it verifies -// that `grpc-trace-bin` header is not set in the carrier's metadata. +// If an invalid span context is injected, it verifies that `grpc-trace-bin` +// header is not set in the carrier's metadata. func (s) TestInject(t *testing.T) { tests := []struct { - name string - scToInject oteltrace.SpanContext // span context to inject from the context to carrier - scExists oteltrace.SpanContext // existing trace data in the context to set using `stats.SetIncomingTrace` to mimick scenario of previous system setting trace context using gRPC OpenCensus plugin. - wantSC oteltrace.SpanContext // expected span context from carrier after injection + name string + injectSC oteltrace.SpanContext + wantSC oteltrace.SpanContext }{ { - name: "inject valid span context, no existing trace data", - scToInject: validSpanContext1, - scExists: oteltrace.SpanContext{}, - wantSC: validSpanContext1, + name: "valid OpenTelemetry span context", + injectSC: validSpanContext, + wantSC: validSpanContext, }, { - name: "invalid span context to inject, existing trace data present", - scToInject: oteltrace.SpanContext{}, - scExists: validSpanContext2, - wantSC: validSpanContext2, - }, - { - name: "valid span context to inject, existing trace data present", - scToInject: validSpanContext1, - scExists: validSpanContext2, - wantSC: validSpanContext1, // if new valid OpenTelemetry span context is present, it overrides existing trace data - }, - { - name: "invalid span context to inject, no trace data present", - scToInject: oteltrace.SpanContext{}, - scExists: oteltrace.SpanContext{}, - wantSC: oteltrace.SpanContext{}, + name: "invalid OpenTelemetry span context", + injectSC: oteltrace.SpanContext{}, + wantSC: oteltrace.SpanContext{}, }, } @@ -98,12 +65,7 @@ func (s) TestInject(t *testing.T) { p := GRPCTraceBinPropagator{} ctx, cancel := context.WithCancel(context.Background()) defer cancel() - if test.scExists.IsValid() { - ctx = stats.SetIncomingTrace(ctx, binary(test.scExists)) - } - if test.scToInject.IsValid() { - ctx = oteltrace.ContextWithSpanContext(ctx, test.scToInject) - } + ctx = oteltrace.ContextWithSpanContext(ctx, test.injectSC) c := itracing.NewCustomCarrier(&metadata.MD{}) p.Inject(ctx, c) @@ -132,10 +94,7 @@ func (s) TestInject(t *testing.T) { // OpenTelemetry span context data from the provided context using carrier. // // If a valid span context was injected, it verifies same trace span context -// is extracted from carrier's metadata for `grpc-trace-bin` header key. It -// also verifies that the binary value of `grpc-trace-bin` header is set -// correcttly using `stats.SetTrace` by verifying the outgoing trace to make -// sure trace context propagation is backward compatible. +// is extracted from carrier's metadata for `grpc-trace-bin` header key. // // If invalid span context was injected, it verifies that valid trace span // context is not extracted. @@ -146,7 +105,7 @@ func (s) TestExtract(t *testing.T) { }{ { name: "valid OpenTelemetry span context", - wantSC: validSpanContext1.WithRemote(true), + wantSC: validSpanContext.WithRemote(true), }, { name: "invalid OpenTelemetry span context", @@ -168,9 +127,6 @@ func (s) TestExtract(t *testing.T) { if !got.Equal(test.wantSC) { t.Fatalf("got span context: %v, want span context: %v", got, test.wantSC) } - if bd != nil && cmp.Equal(bd, stats.OutgoingTrace(ctx)) { - t.Fatalf("stats.OutgoingTrace(ctx) = %v, want %v", stats.OutgoingTrace(ctx), bd) - } }) } } @@ -186,8 +142,8 @@ func (s) TestBinary(t *testing.T) { }{ { name: "valid context", - sc: validSpanContext1, - want: binary(validSpanContext1), + sc: validSpanContext, + want: binary(validSpanContext), }, { name: "zero value context", @@ -220,7 +176,7 @@ func (s) TestFromBinary(t *testing.T) { { name: "valid", b: []byte{0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 17, 18, 19, 20, 21, 22, 23, 24, 2, 1}, - want: validSpanContext1.WithRemote(true), + want: validSpanContext.WithRemote(true), ok: true, }, {