Move wrapping of errors for stacktraces into the trace package

Also added package information to types in trace and fixed places in the code where we were manually wrapping in chain element. Signed-off-by: Ed Warnicke <hagbard@gmail.com>
networkservicemesh · Apr 3, 2020 · f018d7a · f018d7a
1 parent fd5d675
commit f018d7a
Show file tree

Hide file tree

Showing 13 changed files with 111 additions and 20 deletions.
diff --git a/pkg/networkservice/common/authorize/server_test.go b/pkg/networkservice/common/authorize/server_test.go
@@ -28,7 +28,6 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/networkservicemesh/sdk/pkg/networkservice/common/authorize"
-	"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
 )
 
 const (
@@ -89,7 +88,7 @@ func TestAuthzEndpoint(t *testing.T) {
 				rego.Module("example.com", s.policy)).PrepareForEval(context.Background())
 			require.Nilf(t, err, "failed to create new rego policy: %v", err)
 
-			srv := chain.NewNetworkServiceServer(authorize.NewServer(&p))
+			srv := authorize.NewServer(&p)
 
 			checkResult := func(err error) {
 				if !s.denied {

diff --git a/pkg/networkservice/common/heal/client.go b/pkg/networkservice/common/heal/client.go
@@ -21,10 +21,8 @@ import (
 	"context"
 	"time"
 
-	"github.com/sirupsen/logrus"
-
 	"github.com/golang/protobuf/ptypes/empty"
-	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
@@ -103,7 +101,6 @@ func (f *healClient) init() {
 		})
 		return
 	}
-
 	f.eventReceiver = recv
 	f.recvEventExecutor.AsyncExec(f.recvEvent)
 }
@@ -156,7 +153,7 @@ func (f *healClient) recvEvent() {
 func (f *healClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
 	rv, err := next.Client(ctx).Request(ctx, request, opts...)
 	if err != nil {
-		return nil, errors.Wrap(err, "Error calling next")
+		return nil, err
 	}
 	// Clone the request
 	req := request.Clone()
@@ -193,7 +190,7 @@ func (f *healClient) Request(ctx context.Context, request *networkservice.Networ
 func (f *healClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) {
 	rv, err := next.Client(ctx).Close(ctx, conn, opts...)
 	if err != nil {
-		return nil, errors.Wrap(err, "Error calling next")
+		return nil, err
 	}
 	f.updateExecutor.AsyncExec(func() {
 		delete(f.requestors, conn.GetId())

diff --git a/pkg/networkservice/common/refresh/client.go b/pkg/networkservice/common/refresh/client.go
@@ -55,7 +55,7 @@ func NewClient() networkservice.NetworkServiceClient {
 func (t *refreshClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
 	rv, err := next.Server(ctx).Request(ctx, request)
 	if err != nil {
-		return nil, errors.Wrap(err, "Error calling next")
+		return nil, err
 	}
 	// Clone the request
 	req := request.Clone()

diff --git a/pkg/networkservice/common/timeout/server.go b/pkg/networkservice/common/timeout/server.go
@@ -23,8 +23,6 @@ import (
 
 	"github.com/golang/protobuf/ptypes"
 	"github.com/golang/protobuf/ptypes/empty"
-	"github.com/pkg/errors"
-
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
@@ -64,7 +62,7 @@ func NewServer(onTimout *networkservice.NetworkServiceServer) networkservice.Net
 func (t *timeoutServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
 	ct, err := t.createTimer(ctx, request)
 	if err != nil {
-		return nil, errors.Wrapf(err, "Error creating timer from Request.Connection.Path.PathSegment[%d].ExpireTime", request.GetConnection().GetPath().GetIndex())
+		return nil, err
 	}
 	t.executor.AsyncExec(func() {
 		if timer, ok := t.connections[request.GetConnection().GetId()]; !ok || timer.Stop() {

diff --git a/pkg/networkservice/core/trace/client.go b/pkg/networkservice/core/trace/client.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
@@ -40,7 +41,8 @@ func NewNetworkServiceClient(traced networkservice.NetworkServiceClient) network
 
 func (t *traceClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.Request", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -53,6 +55,9 @@ func (t *traceClient) Request(ctx context.Context, request *networkservice.Netwo
 	rv, err := t.traced.Request(ctx, request, opts...)
 
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}
@@ -62,7 +67,8 @@ func (t *traceClient) Request(ctx context.Context, request *networkservice.Netwo
 
 func (t *traceClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Close", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.Close", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 	// Make sure we log to span
 	ctx = withLog(span.Context(), span.Logger())
@@ -71,6 +77,9 @@ func (t *traceClient) Close(ctx context.Context, conn *networkservice.Connection
 	rv, err := t.traced.Close(ctx, conn, opts...)
 
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s/", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/networkservice/core/trace/server.go b/pkg/networkservice/core/trace/server.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/pkg/errors"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 
@@ -39,7 +40,8 @@ func NewNetworkServiceServer(traced networkservice.NetworkServiceServer) network
 
 func (t *traceServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.Request", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -52,6 +54,9 @@ func (t *traceServer) Request(ctx context.Context, request *networkservice.Netwo
 	rv, err := t.traced.Request(ctx, request)
 
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}
@@ -61,7 +66,8 @@ func (t *traceServer) Request(ctx context.Context, request *networkservice.Netwo
 
 func (t *traceServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Close", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.Close", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 	// Make sure we log to span
 	ctx = withLog(span.Context(), span.Logger())
@@ -70,6 +76,9 @@ func (t *traceServer) Close(ctx context.Context, conn *networkservice.Connection
 	rv, err := t.traced.Close(ctx, conn)
 
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/networkservice/core/trace/utils.go b/pkg/networkservice/core/trace/utils.go
@@ -0,0 +1,25 @@
+// Copyright (c) 2020 Cisco and/or its affiliates.
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+// 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 trace
+
+import (
+	"github.com/pkg/errors"
+)
+
+type stackTracer interface {
+	StackTrace() errors.StackTrace
+}
diff --git a/pkg/registry/core/trace/discovery_client.go b/pkg/registry/core/trace/discovery_client.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 
 	"github.com/networkservicemesh/api/pkg/api/registry"
@@ -39,7 +40,8 @@ func NewDiscoveryClient(traced registry.NetworkServiceDiscoveryClient) registry.
 
 func (t *traceDiscoveryClient) FindNetworkService(ctx context.Context, request *registry.FindNetworkServiceRequest, opts ...grpc.CallOption) (*registry.FindNetworkServiceResponse, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.FindNetworkService", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -51,6 +53,9 @@ func (t *traceDiscoveryClient) FindNetworkService(ctx context.Context, request *
 	// Actually call the next
 	rv, err := t.traced.FindNetworkService(ctx, request, opts...)
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s/", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/registry/core/trace/discovery_server.go b/pkg/registry/core/trace/discovery_server.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/networkservicemesh/api/pkg/api/registry"
+	"github.com/pkg/errors"
 
 	"github.com/networkservicemesh/sdk/pkg/tools/spanhelper"
 	"github.com/networkservicemesh/sdk/pkg/tools/typeutils"
@@ -37,7 +38,8 @@ func NewDiscoveryServer(traced registry.NetworkServiceDiscoveryServer) registry.
 
 func (t *traceDiscoveryServer) FindNetworkService(ctx context.Context, request *registry.FindNetworkServiceRequest) (*registry.FindNetworkServiceResponse, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.FindNetworkService", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -50,6 +52,9 @@ func (t *traceDiscoveryServer) FindNetworkService(ctx context.Context, request *
 	rv, err := t.traced.FindNetworkService(ctx, request)
 
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s/", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/registry/core/trace/registry_client.go b/pkg/registry/core/trace/registry_client.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 
 	"github.com/networkservicemesh/api/pkg/api/registry"
@@ -40,7 +41,8 @@ func NewRegistryClient(traced registry.NetworkServiceRegistryClient) registry.Ne
 
 func (t *traceRegistryClient) RegisterNSE(ctx context.Context, request *registry.NSERegistration, opts ...grpc.CallOption) (*registry.NSERegistration, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.RegisterNSE", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -52,6 +54,9 @@ func (t *traceRegistryClient) RegisterNSE(ctx context.Context, request *registry
 	// Actually call the next
 	rv, err := t.traced.RegisterNSE(ctx, request, opts...)
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s/", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/registry/core/trace/registry_server.go b/pkg/registry/core/trace/registry_server.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/pkg/errors"
 
 	"github.com/networkservicemesh/api/pkg/api/registry"
 
@@ -39,7 +40,8 @@ func NewRegistryServer(traced registry.NetworkServiceRegistryServer) registry.Ne
 
 func (t *traceRegistryServer) RegisterNSE(ctx context.Context, request *registry.NSERegistration) (*registry.NSERegistration, error) {
 	// Create a new span
-	span := spanhelper.FromContext(ctx, fmt.Sprintf("%s.Request", typeutils.GetTypeName(t.traced)))
+	operation := fmt.Sprintf("%s/%s.RegisterNSE", typeutils.GetPkgPath(t.traced), typeutils.GetTypeName(t.traced))
+	span := spanhelper.FromContext(ctx, operation)
 	defer span.Finish()
 
 	// Make sure we log to span
@@ -51,6 +53,9 @@ func (t *traceRegistryServer) RegisterNSE(ctx context.Context, request *registry
 	// Actually call the next
 	rv, err := t.traced.RegisterNSE(ctx, request)
 	if err != nil {
+		if _, ok := err.(stackTracer); !ok {
+			err = errors.Wrapf(err, "Error returned from %s/", operation)
+		}
 		span.LogError(err)
 		return nil, err
 	}

diff --git a/pkg/registry/core/trace/utils.go b/pkg/registry/core/trace/utils.go
@@ -0,0 +1,25 @@
+// Copyright (c) 2020 Cisco and/or its affiliates.
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+// 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 trace
+
+import (
+	"github.com/pkg/errors"
+)
+
+type stackTracer interface {
+	StackTrace() errors.StackTrace
+}
diff --git a/pkg/tools/typeutils/typeutils.go b/pkg/tools/typeutils/typeutils.go
@@ -29,3 +29,12 @@ func GetTypeName(value interface{}) string {
 	}
 	return t.Name()
 }
+
+// GetPkgPath return the package path of the underlying struct for an interface, with a * if a ptr
+func GetPkgPath(value interface{}) string {
+	t := reflect.TypeOf(value)
+	if t.Kind() == reflect.Ptr {
+		return "*" + t.Elem().Name()
+	}
+	return t.PkgPath()
+}