stats/internal: OpenTelemetry tracing GRPCTraceBinPropagator #7677

purnesh42H · 2024-09-27T15:59:18Z

This PR adds the GRPCTraceBinPropagator that implements TextMapPropagator as proposed in gRFC A72 which outlines a custom propagator to handle both binary and text formats for trace context propagation. This will allow gRPC to keep using grpc-trace-bin header for context propagation and also support any other text propagators. When using grpc-trace-bin the OpenCensus spanContext and OpenTelemetry spanContext are identical, therefore a gRPC OpenCensus client can speak with a gRPC OpenTelemetry server and vice versa. It is encouraged to use GRPCTraceBinPropagator for the migration. Using the same header greatly simplifies rollout.

Note: Currently, the GRPCTraceBinPropagator is placed within the internal package. We plan to move it out of internal and make it publicly accessible as part of a future OpenTelemetry API change.

RELEASE NOTES: None

codecov · 2024-09-27T16:02:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (b8ee37d) to head (80820c4).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7677      +/-   ##
==========================================
+ Coverage   81.84%   81.97%   +0.13%     
==========================================
  Files         361      362       +1     
  Lines       27827    28119     +292     
==========================================
+ Hits        22775    23051     +276     
- Misses       3850     3860      +10     
- Partials     1202     1208       +6

see 21 files with indirect coverage changes

dfawley · 2024-10-01T20:39:49Z

I'll let @zasweq do the primary review here and just take a 2nd pass.

zasweq

Some comments on implementation; didn't review tests yet. My next pass will be more on correctness, this pass was more general Go style/API and docstring semantics.

zasweq · 2024-10-02T17:58:55Z

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

+
+	"go.opentelemetry.io/otel/propagation"
+	"go.opentelemetry.io/otel/trace"
+	otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing"


This rename is quite long. Also please separate imports: https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/client_metrics.go#L31. Perhaps otelpropagation, oteltrace, itracing?

Changed as suggested except internaltracing instead of itracing

I prefer itracing, as it's more terse and it's the only way I've seen this. Is there places in our codebase where we rename internal/x imports to internalx? If so I'm fine with this.

yeah i saw convention is i* like istats. Changed to itracing

zasweq · 2024-10-02T18:00:40Z

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

+// 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.


Here and elsewhere: I don't know what these comments semantically refer to. What does "cross-cutting concerns" mean? I don't think this is a strict requirement "within a carrier that travels in-band across process boundaries."

both "cross-cutting concerns" and "within a carrier that travels in-band across process boundaries." are taken directly from OpenTelemetry propagation.TextMapCarrier interface https://pkg.go.dev/go.opentelemetry.io/otel/propagation#TextMapPropagator.

"cross-cutting concerns" simply refers to functionalities that are not central to the core logic of your application but are needed across different parts of it (eg. tracing, logging etc.) which are transported "in-band" (within the main data flow) across process boundaries (separate applications).

However, you are right to point out that we should keep the docstrings simplified for anyone to understand the purpose of the method. I have simplified the comments everwhere now. Let me know what you think.

zasweq · 2024-10-02T18:01:11Z

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

+// travels in-band across process boundaries.
+type GRPCTraceBinPropagator struct{}
+
+// Inject set cross-cutting concerns from the Context into the carrier.


Here as well, what is cross-cutting concerns?

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

stats/opentelemetry/internal/tracing/custom_carrier.go

zasweq · 2024-10-02T18:24:50Z

stats/opentelemetry/internal/tracing/custom_carrier.go

+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")


Is this the correct error message? I think both in the case it's not present in md and if it's set to an empty byte string it'll be not found. Is an empty byte string distinct from nil in the return? Is the empty byte string a valid thing to transmit for this header? Do we want this to return an error at all?

Removed returning error from here to be consistent with Get(..)

I'm fine with the error. What is the intended usage of this function? Do you intended to treat not set and set to an empty string equivalent? Is empty string a valid key for this? In that case you should make the error message fail with that. But that relates to the intended usage of this, and the failure modes (i.e. what happens in the error case).

Here we are only looking trace context which needs to conform to following struct. So the propagator will not allow to set any such value (like empty) as it won't be a valid SpanContext. Hence, the only reason for returning nil is client didn't set the grpc-trace-bin header.

// Fields: // // TraceId: (field_id = 0, len = 16, default = "0000000000000000") - // 16-byte array representing the trace_id. // // SpanId: (field_id = 1, len = 8, default = "00000000") - 8-byte array // representing the span_id. // // TraceOptions: (field_id = 2, len = 1, default = "0") - 1-byte array representing the trace_options.

Is empty string a valid key for this?

Binary methods will not deal with string keys because we are using existing stats package

grpc-go/stats/stats.go

Line 316 in d9d8f34

func SetTrace(ctx context.Context, b []byte) context.Context {

to set bytes which sets a custom struct key

zasweq · 2024-10-02T18:26:31Z

stats/opentelemetry/internal/tracing/custom_carrier.go

+
+// 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) {


I don't get the point of this API. It seems to do things by deferring to operations on a context either to the stats package or the metadata package? What is the function of this API?

yeah this requires a bit more background on migration path from opencensus to opentelemetry (see A72).

The design proposes that while gRPC OpenCensus directly interacts with metadata API, gRPC Open Telemetry will use standardized https://pkg.go.dev/go.opentelemetry.io/otel/propagation package for context propagation by encoding them in metadata for the following benefits:

Full integration with OpenTelemetry APIs that is easier for users to reason about.

Make it possible to plugin other propagators that the community supports.

Flexible API that allows clean and simple migration paths to a different propagator.

As of today, OpenTelemetry propagator API only supports https://pkg.go.dev/go.opentelemetry.io/otel/propagation#TextMapPropagator, that is to send string key/value pairs between the client and server, which is different from the binary header that gRPC currently uses. The future roadmap to support binary propagators at OpenTelemetry is unclear. So, gRPC will use propagator API in TextMap format with optimization path to work around the binary propagator API. Once the Open Telemetry binary propagator API is available in the future, we can continuously integrate with those API with little effort.

Therefore, we need a custom implementation for the Carrier that supports both binary and text values. For binary header grpc-trace-bin we are reusing the existing stats package to conform with existing grpc design.

zasweq · 2024-10-02T18:27:54Z

stats/opentelemetry/internal/tracing/custom_carrier.go

+	*c.Ctx = stats.SetTrace(*c.Ctx, value)
+}
+
+// Keys lists the keys stored in the gRPC context for the outgoing RPC.


It doesn't list, it returns a slice of keys.

zasweq · 2024-10-07T21:22:51Z

stats/opentelemetry/internal/tracing/custom_carrier.go

+// context. It returns an empty string if the key is not present in the
+// context.


It also returns empty string if it's set in the context but has no values. But I don't know if this is the right behavior, should we return an error? What is the intended usage of this?

Actually, I replied above the reason why empty value is not possible. We check if the SpanContext is valid in propagator's Inject() method before setting in carrier

span := oteltrace.SpanFromContext(ctx) if !span.SpanContext().IsValid() { return } ```

zasweq · 2024-10-07T21:24:52Z

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

 		return ctx
 	}

-	spanContext, ok := FromBinary([]byte(binaryData))
+	spanContext, ok := FromBinary([]byte(bd))


Switch spanContext to sc please.

zasweq · 2024-10-07T21:26:57Z

stats/opentelemetry/internal/grpc_trace_bin_propagator.go

+// 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.


Please rewrite this comment to say what Fields() returns from a conceptual point of view. What does Fields mean? Fields of what? Why is it returning a key? Or rename the function to be semantically closer to the intended idea of what this represents.

"Basically the propagation.TextMapCarrier interface which GRPCTraceBinPropagator implements has Keys()" your reply to another comment - should we call this Keys() instead?

GRPCTraceBinPropagator is implementing https://github.com/open-telemetry/opentelemetry-go/blob/main/propagation/propagation.go#L82 which has method Fields which is what i had my previous comment.

// Fields returns the keys whose values are set with Inject.
Fields() []string

Included // Fields returns the keys whose values are set with Inject. as well in docstring

zasweq · 2024-10-07T21:41:24Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+	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)
+
+		got := stats.OutgoingTrace(*carrier.Context())
+		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 := otelpropagation.MapCarrier{}
+		propagator.Inject(traceCtx, carrier)
+
+		got := carrier.Get(internaltracing.GRPCTraceBinHeaderKey)
+		want := base64.StdEncoding.EncodeToString(Binary(spanContext))
+		if got != want {
+			t.Fatalf("got = %v, want %v", got, want)
+		}
+	})


Please make t-test.

See https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/csm/observability_test.go#L200 for an example.

yeah i had tabular test before but both tests have different veriffication logic so I wrote in this way

yeah i had them as tabular test before but verification logic is significantly different because of binary and string format so i wrote it this way

I saw there are some existing tests written in similar way

You can declare the want in the t-test as a variable.

If you see logic to retrieve got is different as well. For fast path which will use SetBinary we get it through stats package because we are using CustomCarrier where as for slow path which will use Set, we just retrieve directly using carrier's Get because we are using a different carrier (not implemented by us). Would you prefer to have separate tests altogether like TestInject_FastPath, TestInject_SlowPath?

zasweq · 2024-10-07T21:41:59Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+	propagator := GRPCTraceBinPropagator{}
+	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: oteltrace.FlagsSampled,
+	})
+	traceCtx, traceCancel := context.WithCancel(context.Background())
+	traceCtx = oteltrace.ContextWithSpanContext(traceCtx, spanContext)


Variable names too long for their scope.

Changed all vars to short names

zasweq · 2024-10-07T21:42:46Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+		SpanID:     [8]byte{17, 18, 19, 20, 21, 22, 23, 24},
+		TraceFlags: oteltrace.FlagsSampled,
+	})
+	traceCtx, traceCancel := context.WithCancel(context.Background())


Defer cancel() here is preferred over calling it at the operation at the end. Defer is preferred because what happens if your test fails before it hits the last line/operation. It should still run the cleanup in that case.

zasweq · 2024-10-07T21:43:01Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+	grpctest.RunSubTests(t, s{})
+}
+
+func (s) TestInject(t *testing.T) {


Top level comment please explaining what this test does.

zasweq · 2024-10-07T21:43:31Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+	traceCancel()
+}
+
+func (s) TestExtract(t *testing.T) {


Top level comment here as well.

zasweq · 2024-10-07T21:43:45Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+		}
+		cancel()
+	})
+}


Same comments on this test as above.

zasweq · 2024-10-07T21:44:45Z

stats/opentelemetry/internal/grpc_trace_bin_propagator_test.go

+		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},


What's the reason for 16/8 here? Can we just remove?

We need to strictly adhere to OpenTelemetry SpanContext struct fields

// Fields: // // TraceId: (field_id = 0, len = 16, default = "0000000000000000") - // 16-byte array representing the trace_id. // // SpanId: (field_id = 1, len = 8, default = "00000000") - 8-byte array // representing the span_id. // // TraceOptions: (field_id = 2, len = 1, default = "0") - 1-byte array representing the trace_options.

zasweq · 2024-10-07T21:46:11Z

stats/opentelemetry/internal/tracing/custom_carrier_test.go

+	if len(a) != len(b) {
+		return false
+	}


You could sort the slices, compare length and then iterate through one and compare it to string at others index.

yeah that's an alternate way to compare two slices but do you see any major concern with count frequency approach?

Doesn't cmp.Equal/Diff just do this for you?

do you mean use cmp.Equal after sorting? Because we want to ignore the order for this comparison

https://pkg.go.dev/github.com/google/go-cmp/cmp/cmpopts#SortSlices oh yeah you can use this option.

zasweq · 2024-10-07T21:47:16Z

stats/opentelemetry/internal/tracing/custom_carrier_test.go

+		},
+	}
+
+	for _, tt := range tests {


refactor GRPCTraceBinPropagator to be exported externally Use context instead of metadata in CustomCarrier

purnesh42H force-pushed the otel-tracing-grpc-trace-bin-propagator branch from 92de02a to 9a9336b Compare September 27, 2024 17:21

purnesh42H requested review from dfawley and zasweq September 27, 2024 17:21

purnesh42H assigned dfawley and zasweq Sep 27, 2024

purnesh42H force-pushed the otel-tracing-grpc-trace-bin-propagator branch 2 times, most recently from d0ddcc0 to 5e524a8 Compare September 30, 2024 08:27

purnesh42H added the Type: Feature New features or improvements in behavior label Sep 30, 2024

purnesh42H added this to the 1.68 Release milestone Sep 30, 2024

purnesh42H added the Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability label Sep 30, 2024

dfawley removed their assignment Oct 1, 2024

zasweq reviewed Oct 2, 2024

View reviewed changes

zasweq assigned purnesh42H and unassigned zasweq Oct 2, 2024

purnesh42H requested a review from zasweq October 6, 2024 18:22

purnesh42H assigned zasweq and unassigned purnesh42H Oct 6, 2024

zasweq reviewed Oct 7, 2024

View reviewed changes

zasweq assigned purnesh42H and unassigned zasweq Oct 7, 2024

purnesh42H added 4 commits October 9, 2024 21:03

stats: opentelemetry GrpcTraceBinPropagator

326b89b

refactor GRPCTraceBinPropagator to be exported externally Use context instead of metadata in CustomCarrier

Address style, clarification review comments

ec54eed

Make *CustomCarrier implement carrier interface

b275569

Address 2nd round of style, docstring comments

f9209c3

purnesh42H force-pushed the otel-tracing-grpc-trace-bin-propagator branch from 05ec5f3 to f9209c3 Compare October 9, 2024 15:33

purnesh42H requested a review from zasweq October 9, 2024 15:41

purnesh42H assigned zasweq and unassigned purnesh42H Oct 9, 2024

use cmp.equal with cmpopts.SortSlices for equating keys slices

80820c4

		// context. It returns an empty string if the key is not present in the
		// context.

		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},

stats/internal: OpenTelemetry tracing GRPCTraceBinPropagator #7677

Are you sure you want to change the base?

stats/internal: OpenTelemetry tracing GRPCTraceBinPropagator #7677

Conversation

purnesh42H commented Sep 27, 2024 • edited Loading

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

dfawley commented Oct 1, 2024

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

purnesh42H Oct 6, 2024 •

edited

Loading

purnesh42H Oct 9, 2024 •

edited

Loading

purnesh42H Oct 9, 2024 •

edited

Loading

purnesh42H Oct 13, 2024 •

edited

Loading