stats/opentelemetry: introduce tracing propagator and carrier #7677
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7677 +/- ##
==========================================
- Coverage 81.91% 81.86% -0.05%
==========================================
Files 374 377 +3
Lines 37930 38065 +135
==========================================
+ Hits 31071 31163 +92
- Misses 5566 5591 +25
- Partials 1293 1311 +18
|
purnesh42H
force-pushed
the
otel-tracing-grpc-trace-bin-propagator
branch
from
92de02a
to
9a9336b
Compare
purnesh42H
force-pushed
the
otel-tracing-grpc-trace-bin-propagator
branch
2 times, most recently
from
d0ddcc0
to
5e524a8
Compare
purnesh42H
added
the
Area: Observability
|
I'll let @zasweq do the primary review here and just take a 2nd pass. |
| // 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. |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Removed returning error from here to be consistent with Get(..)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is empty string a valid key for this?
Binary methods will not deal with string keys because we are using existing stats package
Line 316 in d9d8f34
|
|
||
| // 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // context. It returns an empty string if the key is not present in the | ||
| // context. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
}
```
There was a problem hiding this comment.
Please add to documentation the case if it's set in context but has no values. We don't assume this operation comes after that in the callsite, this is a documentation for this layer/operation.
| 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) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Please make t-test.
See https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/csm/observability_test.go#L200 for an example.
There was a problem hiding this comment.
yeah i had tabular test before but both tests have different veriffication logic so I wrote in this way
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I saw there are some existing tests written in similar way
There was a problem hiding this comment.
You can declare the want in the t-test as a variable.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah separate tests are preferred over t.Run(). Thanks.
There was a problem hiding this comment.
Separated the fast and slow path tests
| } | ||
| cancel() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Same comments on this test as above.
purnesh42H
force-pushed
the
otel-tracing-grpc-trace-bin-propagator
branch
from
02196c9
to
d708a8b
Compare
purnesh42H
force-pushed
the
otel-tracing-grpc-trace-bin-propagator
branch
from
1e22ed3
to
0c80222
Compare
purnesh42H
force-pushed
the
otel-tracing-grpc-trace-bin-propagator
branch
from
0c80222
to
a3c8693
Compare
| if !test.injectSC.IsValid() { | ||
| if len(gotH) > 0 { | ||
| t.Fatalf("got %v value from Carrier's context metadata grpc-trace-bin header, want empty", gotH) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
When your test cases diverge in behavior so much, it really just feels like two different tests instead of one table driven test. Optionally rewrite that way to make it easier to understand.
There was a problem hiding this comment.
Wrote separate tests for valid and invalid span context injection
This PR adds the
GRPCTraceBinPropagatorthat implementsTextMapPropagatoras 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 usinggrpc-trace-binheader for context propagation and also support any other text propagators. When usinggrpc-trace-binthe 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 useGRPCTraceBinPropagatorfor the migration. Using the same header greatly simplifies rollout.RELEASE NOTES: None