Move wrapping of errors for stacktraces into the trace package (#174)

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 · a3fe213 · a3fe213
1 parent fd5d675
commit a3fe213
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()
+}