From ad3e39cab195674c9a960ce5c2555f5da07da371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 7 Nov 2023 09:09:28 +0100 Subject: [PATCH 1/2] otelgrpc: Deprecate interceptors in favor of stats handlers --- CHANGELOG.md | 3 +- .../google.golang.org/grpc/otelgrpc/config.go | 2 ++ .../google.golang.org/grpc/otelgrpc/doc.go | 29 ++----------------- ...le_interceptor_test.go => example_test.go} | 16 +++------- .../grpc/otelgrpc/interceptor.go | 8 +++++ .../grpc/otelgrpc/test/grpc_test.go | 4 +++ .../grpc/otelgrpc/test/interceptor_test.go | 11 +++++++ 7 files changed, 34 insertions(+), 39 deletions(-) rename instrumentation/google.golang.org/grpc/otelgrpc/{example_interceptor_test.go => example_test.go} (59%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5741c65439e..2301831959c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add metrics support (No-op, OTLP and Prometheus) to `go.opentelemetry.io/contrib/exporters/autoexport`. (#4229, #4479) - Add support for `console` span exporter and metrics exporter in `go.opentelemetry.io/contrib/exporters/autoexport`. (#4486) - Set unit and description on all instruments in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4500) -- Add metric support for `grpc.StatsHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4356) +- Add metric support for `grpc.StatsHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4356) ### Changed @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Deprecated - In `go.opentelemetry.io/contrib/exporters/autoexport`, `Option` was renamed to `SpanOption`. The old name is deprecated but continues to be supported as an alias. (#4229) +- The interceptors (`UnaryClientInterceptor`, `StreamClientInterceptor`, `UnaryServerInterceptor`, `StreamServerInterceptor`, `WithInterceptorFilter`) are deprecated. Use stats handlers (`NewClientHandler`, `NewServerHandler`) instead. (#4534) ### Fixed diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/config.go b/instrumentation/google.golang.org/grpc/otelgrpc/config.go index bad05b6734f..7e7f920b9d1 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/config.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/config.go @@ -145,6 +145,8 @@ func (o tracerProviderOption) apply(c *config) { } // WithInterceptorFilter returns an Option to use the request filter. +// +// Deprecated: Use stats handlers instead. func WithInterceptorFilter(f Filter) Option { return interceptorFilterOption{f: f} } diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/doc.go b/instrumentation/google.golang.org/grpc/otelgrpc/doc.go index a993e0fc921..958dcd87a4c 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/doc.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/doc.go @@ -13,33 +13,10 @@ // limitations under the License. /* -Package otelgrpc is the instrumentation library for [google.golang.org/grpc] +Package otelgrpc is the instrumentation library for [google.golang.org/grpc]. -For now you can instrument your program which use [google.golang.org/grpc] in two ways: +Use [NewClientHandler] with [grpc.WithStatsHandler] to instrument a gRPC client. - - by [grpc.UnaryClientInterceptor], [grpc.UnaryServerInterceptor], [grpc.StreamClientInterceptor], [grpc.StreamServerInterceptor] - - by [stats.Handler] - -Notice: Do not use both interceptors and [stats.Handler] at the same time! If so, you will get duplicated spans and the parent/child relationships between spans will also be broken. - -We strongly still recommand you to use [stats.Handler], mainly for two reasons: - -Functional advantages: [stats.Handler] has more information for user to build more flexible and granular metric, for example - - - multiple different types of represent "data length": In [stats.InPayload], there exists "Length", "CompressedLength", "WireLength" to denote the size of uncompressed, compressed payload data, with or without framing data. But in interceptors, we can only got uncompressed data, and this feature is also removed due to performance problem. - - - more accurate timestamp: [stats.InPayload]'s "RecvTime" and [stats.OutPayload]'s "SentTime" records more accurate timestamp that server got and sent the message, the timestamp recorded by interceptors depends on the location of this interceptors in the total interceptor chain. - - - some other use cases: for example, catch failure of decoding message. - -Performance advantages: If too many interceptors are registered in a service, the interceptor chain can become too long, which increases the latency and processing time of the entire RPC call. - -[stats.Handler]: https://pkg.go.dev/google.golang.org/grpc/stats#Handler -[grpc.UnaryClientInterceptor]: https://pkg.go.dev/google.golang.org/grpc#UnaryClientInterceptor -[grpc.UnaryServerInterceptor]: https://pkg.go.dev/google.golang.org/grpc#UnaryServerInterceptor -[grpc.StreamClientInterceptor]: https://pkg.go.dev/google.golang.org/grpc#StreamClientInterceptor -[grpc.StreamServerInterceptor]: https://pkg.go.dev/google.golang.org/grpc#StreamServerInterceptor -[stats.OutPayload]: https://pkg.go.dev/google.golang.org/grpc/stats#OutPayload -[stats.InPayload]: https://pkg.go.dev/google.golang.org/grpc/stats#InPayload +Use [NewServerHandler] with [grpc.StatsHandler] to instrument a gRPC server. */ package otelgrpc // import "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/example_interceptor_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/example_test.go similarity index 59% rename from instrumentation/google.golang.org/grpc/otelgrpc/example_interceptor_test.go rename to instrumentation/google.golang.org/grpc/otelgrpc/example_test.go index 908a5c477ac..5aa7bfd6914 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/example_interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/example_test.go @@ -20,18 +20,10 @@ import ( "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" ) -func ExampleStreamClientInterceptor() { - _, _ = grpc.Dial("localhost", grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor())) +func ExampleNewClientHandler() { + _, _ = grpc.Dial("localhost", grpc.WithStatsHandler(otelgrpc.NewClientHandler())) } -func ExampleUnaryClientInterceptor() { - _, _ = grpc.Dial("localhost", grpc.WithUnaryInterceptor(otelgrpc.UnaryClientInterceptor())) -} - -func ExampleStreamServerInterceptor() { - _ = grpc.NewServer(grpc.StreamInterceptor(otelgrpc.StreamServerInterceptor())) -} - -func ExampleUnaryServerInterceptor() { - _ = grpc.NewServer(grpc.UnaryInterceptor(otelgrpc.UnaryServerInterceptor())) +func ExampleNewServerHandler() { + _ = grpc.NewServer(grpc.StatsHandler(otelgrpc.NewServerHandler())) } diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index c737d29dcd0..8ccb5db8af7 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -60,6 +60,8 @@ var ( // UnaryClientInterceptor returns a grpc.UnaryClientInterceptor suitable // for use in a grpc.Dial call. +// +// Deprecated: Use [NewClientHandler] instead. func UnaryClientInterceptor(opts ...Option) grpc.UnaryClientInterceptor { cfg := newConfig(opts, "client") tracer := cfg.TracerProvider.Tracer( @@ -254,6 +256,8 @@ func (w *clientStream) sendStreamEvent(eventType streamEventType, err error) { // StreamClientInterceptor returns a grpc.StreamClientInterceptor suitable // for use in a grpc.Dial call. +// +// Deprecated: Use [NewClientHandler] instead. func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { cfg := newConfig(opts, "client") tracer := cfg.TracerProvider.Tracer( @@ -324,6 +328,8 @@ func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { // UnaryServerInterceptor returns a grpc.UnaryServerInterceptor suitable // for use in a grpc.NewServer call. +// +// Deprecated: Use [NewServerHandler] instead. func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { cfg := newConfig(opts, "server") tracer := cfg.TracerProvider.Tracer( @@ -445,6 +451,8 @@ func wrapServerStream(ctx context.Context, ss grpc.ServerStream, cfg *config) *s // StreamServerInterceptor returns a grpc.StreamServerInterceptor suitable // for use in a grpc.NewServer call. +// +// Deprecated: Use [NewServerHandler] instead. func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { cfg := newConfig(opts, "server") tracer := cfg.TracerProvider.Tracer( diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go index 6d8a82676ba..ab001567eb5 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -108,21 +108,25 @@ func TestInterceptors(t *testing.T) { listener, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err, "failed to open port") err = newGrpcTest(listener, []grpc.DialOption{ + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. grpc.WithUnaryInterceptor(otelgrpc.UnaryClientInterceptor( otelgrpc.WithTracerProvider(clientUnaryTP), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), )), + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor( otelgrpc.WithTracerProvider(clientStreamTP), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), )), }, []grpc.ServerOption{ + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. grpc.UnaryInterceptor(otelgrpc.UnaryServerInterceptor( otelgrpc.WithTracerProvider(serverUnaryTP), otelgrpc.WithMeterProvider(serverUnaryMP), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), )), + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. grpc.StreamInterceptor(otelgrpc.StreamServerInterceptor( otelgrpc.WithTracerProvider(serverStreamTP), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go index 03e1876ceb2..400f970dba0 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go @@ -92,19 +92,23 @@ func TestUnaryClientInterceptor(t *testing.T) { sr := tracetest.NewSpanRecorder() tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. unaryInterceptor := otelgrpc.UnaryClientInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), otelgrpc.WithSpanOptions(oteltrace.WithAttributes(attribute.Bool("custom", true))), ) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. unaryInterceptorOnlySentEvents := otelgrpc.UnaryClientInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMessageEvents(otelgrpc.SentEvents), ) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. unaryInterceptorOnlyReceivedEvents := otelgrpc.UnaryClientInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents), ) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. unaryInterceptorNoEvents := otelgrpc.UnaryClientInterceptor( otelgrpc.WithTracerProvider(tp), ) @@ -405,6 +409,7 @@ func createInterceptedStreamClient(t *testing.T, method string, opts clientStrea if len(opts.Events) > 0 { interceptorOpts = append(interceptorOpts, otelgrpc.WithMessageEvents(opts.Events...)) } + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. streamCI := otelgrpc.StreamClientInterceptor(interceptorOpts...) streamClient, err := streamCI( @@ -651,6 +656,7 @@ func TestStreamClientInterceptorCancelContext(t *testing.T) { // tracer sr := tracetest.NewSpanRecorder() tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. streamCI := otelgrpc.StreamClientInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), @@ -714,6 +720,7 @@ func TestStreamClientInterceptorWithError(t *testing.T) { // tracer sr := tracetest.NewSpanRecorder() tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. streamCI := otelgrpc.StreamClientInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents), @@ -877,6 +884,7 @@ func TestUnaryServerInterceptor(t *testing.T) { mr := metric.NewManualReader() mp := metric.NewMeterProvider(metric.WithReader(mr)) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. usi := otelgrpc.UnaryServerInterceptor( otelgrpc.WithTracerProvider(tp), otelgrpc.WithMeterProvider(mp), @@ -937,6 +945,7 @@ func TestUnaryServerInterceptorEvents(t *testing.T) { if len(testCase.Events) > 0 { opts = append(opts, otelgrpc.WithMessageEvents(testCase.Events...)) } + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. usi := otelgrpc.UnaryServerInterceptor(opts...) grpcCode := grpc_codes.OK name := grpcCode.String() @@ -998,6 +1007,7 @@ func TestStreamServerInterceptor(t *testing.T) { sr := tracetest.NewSpanRecorder() tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. usi := otelgrpc.StreamServerInterceptor( otelgrpc.WithTracerProvider(tp), ) @@ -1039,6 +1049,7 @@ func TestStreamServerInterceptorEvents(t *testing.T) { if len(testCase.Events) > 0 { opts = append(opts, otelgrpc.WithMessageEvents(testCase.Events...)) } + //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. usi := otelgrpc.StreamServerInterceptor(opts...) stream := &mockServerStream{} From 69ebb5354d89a12213fee29197eee8e63e3f1532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 7 Nov 2023 09:18:45 +0100 Subject: [PATCH 2/2] Update opencensus example --- propagators/opencensus/examples/opentelemetry_server/server.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/propagators/opencensus/examples/opentelemetry_server/server.go b/propagators/opencensus/examples/opentelemetry_server/server.go index ed2f6c32387..9ba69110d96 100644 --- a/propagators/opencensus/examples/opentelemetry_server/server.go +++ b/propagators/opencensus/examples/opentelemetry_server/server.go @@ -71,8 +71,7 @@ func main() { // handler to enable tracing. log.Println("Starting the GRPC server, and using the OpenCensus binary propagation format.") s := grpc.NewServer( - grpc.UnaryInterceptor(otelgrpc.UnaryServerInterceptor(otelgrpc.WithPropagators(opencensus.Binary{}))), - grpc.StreamInterceptor(otelgrpc.StreamServerInterceptor(otelgrpc.WithPropagators(opencensus.Binary{})))) + grpc.StatsHandler(otelgrpc.NewServerHandler(otelgrpc.WithPropagators(opencensus.Binary{})))) pb.RegisterGreeterServer(s, &server{}) if err := s.Serve(lis); err != nil {