From 1a665baeb096cf61e6f04dcb5c15d6cfd2bb13e0 Mon Sep 17 00:00:00 2001 From: Adam BEN LTAIFA Date: Wed, 2 Apr 2025 22:26:13 +0100 Subject: [PATCH 1/3] Code related to the deprecated function StreamClientInterceptor removed --- CHANGELOG.md | 1 + .../grpc/otelgrpc/benchmark_test.go | 8 - .../grpc/otelgrpc/interceptor.go | 151 ------ .../grpc/otelgrpc/test/go.mod | 1 - .../grpc/otelgrpc/test/grpc_test.go | 185 -------- .../grpc/otelgrpc/test/interceptor_test.go | 428 +----------------- 6 files changed, 2 insertions(+), 772 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b89474c4e23..efb567f02b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The deprecated `SemVersion` function in `go.opentelemetry.io/contrib/samplers/probability/consistent` is removed, use `Version` instead. (#7072) - The deprecated `SemVersion` function is removed in `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`, use `Version` function instead. (#7084) - The deprecated `SemVersion` function is removed in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`, use `Version` function instead. (#7085) +- The deprecated `StreamClientInterceptor` function is removed in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go`. (#7105) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/benchmark_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/benchmark_test.go index 544b28811c3..14debf8d659 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/benchmark_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/benchmark_test.go @@ -93,11 +93,3 @@ func BenchmarkUnaryClientInterceptor(b *testing.B) { )), }, nil) } - -func BenchmarkStreamClientInterceptor(b *testing.B) { - benchmark(b, []grpc.DialOption{ - grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor( - otelgrpc.WithTracerProvider(tracerProvider), - )), - }, nil) -} diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index 7d5ed058082..4a5b7df16c7 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -7,15 +7,12 @@ package otelgrpc // import "go.opentelemetry.io/contrib/instrumentation/google.g // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/rpc.md import ( "context" - "errors" - "io" "net" "strconv" "time" "google.golang.org/grpc" grpc_codes "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" @@ -115,156 +112,8 @@ func UnaryClientInterceptor(opts ...Option) grpc.UnaryClientInterceptor { } } -// clientStream wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and -// SendMsg method call. -type clientStream struct { - grpc.ClientStream - desc *grpc.StreamDesc - - span trace.Span - - receivedEvent bool - sentEvent bool - - receivedMessageID int - sentMessageID int -} - var _ = proto.Marshal -func (w *clientStream) RecvMsg(m interface{}) error { - err := w.ClientStream.RecvMsg(m) - - if err == nil && !w.desc.ServerStreams { - w.endSpan(nil) - } else if errors.Is(err, io.EOF) { - w.endSpan(nil) - } else if err != nil { - w.endSpan(err) - } else { - w.receivedMessageID++ - - if w.receivedEvent { - messageReceived.Event(w.Context(), w.receivedMessageID, m) - } - } - - return err -} - -func (w *clientStream) SendMsg(m interface{}) error { - err := w.ClientStream.SendMsg(m) - - w.sentMessageID++ - - if w.sentEvent { - messageSent.Event(w.Context(), w.sentMessageID, m) - } - - if err != nil { - w.endSpan(err) - } - - return err -} - -func (w *clientStream) Header() (metadata.MD, error) { - md, err := w.ClientStream.Header() - if err != nil { - w.endSpan(err) - } - - return md, err -} - -func (w *clientStream) CloseSend() error { - err := w.ClientStream.CloseSend() - if err != nil { - w.endSpan(err) - } - - return err -} - -func wrapClientStream(s grpc.ClientStream, desc *grpc.StreamDesc, span trace.Span, cfg *config) *clientStream { - return &clientStream{ - ClientStream: s, - span: span, - desc: desc, - receivedEvent: cfg.ReceivedEvent, - sentEvent: cfg.SentEvent, - } -} - -func (w *clientStream) endSpan(err error) { - if err != nil { - s, _ := status.FromError(err) - w.span.SetStatus(codes.Error, s.Message()) - w.span.SetAttributes(statusCodeAttr(s.Code())) - } else { - w.span.SetAttributes(statusCodeAttr(grpc_codes.OK)) - } - - w.span.End() -} - -// StreamClientInterceptor returns a grpc.StreamClientInterceptor suitable -// for use in a grpc.NewClient call. -// -// Deprecated: Use [NewClientHandler] instead. -func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { - cfg := newConfig(opts, "client") - tracer := cfg.TracerProvider.Tracer( - ScopeName, - trace.WithInstrumentationVersion(Version()), - ) - - return func( - ctx context.Context, - desc *grpc.StreamDesc, - cc *grpc.ClientConn, - method string, - streamer grpc.Streamer, - callOpts ...grpc.CallOption, - ) (grpc.ClientStream, error) { - i := &InterceptorInfo{ - Method: method, - Type: StreamClient, - } - if cfg.InterceptorFilter != nil && !cfg.InterceptorFilter(i) { - return streamer(ctx, desc, cc, method, callOpts...) - } - - name, attr, _ := telemetryAttributes(method, cc.Target()) - - startOpts := append([]trace.SpanStartOption{ - trace.WithSpanKind(trace.SpanKindClient), - trace.WithAttributes(attr...), - }, - cfg.SpanStartOptions..., - ) - - ctx, span := tracer.Start( - ctx, - name, - startOpts..., - ) - - ctx = inject(ctx, cfg.Propagators) - - s, err := streamer(ctx, desc, cc, method, callOpts...) - if err != nil { - grpcStatus, _ := status.FromError(err) - span.SetStatus(codes.Error, grpcStatus.Message()) - span.SetAttributes(statusCodeAttr(grpcStatus.Code())) - span.End() - return s, err - } - stream := wrapClientStream(s, desc, span, cfg) - return stream, nil - } -} - // UnaryServerInterceptor returns a grpc.UnaryServerInterceptor suitable // for use in a grpc.NewServer call. // diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/go.mod b/instrumentation/google.golang.org/grpc/otelgrpc/test/go.mod index c3ac6a701e7..b8311abd2bd 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/go.mod +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/go.mod @@ -9,7 +9,6 @@ require ( go.opentelemetry.io/otel/sdk v1.35.0 go.opentelemetry.io/otel/sdk/metric v1.35.0 go.opentelemetry.io/otel/trace v1.35.0 - go.uber.org/goleak v1.3.0 google.golang.org/grpc v1.71.1 ) 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 7473edee1ab..dcc996b2abf 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -83,9 +83,6 @@ func TestInterceptors(t *testing.T) { clientUnarySR := tracetest.NewSpanRecorder() clientUnaryTP := trace.NewTracerProvider(trace.WithSpanProcessor(clientUnarySR)) - clientStreamSR := tracetest.NewSpanRecorder() - clientStreamTP := trace.NewTracerProvider(trace.WithSpanProcessor(clientStreamSR)) - serverUnarySR := tracetest.NewSpanRecorder() serverUnaryTP := trace.NewTracerProvider(trace.WithSpanProcessor(serverUnarySR)) serverUnaryMetricReader := metric.NewManualReader() @@ -103,11 +100,6 @@ func TestInterceptors(t *testing.T) { 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. @@ -131,10 +123,6 @@ func TestInterceptors(t *testing.T) { checkUnaryClientSpans(t, clientUnarySR.Ended(), listener.Addr().String()) }) - t.Run("StreamClientSpans", func(t *testing.T) { - checkStreamClientSpans(t, clientStreamSR.Ended(), listener.Addr().String()) - }) - t.Run("UnaryServerSpans", func(t *testing.T) { checkUnaryServerSpans(t, serverUnarySR.Ended()) checkUnaryServerRecords(t, serverUnaryMetricReader) @@ -212,179 +200,6 @@ func checkUnaryClientSpans(t *testing.T, spans []trace.ReadOnlySpan, addr string }, largeSpan.Attributes()) } -func checkStreamClientSpans(t *testing.T, spans []trace.ReadOnlySpan, addr string) { - require.Len(t, spans, 3) - - host, p, err := net.SplitHostPort(addr) - require.NoError(t, err) - port, err := strconv.Atoi(p) - require.NoError(t, err) - - streamInput := spans[0] - assert.False(t, streamInput.EndTime().IsZero()) - assert.Equal(t, "grpc.testing.TestService/StreamingInputCall", streamInput.Name()) - // sizes from reqSizes in "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test". - assertEvents(t, []trace.Event{ - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(1), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(2), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(3), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(4), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - // client does not record an event for the server response. - }, streamInput.Events()) - assert.ElementsMatch(t, []attribute.KeyValue{ - semconv.RPCMethod("StreamingInputCall"), - semconv.RPCService("grpc.testing.TestService"), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)), - semconv.NetSockPeerAddr(host), - semconv.NetSockPeerPort(port), - }, streamInput.Attributes()) - - streamOutput := spans[1] - assert.False(t, streamOutput.EndTime().IsZero()) - assert.Equal(t, "grpc.testing.TestService/StreamingOutputCall", streamOutput.Name()) - // sizes from respSizes in "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test". - assertEvents(t, []trace.Event{ - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(1), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(1), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(2), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(3), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(4), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - }, streamOutput.Events()) - assert.ElementsMatch(t, []attribute.KeyValue{ - semconv.RPCMethod("StreamingOutputCall"), - semconv.RPCService("grpc.testing.TestService"), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)), - semconv.NetSockPeerAddr(host), - semconv.NetSockPeerPort(port), - }, streamOutput.Attributes()) - - pingPong := spans[2] - assert.False(t, pingPong.EndTime().IsZero()) - assert.Equal(t, "grpc.testing.TestService/FullDuplexCall", pingPong.Name()) - assertEvents(t, []trace.Event{ - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(1), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(1), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(2), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(2), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(3), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(3), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(4), - otelgrpc.RPCMessageTypeKey.String("SENT"), - }, - }, - { - Name: "message", - Attributes: []attribute.KeyValue{ - otelgrpc.RPCMessageIDKey.Int(4), - otelgrpc.RPCMessageTypeKey.String("RECEIVED"), - }, - }, - }, pingPong.Events()) - assert.ElementsMatch(t, []attribute.KeyValue{ - semconv.RPCMethod("FullDuplexCall"), - semconv.RPCService("grpc.testing.TestService"), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)), - semconv.NetSockPeerAddr(host), - semconv.NetSockPeerPort(port), - }, pingPong.Attributes()) -} - func checkStreamServerSpans(t *testing.T, spans []trace.ReadOnlySpan) { require.Len(t, spans, 3) 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 056e08b00c2..d016b559952 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go @@ -5,15 +5,11 @@ package test import ( "context" - "errors" - "io" "net" "strings" "testing" - "time" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" - "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal/test" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/metric" @@ -26,11 +22,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/goleak" "google.golang.org/grpc" grpc_codes "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" "google.golang.org/grpc/test/bufconn" @@ -340,408 +334,7 @@ func eventAttrMap(events []trace.Event) []map[attribute.Key]attribute.Value { return maps } -type mockClientStream struct { - Desc *grpc.StreamDesc - Ctx context.Context - msgs []grpc_testing.SimpleResponse -} - -func (mockClientStream) SendMsg(m interface{}) error { return nil } -func (c *mockClientStream) RecvMsg(m interface{}) error { - if len(c.msgs) == 0 { - return io.EOF - } - c.msgs = c.msgs[1:] - return nil -} -func (mockClientStream) CloseSend() error { return nil } -func (c mockClientStream) Context() context.Context { return c.Ctx } -func (mockClientStream) Header() (metadata.MD, error) { return nil, nil } -func (mockClientStream) Trailer() metadata.MD { return nil } - -type clientStreamOpts struct { - NumRecvMsgs int - DisableServerStreams bool - Events []otelgrpc.Event -} - -func newMockClientStream(opts clientStreamOpts) *mockClientStream { - var msgs []grpc_testing.SimpleResponse - for i := 0; i < opts.NumRecvMsgs; i++ { - msgs = append(msgs, grpc_testing.SimpleResponse{}) - } - return &mockClientStream{msgs: msgs} -} - -func createInterceptedStreamClient(t *testing.T, method string, opts clientStreamOpts) (grpc.ClientStream, *tracetest.SpanRecorder) { - mockStream := newMockClientStream(opts) - clientConn, err := grpc.NewClient("fake:8906", - grpc.WithContextDialer(ctxDialer()), - grpc.WithTransportCredentials(insecure.NewCredentials()), - ) - if err != nil { - t.Fatalf("failed to create client connection: %v", err) - } - defer clientConn.Close() - - // tracer - sr := tracetest.NewSpanRecorder() - tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) - interceptorOpts := []otelgrpc.Option{ - otelgrpc.WithTracerProvider(tp), - otelgrpc.WithSpanOptions(oteltrace.WithAttributes(attribute.Bool("custom", true))), - } - 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( - context.Background(), - &grpc.StreamDesc{ServerStreams: !opts.DisableServerStreams}, - clientConn, - method, - func(ctx context.Context, - desc *grpc.StreamDesc, - cc *grpc.ClientConn, - method string, - opts ...grpc.CallOption, - ) (grpc.ClientStream, error) { - mockStream.Desc = desc - mockStream.Ctx = ctx - return mockStream, nil - }, - ) - require.NoError(t, err, "initialize grpc stream client") - return streamClient, sr -} - -func TestStreamClientInterceptorOnBIDIStream(t *testing.T) { - defer goleak.VerifyNone(t) - - method := "/github.com.serviceName/bar" - name := "github.com.serviceName/bar" - opts := clientStreamOpts{ - NumRecvMsgs: 10, - Events: []otelgrpc.Event{otelgrpc.SentEvents, otelgrpc.ReceivedEvents}, - } - streamClient, sr := createInterceptedStreamClient(t, method, opts) - _, ok := getSpanFromRecorder(sr, name) - require.False(t, ok, "span should not end while stream is open") - - req := &grpc_testing.SimpleRequest{} - reply := &grpc_testing.SimpleResponse{} - - // send and receive fake data - for i := 0; i < 10; i++ { - _ = streamClient.SendMsg(req) - _ = streamClient.RecvMsg(reply) - } - - // The stream has been exhausted so next read should get a EOF and the stream should be considered closed. - err := streamClient.RecvMsg(reply) - require.Equal(t, io.EOF, err) - - // wait for span end that is called in separate go routine - var span trace.ReadOnlySpan - require.Eventually(t, func() bool { - span, ok = getSpanFromRecorder(sr, name) - return ok - }, 5*time.Second, time.Second, "missing span %s", name) - - expectedAttr := []attribute.KeyValue{ - semconv.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(grpc_codes.OK)), - semconv.RPCService("github.com.serviceName"), - semconv.RPCMethod("bar"), - semconv.NetPeerName("fake"), - semconv.NetPeerPort(8906), - attribute.Bool("custom", true), - } - assert.ElementsMatch(t, expectedAttr, span.Attributes()) - - events := span.Events() - require.Len(t, events, 20) - for i := 0; i < 20; i += 2 { - msgID := i/2 + 1 - validate := func(eventName string, attrs []attribute.KeyValue) { - for _, kv := range attrs { - k, v := kv.Key, kv.Value - if k == otelgrpc.RPCMessageTypeKey && v.AsString() != eventName { - t.Errorf("invalid event on index: %d expecting %s event, receive %s event", i, eventName, v.AsString()) - } - if k == otelgrpc.RPCMessageIDKey && v != attribute.IntValue(msgID) { - t.Errorf("invalid id for message event expected %d received %d", msgID, v.AsInt64()) - } - } - } - validate("SENT", events[i].Attributes) - validate("RECEIVED", events[i+1].Attributes) - } - - // ensure CloseSend can be subsequently called - _ = streamClient.CloseSend() -} - -func TestStreamClientInterceptorEvents(t *testing.T) { - testCases := []struct { - Name string - Events []otelgrpc.Event - }{ - {Name: "With both events", Events: []otelgrpc.Event{otelgrpc.SentEvents, otelgrpc.ReceivedEvents}}, - {Name: "With only sent events", Events: []otelgrpc.Event{otelgrpc.SentEvents}}, - {Name: "With only received events", Events: []otelgrpc.Event{otelgrpc.ReceivedEvents}}, - {Name: "No events", Events: []otelgrpc.Event{}}, - } - - for _, testCase := range testCases { - t.Run(testCase.Name, func(t *testing.T) { - defer goleak.VerifyNone(t) - - method := "/github.com.serviceName/bar" - name := "github.com.serviceName/bar" - streamClient, sr := createInterceptedStreamClient(t, method, clientStreamOpts{NumRecvMsgs: 1, Events: testCase.Events}) - _, ok := getSpanFromRecorder(sr, name) - require.False(t, ok, "span should not end while stream is open") - - req := &grpc_testing.SimpleRequest{} - reply := &grpc_testing.SimpleResponse{} - var eventsAttr []map[attribute.Key]attribute.Value - - // send and receive fake data - _ = streamClient.SendMsg(req) - _ = streamClient.RecvMsg(reply) - for _, event := range testCase.Events { - switch event { - case otelgrpc.SentEvents: - eventsAttr = append(eventsAttr, - map[attribute.Key]attribute.Value{ - otelgrpc.RPCMessageTypeKey: attribute.StringValue("SENT"), - otelgrpc.RPCMessageIDKey: attribute.IntValue(1), - }, - ) - case otelgrpc.ReceivedEvents: - eventsAttr = append(eventsAttr, - map[attribute.Key]attribute.Value{ - otelgrpc.RPCMessageTypeKey: attribute.StringValue("RECEIVED"), - otelgrpc.RPCMessageIDKey: attribute.IntValue(1), - }, - ) - } - } - - // The stream has been exhausted so next read should get a EOF and the stream should be considered closed. - err := streamClient.RecvMsg(reply) - require.Equal(t, io.EOF, err) - - // wait for span end that is called in separate go routine - var span trace.ReadOnlySpan - require.Eventually(t, func() bool { - span, ok = getSpanFromRecorder(sr, name) - return ok - }, 5*time.Second, time.Second, "missing span %s", name) - - if len(testCase.Events) == 0 { - assert.Empty(t, span.Events()) - } else { - assert.Len(t, span.Events(), len(eventsAttr)) - assert.Equal(t, eventsAttr, eventAttrMap(span.Events())) - } - - // ensure CloseSend can be subsequently called - _ = streamClient.CloseSend() - }) - } -} - -func TestStreamClientInterceptorOnUnidirectionalClientServerStream(t *testing.T) { - defer goleak.VerifyNone(t) - - method := "/github.com.serviceName/bar" - name := "github.com.serviceName/bar" - opts := clientStreamOpts{ - NumRecvMsgs: 1, - DisableServerStreams: true, - Events: []otelgrpc.Event{otelgrpc.ReceivedEvents, otelgrpc.SentEvents}, - } - streamClient, sr := createInterceptedStreamClient(t, method, opts) - _, ok := getSpanFromRecorder(sr, name) - require.False(t, ok, "span should not end while stream is open") - - req := &grpc_testing.SimpleRequest{} - reply := &grpc_testing.SimpleResponse{} - - // send fake data - for i := 0; i < 10; i++ { - _ = streamClient.SendMsg(req) - } - - // A real user would call CloseAndRecv() on the generated client which would generate a sequence of CloseSend() - // and RecvMsg() calls. - _ = streamClient.CloseSend() - err := streamClient.RecvMsg(reply) - require.NoError(t, err) - - // wait for span end that is called in separate go routine - var span trace.ReadOnlySpan - require.Eventually(t, func() bool { - span, ok = getSpanFromRecorder(sr, name) - return ok - }, 5*time.Second, time.Second, "missing span %s", name) - - expectedAttr := []attribute.KeyValue{ - semconv.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(grpc_codes.OK)), - semconv.RPCService("github.com.serviceName"), - semconv.RPCMethod("bar"), - semconv.NetPeerName("fake"), - semconv.NetPeerPort(8906), - attribute.Bool("custom", true), - } - assert.ElementsMatch(t, expectedAttr, span.Attributes()) - - // Note that there's no "RECEIVED" event generated for the server response. This is a bug. - events := span.Events() - require.Len(t, events, 10) - for i := 0; i < 10; i++ { - msgID := i + 1 - validate := func(eventName string, attrs []attribute.KeyValue) { - for _, kv := range attrs { - k, v := kv.Key, kv.Value - if k == otelgrpc.RPCMessageTypeKey && v.AsString() != eventName { - t.Errorf("invalid event on index: %d expecting %s event, receive %s event", i, eventName, v.AsString()) - } - if k == otelgrpc.RPCMessageIDKey && v != attribute.IntValue(msgID) { - t.Errorf("invalid id for message event expected %d received %d", msgID, v.AsInt64()) - } - } - } - validate("SENT", events[i].Attributes) - } -} - -// TestStreamClientInterceptorCancelContext tests a cancel context situation. -// There should be no goleaks. -func TestStreamClientInterceptorCancelContext(t *testing.T) { - defer goleak.VerifyNone(t) - - clientConn, err := grpc.NewClient("fake:8906", - grpc.WithContextDialer(ctxDialer()), - grpc.WithTransportCredentials(insecure.NewCredentials()), - ) - if err != nil { - t.Fatalf("failed to create client connection: %v", err) - } - defer clientConn.Close() - - // 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), - ) - - var mockClStr *mockClientStream - method := "/github.com.serviceName/bar" - name := "github.com.serviceName/bar" - - // create a context with cancel - cancelCtx, cancel := context.WithCancel(context.Background()) - defer cancel() - streamClient, err := streamCI( - cancelCtx, - &grpc.StreamDesc{ServerStreams: true}, - clientConn, - method, - func(ctx context.Context, - desc *grpc.StreamDesc, - cc *grpc.ClientConn, - method string, - opts ...grpc.CallOption, - ) (grpc.ClientStream, error) { - mockClStr = &mockClientStream{Desc: desc, Ctx: ctx} - return mockClStr, nil - }, - ) - require.NoError(t, err, "initialize grpc stream client") - _, ok := getSpanFromRecorder(sr, name) - require.False(t, ok, "span should not ended while stream is open") - - req := &grpc_testing.SimpleRequest{} - reply := &grpc_testing.SimpleResponse{} - - // send and receive fake data - for i := 0; i < 10; i++ { - _ = streamClient.SendMsg(req) - _ = streamClient.RecvMsg(reply) - } - - // close client stream - _ = streamClient.CloseSend() -} - -// TestStreamClientInterceptorWithError tests a situation that streamer returns an error. -func TestStreamClientInterceptorWithError(t *testing.T) { - defer goleak.VerifyNone(t) - - clientConn, err := grpc.NewClient("fake:8906", - grpc.WithContextDialer(ctxDialer()), - grpc.WithTransportCredentials(insecure.NewCredentials()), - ) - if err != nil { - t.Fatalf("failed to create client connection: %v", err) - } - defer clientConn.Close() - - // 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), - ) - - var mockClStr *mockClientStream - method := "/github.com.serviceName/bar" - name := "github.com.serviceName/bar" - - streamClient, err := streamCI( - context.Background(), - &grpc.StreamDesc{ServerStreams: true}, - clientConn, - method, - func(ctx context.Context, - desc *grpc.StreamDesc, - cc *grpc.ClientConn, - method string, - opts ...grpc.CallOption, - ) (grpc.ClientStream, error) { - mockClStr = &mockClientStream{Desc: desc, Ctx: ctx} - return mockClStr, errors.New("test") - }, - ) - require.Error(t, err, "initialize grpc stream client") - assert.IsType(t, &mockClientStream{}, streamClient) - - span, ok := getSpanFromRecorder(sr, name) - require.True(t, ok, "missing span %s", name) - - expectedAttr := []attribute.KeyValue{ - semconv.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(grpc_codes.Unknown)), - semconv.RPCService("github.com.serviceName"), - semconv.RPCMethod("bar"), - semconv.NetPeerName("fake"), - semconv.NetPeerPort(8906), - } - assert.ElementsMatch(t, expectedAttr, span.Attributes()) - assert.Equal(t, codes.Error, span.Status().Code) -} - +// serverChecks can't be removed in #7105. Should be removed when issue #7106 is fixed. var serverChecks = []struct { grpcCode grpc_codes.Code wantSpanCode codes.Code @@ -1108,22 +701,3 @@ func assertServerMetrics(t *testing.T, reader metric.Reader, serviceName, name s require.Len(t, rm.ScopeMetrics, 1) metricdatatest.AssertEqual(t, want, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) } - -func BenchmarkStreamClientInterceptor(b *testing.B) { - listener, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(b, err, "failed to open port") - client := newGrpcTest(b, listener, - []grpc.DialOption{ - //nolint:staticcheck // Interceptors are deprecated and will be removed in the next release. - grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor()), - }, - []grpc.ServerOption{}, - ) - - b.ResetTimer() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - for i := 0; i < b.N; i++ { - test.DoClientStreaming(ctx, client) - } -} From 3cc571d202b4d01486423a07350e1ab72f43b9b1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 8 Apr 2025 13:47:16 -0700 Subject: [PATCH 2/3] Apply suggestions from code review --- CHANGELOG.md | 2 +- .../google.golang.org/grpc/otelgrpc/test/interceptor_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efb567f02b2..0026de67970 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The deprecated `SemVersion` function in `go.opentelemetry.io/contrib/samplers/probability/consistent` is removed, use `Version` instead. (#7072) - The deprecated `SemVersion` function is removed in `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`, use `Version` function instead. (#7084) - The deprecated `SemVersion` function is removed in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`, use `Version` function instead. (#7085) -- The deprecated `StreamClientInterceptor` function is removed in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go`. (#7105) +- The deprecated `StreamClientInterceptor` function is removed in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go`. (#7126) 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 d016b559952..29c961e5e78 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go @@ -334,7 +334,7 @@ func eventAttrMap(events []trace.Event) []map[attribute.Key]attribute.Value { return maps } -// serverChecks can't be removed in #7105. Should be removed when issue #7106 is fixed. +// TODO: Remove when issue #7106 is fixed. var serverChecks = []struct { grpcCode grpc_codes.Code wantSpanCode codes.Code From 6c10111168f781f8a96564805cc28e51a70b17d5 Mon Sep 17 00:00:00 2001 From: Adam Ben Ltaifa Date: Sat, 17 May 2025 17:11:15 +0200 Subject: [PATCH 3/3] Remove serverChecks and deletion of unused code --- .../grpc/otelgrpc/test/grpc_test.go | 7 - .../grpc/otelgrpc/test/interceptor_test.go | 141 ------------------ .../grpc/otelgrpc/test/stats_handler_test.go | 135 ----------------- 3 files changed, 283 deletions(-) delete mode 100644 instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go 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 748ac1b2323..48285da845d 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -17,7 +17,6 @@ import ( "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal/test" "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" semconv "go.opentelemetry.io/otel/semconv/v1.17.0" @@ -25,12 +24,6 @@ import ( pb "google.golang.org/grpc/interop/grpc_testing" ) -var wantInstrumentationScope = instrumentation.Scope{ - Name: "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc", - SchemaURL: "https://opentelemetry.io/schemas/1.17.0", - Version: otelgrpc.Version(), -} - // newGrpcTest creates a grpc server, starts it, and returns the client, closes everything down during test cleanup. func newGrpcTest(t testing.TB, listener net.Listener, cOpt []grpc.DialOption, sOpt []grpc.ServerOption) pb.TestServiceClient { grpcServer := grpc.NewServer(sOpt...) 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 25f21b33515..99f9fc1f919 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go @@ -9,7 +9,6 @@ import ( "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" @@ -42,117 +41,6 @@ func eventAttrMap(events []trace.Event) []map[attribute.Key]attribute.Value { return maps } -// TODO: Remove when issue #7106 is fixed. -var serverChecks = []struct { - grpcCode grpc_codes.Code - wantSpanCode codes.Code - wantSpanStatusDescription string -}{ - { - grpcCode: grpc_codes.OK, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.Canceled, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.Unknown, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.Unknown.String(), - }, - { - grpcCode: grpc_codes.InvalidArgument, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.DeadlineExceeded, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.DeadlineExceeded.String(), - }, - { - grpcCode: grpc_codes.NotFound, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.AlreadyExists, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.PermissionDenied, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.ResourceExhausted, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.FailedPrecondition, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.Aborted, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.OutOfRange, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, - { - grpcCode: grpc_codes.Unimplemented, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.Unimplemented.String(), - }, - { - grpcCode: grpc_codes.Internal, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.Internal.String(), - }, - { - grpcCode: grpc_codes.Unavailable, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.Unavailable.String(), - }, - { - grpcCode: grpc_codes.DataLoss, - wantSpanCode: codes.Error, - wantSpanStatusDescription: grpc_codes.DataLoss.String(), - }, - { - grpcCode: grpc_codes.Unauthenticated, - wantSpanCode: codes.Unset, - wantSpanStatusDescription: "", - }, -} - -func assertServerSpan(t *testing.T, wantSpanCode codes.Code, wantSpanStatusDescription string, wantGrpcCode grpc_codes.Code, span trace.ReadOnlySpan) { - // validate span status - assert.Equal(t, wantSpanCode, span.Status().Code) - assert.Equal(t, wantSpanStatusDescription, span.Status().Description) - - // validate grpc code span attribute - var codeAttr attribute.KeyValue - for _, a := range span.Attributes() { - if a.Key == otelgrpc.GRPCStatusCodeKey { - codeAttr = a - break - } - } - - require.True(t, codeAttr.Valid(), "attributes contain gRPC status code") - assert.Equal(t, attribute.Int64Value(int64(wantGrpcCode)), codeAttr.Value) -} - type mockServerStream struct { grpc.ServerStream } @@ -167,35 +55,6 @@ func (m *mockServerStream) RecvMsg(_ interface{}) error { return nil } -// TestStreamServerInterceptor tests the server interceptor for streaming RPCs. -func TestStreamServerInterceptor(t *testing.T) { - for _, check := range serverChecks { - name := check.grpcCode.String() - t.Run(name, func(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), - ) - - // call the stream interceptor - grpcErr := status.Error(check.grpcCode, check.grpcCode.String()) - handler := func(_ interface{}, _ grpc.ServerStream) error { - return grpcErr - } - err := usi(&grpc_testing.SimpleRequest{}, &mockServerStream{}, &grpc.StreamServerInfo{FullMethod: name}, handler) - assert.Equal(t, grpcErr, err) - - // validate span - span, ok := getSpanFromRecorder(sr, name) - require.True(t, ok, "missing span %s", name) - assertServerSpan(t, check.wantSpanCode, check.wantSpanStatusDescription, check.grpcCode, span) - }) - } -} - func TestStreamServerInterceptorEvents(t *testing.T) { testCases := []struct { Name string diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go deleted file mode 100644 index 1f96bbd4f8e..00000000000 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go +++ /dev/null @@ -1,135 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package test - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - grpc_codes "google.golang.org/grpc/codes" - "google.golang.org/grpc/stats" - "google.golang.org/grpc/status" - - "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/sdk/metric" - "go.opentelemetry.io/otel/sdk/metric/metricdata" - "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" - "go.opentelemetry.io/otel/sdk/trace" - "go.opentelemetry.io/otel/sdk/trace/tracetest" - semconv "go.opentelemetry.io/otel/semconv/v1.17.0" -) - -func TestStatsHandlerHandleRPCServerErrors(t *testing.T) { - for _, check := range serverChecks { - name := check.grpcCode.String() - t.Run(name, func(t *testing.T) { - t.Setenv("OTEL_METRICS_EXEMPLAR_FILTER", "always_off") - sr := tracetest.NewSpanRecorder() - tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) - - mr := metric.NewManualReader() - mp := metric.NewMeterProvider(metric.WithReader(mr)) - - serverHandler := otelgrpc.NewServerHandler( - otelgrpc.WithTracerProvider(tp), - otelgrpc.WithMeterProvider(mp), - otelgrpc.WithMetricAttributes(testMetricAttr), - ) - - serviceName := "TestGrpcService" - methodName := serviceName + "/" + name - fullMethodName := "/" + methodName - // call the server handler - ctx := serverHandler.TagRPC(context.Background(), &stats.RPCTagInfo{ - FullMethodName: fullMethodName, - }) - - grpcErr := status.Error(check.grpcCode, check.grpcCode.String()) - serverHandler.HandleRPC(ctx, &stats.End{ - Error: grpcErr, - }) - - // validate span - span, ok := getSpanFromRecorder(sr, methodName) - require.True(t, ok, "missing span %s", methodName) - assertServerSpan(t, check.wantSpanCode, check.wantSpanStatusDescription, check.grpcCode, span) - - // validate metric - assertStatsHandlerServerMetrics(t, mr, serviceName, name, check.grpcCode) - }) - } -} - -func assertStatsHandlerServerMetrics(t *testing.T, reader metric.Reader, serviceName, name string, code grpc_codes.Code) { - want := metricdata.ScopeMetrics{ - Scope: wantInstrumentationScope, - Metrics: []metricdata.Metrics{ - { - Name: "rpc.server.duration", - Description: "Measures the duration of inbound RPC.", - Unit: "ms", - Data: metricdata.Histogram[float64]{ - Temporality: metricdata.CumulativeTemporality, - DataPoints: []metricdata.HistogramDataPoint[float64]{ - { - Attributes: attribute.NewSet( - semconv.RPCMethod(name), - semconv.RPCService(serviceName), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(code)), - testMetricAttr, - ), - }, - }, - }, - }, - { - Name: "rpc.server.requests_per_rpc", - Description: "Measures the number of messages received per RPC. Should be 1 for all non-streaming RPCs.", - Unit: "{count}", - Data: metricdata.Histogram[int64]{ - Temporality: metricdata.CumulativeTemporality, - DataPoints: []metricdata.HistogramDataPoint[int64]{ - { - Attributes: attribute.NewSet( - semconv.RPCMethod(name), - semconv.RPCService(serviceName), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(code)), - testMetricAttr, - ), - }, - }, - }, - }, - { - Name: "rpc.server.responses_per_rpc", - Description: "Measures the number of messages received per RPC. Should be 1 for all non-streaming RPCs.", - Unit: "{count}", - Data: metricdata.Histogram[int64]{ - Temporality: metricdata.CumulativeTemporality, - DataPoints: []metricdata.HistogramDataPoint[int64]{ - { - Attributes: attribute.NewSet( - semconv.RPCMethod(name), - semconv.RPCService(serviceName), - otelgrpc.RPCSystemGRPC, - otelgrpc.GRPCStatusCodeKey.Int64(int64(code)), - testMetricAttr, - ), - }, - }, - }, - }, - }, - } - rm := metricdata.ResourceMetrics{} - err := reader.Collect(context.Background(), &rm) - assert.NoError(t, err) - require.Len(t, rm.ScopeMetrics, 1) - metricdatatest.AssertEqual(t, want, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) -}