From e43928654816141b9475522e6d910fb40df53465 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy <126021+ash2k@users.noreply.github.com> Date: Mon, 7 Aug 2023 22:48:08 +1000 Subject: [PATCH] Fix gRPC service/method URL path parsing discrepancies (#4135) --- CHANGELOG.md | 1 + .../grpc/otelgrpc/internal/parse.go | 14 +++- .../grpc/otelgrpc/internal/parse_test.go | 70 +++++++++++++++++++ .../grpc/otelgrpc/test/interceptor_test.go | 66 +---------------- 4 files changed, 84 insertions(+), 67 deletions(-) create mode 100644 instrumentation/google.golang.org/grpc/otelgrpc/internal/parse_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a49d1555998..f6f8162653f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` - Do not modify the origin request in RoundTripper in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4033) - Handle empty value of `OTEL_PROPAGATORS` environment variable the same way as when the variable is unset in `go.opentelemetry.io/contrib/propagators/autoprop`. (#4101) +- Fix gRPC service/method URL path parsing discrepancies. (#4135) ### Deprecated diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse.go b/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse.go index ae160d58750..cf32a9e978c 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse.go @@ -24,13 +24,21 @@ import ( // ParseFullMethod returns a span name following the OpenTelemetry semantic // conventions as well as all applicable span attribute.KeyValue attributes based // on a gRPC's FullMethod. +// +// Parsing is consistent with grpc-go implementation: +// https://github.com/grpc/grpc-go/blob/v1.57.0/internal/grpcutil/method.go#L26-L39 func ParseFullMethod(fullMethod string) (string, []attribute.KeyValue) { - name := strings.TrimLeft(fullMethod, "/") - service, method, found := strings.Cut(name, "/") - if !found { + if !strings.HasPrefix(fullMethod, "/") { + // Invalid format, does not follow `/package.service/method`. + return fullMethod, nil + } + name := fullMethod[1:] + pos := strings.LastIndex(name, "/") + if pos < 0 { // Invalid format, does not follow `/package.service/method`. return name, nil } + service, method := name[:pos], name[pos+1:] var attrs []attribute.KeyValue if service != "" { diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse_test.go new file mode 100644 index 00000000000..96b487f198d --- /dev/null +++ b/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse_test.go @@ -0,0 +1,70 @@ +// Copyright The OpenTelemetry 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 "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal" + +import ( + "testing" + + "go.opentelemetry.io/otel/attribute" + semconv "go.opentelemetry.io/otel/semconv/v1.17.0" + + "github.com/stretchr/testify/assert" +) + +func TestParseFullMethod(t *testing.T) { + cases := []struct { + input string + expectedName string + expectedAttrs []attribute.KeyValue + }{ + { + input: "no_slash/method", + expectedName: "no_slash/method", + }, + { + input: "/slash_but_no_second_slash", + expectedName: "slash_but_no_second_slash", + }, + { + input: "/service_only/", + expectedName: "service_only/", + expectedAttrs: []attribute.KeyValue{ + semconv.RPCService("service_only"), + }, + }, + { + input: "//method_only", + expectedName: "/method_only", + expectedAttrs: []attribute.KeyValue{ + semconv.RPCMethod("method_only"), + }, + }, + { + input: "/service/method", + expectedName: "service/method", + expectedAttrs: []attribute.KeyValue{ + semconv.RPCService("service"), + semconv.RPCMethod("method"), + }, + }, + } + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + name, attrs := ParseFullMethod(tc.input) + assert.Equal(t, tc.expectedName, name) + assert.Equal(t, tc.expectedAttrs, attrs) + }) + } +} 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 97576cba92f..868399b8f9e 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go @@ -24,7 +24,6 @@ import ( "time" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" - "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" @@ -217,7 +216,7 @@ func TestUnaryClientInterceptor(t *testing.T) { eventsAttr: []map[attribute.Key]attribute.Value{}, }, { - method: "serviceName/bar", + method: "/serviceName/bar", name: "serviceName/bar", interceptor: unaryInterceptor, expectedAttr: []attribute.KeyValue{ @@ -240,7 +239,7 @@ func TestUnaryClientInterceptor(t *testing.T) { }, }, { - method: "serviceName/bar_error", + method: "/serviceName/bar_error", name: "serviceName/bar_error", interceptor: unaryInterceptor, expectedSpanCode: codes.Error, @@ -1056,64 +1055,3 @@ func TestStreamServerInterceptorEvents(t *testing.T) { }) } } - -func TestParseFullMethod(t *testing.T) { - tests := []struct { - fullMethod string - name string - attr []attribute.KeyValue - }{ - { - fullMethod: "/grpc.test.EchoService/Echo", - name: "grpc.test.EchoService/Echo", - attr: []attribute.KeyValue{ - semconv.RPCService("grpc.test.EchoService"), - semconv.RPCMethod("Echo"), - }, - }, { - fullMethod: "/com.example.ExampleRmiService/exampleMethod", - name: "com.example.ExampleRmiService/exampleMethod", - attr: []attribute.KeyValue{ - semconv.RPCService("com.example.ExampleRmiService"), - semconv.RPCMethod("exampleMethod"), - }, - }, { - fullMethod: "/MyCalcService.Calculator/Add", - name: "MyCalcService.Calculator/Add", - attr: []attribute.KeyValue{ - semconv.RPCService("MyCalcService.Calculator"), - semconv.RPCMethod("Add"), - }, - }, { - fullMethod: "/MyServiceReference.ICalculator/Add", - name: "MyServiceReference.ICalculator/Add", - attr: []attribute.KeyValue{ - semconv.RPCService("MyServiceReference.ICalculator"), - semconv.RPCMethod("Add"), - }, - }, { - fullMethod: "/MyServiceWithNoPackage/theMethod", - name: "MyServiceWithNoPackage/theMethod", - attr: []attribute.KeyValue{ - semconv.RPCService("MyServiceWithNoPackage"), - semconv.RPCMethod("theMethod"), - }, - }, { - fullMethod: "/pkg.srv", - name: "pkg.srv", - attr: []attribute.KeyValue(nil), - }, { - fullMethod: "/pkg.srv/", - name: "pkg.srv/", - attr: []attribute.KeyValue{ - semconv.RPCService("pkg.srv"), - }, - }, - } - - for _, test := range tests { - n, a := internal.ParseFullMethod(test.fullMethod) - assert.Equal(t, test.name, n) - assert.Equal(t, test.attr, a) - } -}