Make tracing interface to coalesce logging/tracing (#519)

* Define and use an interface for logging. This allows alternative implementations to use whatever logging package they want. Currently the interface just mimicks what logrus already implements, with minor modifications to not rely on logrus itself. I think the interface is pretty solid in terms of logging implementations being able to do what they need to. * Make tracing interface to coalesce logging/tracing Allows us to share data between the tracer and the logger so we can simplify log/trace handling wher we generally want data to go both places.
2019-02-22 11:36:03 -08:00
parent bd103a43bd
commit 1bfffa975e
18 changed files with 605 additions and 188 deletions
--- a/vkubelet/api/helpers.go
+++ b/vkubelet/api/helpers.go
@@ -25,7 +25,7 @@ func handleError(f handlerFunc) http.HandlerFunc {
 		if code >= 500 {
 			logger.Error("Internal server error on request")
 		} else {
-			log.Trace(logger, "Error on request")
+			logger.Debug("Error on request")
 		}
 	}
 }
--- a/vkubelet/apiserver.go
+++ b/vkubelet/apiserver.go
@@ -3,7 +3,6 @@ package vkubelet
 import (
 	"net/http"

-	"github.com/Sirupsen/logrus"
 	"github.com/gorilla/mux"
 	"github.com/virtual-kubelet/virtual-kubelet/log"
 	"github.com/virtual-kubelet/virtual-kubelet/providers"
@@ -73,7 +72,7 @@ func AttachMetricsRoutes(p providers.Provider, mux ServeMux) {

 func instrumentRequest(r *http.Request) *http.Request {
 	ctx := r.Context()
-	logger := log.G(ctx).WithFields(logrus.Fields{
+	logger := log.G(ctx).WithFields(log.Fields{
 		"uri":  r.RequestURI,
 		"vars": mux.Vars(r),
 	})
@@ -96,14 +95,12 @@ func InstrumentHandler(h http.Handler) http.Handler {

 // NotFound provides a handler for cases where the requested endpoint doesn't exist
 func NotFound(w http.ResponseWriter, r *http.Request) {
-	logger := log.G(r.Context())
-	log.Trace(logger, "404 request not found")
+	log.G(r.Context()).Debug("404 request not found")
 	http.Error(w, "404 request not found", http.StatusNotFound)
 }

 // NotImplemented provides a handler for cases where a provider does not implement a given API
 func NotImplemented(w http.ResponseWriter, r *http.Request) {
-	logger := log.G(r.Context())
-	log.Trace(logger, "501 not implemented")
+	log.G(r.Context()).Debug("501 not implemented")
 	http.Error(w, "501 not implemented", http.StatusNotImplemented)
 }
--- a/vkubelet/node.go
+++ b/vkubelet/node.go
@@ -4,10 +4,10 @@ import (
 	"context"
 	"strings"

+	"github.com/cpuguy83/strongerrors/status/ocstatus"
 	"github.com/virtual-kubelet/virtual-kubelet/log"
+	"github.com/virtual-kubelet/virtual-kubelet/trace"
 	"github.com/virtual-kubelet/virtual-kubelet/version"
-
-	"go.opencensus.io/trace"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -57,12 +57,11 @@ func (s *Server) registerNode(ctx context.Context) error {
 			DaemonEndpoints: *s.provider.NodeDaemonEndpoints(ctx),
 		},
 	}
-	addNodeAttributes(span, node)
+	ctx = addNodeAttributes(ctx, span, node)
 	if _, err := s.k8sClient.CoreV1().Nodes().Create(node); err != nil && !errors.IsAlreadyExists(err) {
-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: err.Error()})
+		span.SetStatus(ocstatus.FromError(err))
 		return err
 	}
-	span.Annotate(nil, "Registered node with k8s")

 	log.G(ctx).Info("Registered node")

@@ -77,19 +76,20 @@ func (s *Server) updateNode(ctx context.Context) {
 	opts := metav1.GetOptions{}
 	n, err := s.k8sClient.CoreV1().Nodes().Get(s.nodeName, opts)
 	if err != nil && !errors.IsNotFound(err) {
-		log.G(ctx).WithError(err).Error("Failed to retrieve node")
-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: err.Error()})
+		log.G(ctx).WithError(err).Error("Failed to retrive node")
+		span.SetStatus(ocstatus.FromError(err))
 		return
 	}
-	addNodeAttributes(span, n)
-	span.Annotate(nil, "Fetched node details from k8s")
+
+	ctx = addNodeAttributes(ctx, span, n)
+	log.G(ctx).Debug("Fetched node details from k8s")

 	if errors.IsNotFound(err) {
 		if err = s.registerNode(ctx); err != nil {
 			log.G(ctx).WithError(err).Error("Failed to register node")
-			span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: err.Error()})
+			span.SetStatus(ocstatus.FromError(err))
 		} else {
-			span.Annotate(nil, "Registered node in k8s")
+			log.G(ctx).Debug("Registered node in k8s")
 		}
 		return
 	}
@@ -106,7 +106,7 @@ func (s *Server) updateNode(ctx context.Context) {
 	n, err = s.k8sClient.CoreV1().Nodes().UpdateStatus(n)
 	if err != nil {
 		log.G(ctx).WithError(err).Error("Failed to update node")
-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: err.Error()})
+		span.SetStatus(ocstatus.FromError(err))
 		return
 	}
 }
@@ -125,13 +125,11 @@ func (t taintsStringer) String() string {
 	return s
 }

-func addNodeAttributes(span *trace.Span, n *corev1.Node) {
-	span.AddAttributes(
-		trace.StringAttribute("UID", string(n.UID)),
-		trace.StringAttribute("name", n.Name),
-		trace.StringAttribute("cluster", n.ClusterName),
-	)
-	if span.IsRecordingEvents() {
-		span.AddAttributes(trace.StringAttribute("taints", taintsStringer(n.Spec.Taints).String()))
-	}
+func addNodeAttributes(ctx context.Context, span trace.Span, n *corev1.Node) context.Context {
+	return span.WithFields(ctx, log.Fields{
+		"UID":     string(n.UID),
+		"name":    n.Name,
+		"cluster": n.ClusterName,
+		"tains":   taintsStringer(n.Spec.Taints),
+	})
 }
--- a/vkubelet/pod.go
+++ b/vkubelet/pod.go
@@ -2,29 +2,27 @@ package vkubelet

 import (
 	"context"
-	"fmt"
 	"sync"
 	"time"

 	"github.com/cpuguy83/strongerrors/status/ocstatus"
 	pkgerrors "github.com/pkg/errors"
-	"go.opencensus.io/trace"
+	"github.com/virtual-kubelet/virtual-kubelet/log"
+	"github.com/virtual-kubelet/virtual-kubelet/trace"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
-
-	"github.com/virtual-kubelet/virtual-kubelet/log"
 )

-func addPodAttributes(span *trace.Span, pod *corev1.Pod) {
-	span.AddAttributes(
-		trace.StringAttribute("uid", string(pod.GetUID())),
-		trace.StringAttribute("namespace", pod.GetNamespace()),
-		trace.StringAttribute("name", pod.GetName()),
-		trace.StringAttribute("phase", string(pod.Status.Phase)),
-		trace.StringAttribute("reason", pod.Status.Reason),
-	)
+func addPodAttributes(ctx context.Context, span trace.Span, pod *corev1.Pod) context.Context {
+	return span.WithFields(ctx, log.Fields{
+		"uid":       string(pod.GetUID()),
+		"namespace": pod.GetNamespace(),
+		"name":      pod.GetName(),
+		"phase":     string(pod.Status.Phase),
+		"reason":    pod.Status.Reason,
+	})
 }

 func (s *Server) createOrUpdatePod(ctx context.Context, pod *corev1.Pod, recorder record.EventRecorder) error {
@@ -40,14 +38,17 @@ func (s *Server) createOrUpdatePod(ctx context.Context, pod *corev1.Pod, recorde

 	ctx, span := trace.StartSpan(ctx, "createOrUpdatePod")
 	defer span.End()
-	addPodAttributes(span, pod)
+	addPodAttributes(ctx, span, pod)

 	if err := populateEnvironmentVariables(ctx, pod, s.resourceManager, recorder); err != nil {
-		span.SetStatus(trace.Status{Code: trace.StatusCodeInvalidArgument, Message: err.Error()})
+		span.SetStatus(ocstatus.FromError(err))
 		return err
 	}

-	logger := log.G(ctx).WithField("pod", pod.GetName()).WithField("namespace", pod.GetNamespace())
+	ctx = span.WithFields(ctx, log.Fields{
+		"pod":       pod.GetName(),
+		"namespace": pod.GetNamespace(),
+	})

 	if origErr := s.provider.CreatePod(ctx, pod); origErr != nil {
 		podPhase := corev1.PodPending
@@ -60,19 +61,23 @@ func (s *Server) createOrUpdatePod(ctx context.Context, pod *corev1.Pod, recorde
 		pod.Status.Reason = podStatusReasonProviderFailed
 		pod.Status.Message = origErr.Error()

+		logger := log.G(ctx).WithFields(log.Fields{
+			"podPhase": podPhase,
+			"reason":   pod.Status.Reason,
+		})
+
 		_, err := s.k8sClient.CoreV1().Pods(pod.Namespace).UpdateStatus(pod)
 		if err != nil {
 			logger.WithError(err).Warn("Failed to update pod status")
 		} else {
-			span.Annotate(nil, "Updated k8s pod status")
+			logger.Info("Updated k8s pod status")
 		}

-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: origErr.Error()})
+		span.SetStatus(ocstatus.FromError(origErr))
 		return origErr
 	}
-	span.Annotate(nil, "Created pod in provider")

-	logger.Info("Pod created")
+	log.G(ctx).Info("Created pod in provider")

 	return nil
 }
@@ -89,23 +94,22 @@ func (s *Server) deletePod(ctx context.Context, namespace, name string) error {

 	ctx, span := trace.StartSpan(ctx, "deletePod")
 	defer span.End()
-	addPodAttributes(span, pod)
+	ctx = addPodAttributes(ctx, span, pod)

 	var delErr error
 	if delErr = s.provider.DeletePod(ctx, pod); delErr != nil && errors.IsNotFound(delErr) {
-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: delErr.Error()})
+		span.SetStatus(ocstatus.FromError(delErr))
 		return delErr
 	}
-	span.Annotate(nil, "Deleted pod from provider")

-	logger := log.G(ctx).WithField("pod", pod.GetName()).WithField("namespace", pod.GetNamespace())
+	log.G(ctx).Debug("Deleted pod from provider")
+
 	if !errors.IsNotFound(delErr) {
 		if err := s.forceDeletePodResource(ctx, namespace, name); err != nil {
 			span.SetStatus(ocstatus.FromError(err))
 			return err
 		}
-		span.Annotate(nil, "Deleted pod from k8s")
-		logger.Info("Pod deleted")
+		log.G(ctx).Info("Deleted pod from Kubernetes")
 	}

 	return nil
@@ -114,19 +118,19 @@ func (s *Server) deletePod(ctx context.Context, namespace, name string) error {
 func (s *Server) forceDeletePodResource(ctx context.Context, namespace, name string) error {
 	ctx, span := trace.StartSpan(ctx, "forceDeletePodResource")
 	defer span.End()
-	span.AddAttributes(
-		trace.StringAttribute("namespace", namespace),
-		trace.StringAttribute("name", name),
-	)
+	ctx = span.WithFields(ctx, log.Fields{
+		"namespace": namespace,
+		"name":      name,
+	})

 	var grace int64
 	if err := s.k8sClient.CoreV1().Pods(namespace).Delete(name, &metav1.DeleteOptions{GracePeriodSeconds: &grace}); err != nil {
 		if errors.IsNotFound(err) {
-			span.Annotate(nil, "Pod does not exist in Kubernetes, nothing to delete")
+			log.G(ctx).Debug("Pod does not exist in Kubernetes, nothing to delete")
 			return nil
 		}
-		span.SetStatus(trace.Status{Code: trace.StatusCodeUnknown, Message: err.Error()})
-		return fmt.Errorf("Failed to delete Kubernetes pod: %s", err)
+		span.SetStatus(ocstatus.FromError(err))
+		return pkgerrors.Wrap(err, "Failed to delete Kubernetes pod")
 	}
 	return nil
 }
@@ -138,7 +142,8 @@ func (s *Server) updatePodStatuses(ctx context.Context) {

 	// Update all the pods with the provider status.
 	pods := s.resourceManager.GetPods()
-	span.AddAttributes(trace.Int64Attribute("nPods", int64(len(pods))))
+
+	ctx = span.WithField(ctx, "nPods", int64(len(pods)))

 	sema := make(chan struct{}, s.podSyncWorkers)
 	var wg sync.WaitGroup
@@ -157,8 +162,12 @@ func (s *Server) updatePodStatuses(ctx context.Context) {
 			defer func() { <-sema }()

 			if err := s.updatePodStatus(ctx, pod); err != nil {
-				logger := log.G(ctx).WithField("pod", pod.GetName()).WithField("namespace", pod.GetNamespace()).WithField("status", pod.Status.Phase).WithField("reason", pod.Status.Reason)
-				logger.Error(err)
+				log.G(ctx).WithFields(log.Fields{
+					"pod":       pod.GetName(),
+					"namespace": pod.GetNamespace(),
+					"status":    pod.Status.Phase,
+					"reason":    pod.Status.Reason,
+				}).Error(err)
 			}

 		}(pod)
@@ -170,7 +179,7 @@ func (s *Server) updatePodStatuses(ctx context.Context) {
 func (s *Server) updatePodStatus(ctx context.Context, pod *corev1.Pod) error {
 	ctx, span := trace.StartSpan(ctx, "updatePodStatus")
 	defer span.End()
-	addPodAttributes(span, pod)
+	ctx = addPodAttributes(ctx, span, pod)

 	if pod.Status.Phase == corev1.PodSucceeded ||
 		pod.Status.Phase == corev1.PodFailed ||
@@ -214,9 +223,10 @@ func (s *Server) updatePodStatus(ctx context.Context, pod *corev1.Pod) error {
 		return pkgerrors.Wrap(err, "error while updating pod status in kubernetes")
 	}

-	span.Annotate([]trace.Attribute{
-		trace.StringAttribute("new phase", string(pod.Status.Phase)),
-		trace.StringAttribute("new reason", pod.Status.Reason),
-	}, "updated pod status in kubernetes")
+	log.G(ctx).WithFields(log.Fields{
+		"new phase":  string(pod.Status.Phase),
+		"new reason": pod.Status.Reason,
+	}).Debug("Updated pod status in kubernetes")
+
 	return nil
 }
--- a/vkubelet/podcontroller.go
+++ b/vkubelet/podcontroller.go
@@ -24,11 +24,11 @@ import (

 	"github.com/cpuguy83/strongerrors/status/ocstatus"
 	pkgerrors "github.com/pkg/errors"
-	"go.opencensus.io/trace"
+	"github.com/virtual-kubelet/virtual-kubelet/trace"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/client-go/informers/core/v1"
+	v1 "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 	typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	corev1listers "k8s.io/client-go/listers/core/v1"
@@ -169,7 +169,7 @@ func (pc *PodController) processNextWorkItem(ctx context.Context, workerId strin
 	defer span.End()

 	// Add the ID of the current worker as an attribute to the current span.
-	span.AddAttributes(trace.StringAttribute("workerId", workerId))
+	ctx = span.WithField(ctx, "workerId", workerId)

 	// We wrap this block in a func so we can defer pc.workqueue.Done.
 	err := func(obj interface{}) error {
@@ -190,7 +190,7 @@ func (pc *PodController) processNextWorkItem(ctx context.Context, workerId strin
 			return nil
 		}
 		// Add the current key as an attribute to the current span.
-		span.AddAttributes(trace.StringAttribute("key", key))
+		ctx = span.WithField(ctx, "key", key)
 		// Run the syncHandler, passing it the namespace/name string of the Pod resource to be synced.
 		if err := pc.syncHandler(ctx, key); err != nil {
 			if pc.workqueue.NumRequeues(key) < maxRetries {
@@ -224,7 +224,7 @@ func (pc *PodController) syncHandler(ctx context.Context, key string) error {
 	defer span.End()

 	// Add the current key as an attribute to the current span.
-	span.AddAttributes(trace.StringAttribute("key", key))
+	ctx = span.WithField(ctx, "key", key)

 	// Convert the namespace/name string into a distinct namespace and name.
 	namespace, name, err := cache.SplitMetaNamespaceKey(key)
@@ -263,7 +263,7 @@ func (pc *PodController) syncPodInProvider(ctx context.Context, pod *corev1.Pod)
 	defer span.End()

 	// Add the pod's attributes to the current span.
-	addPodAttributes(span, pod)
+	ctx = addPodAttributes(ctx, span, pod)

 	// Check whether the pod has been marked for deletion.
 	// If it does, guarantee it is deleted in the provider and Kubernetes.
@@ -344,7 +344,7 @@ func (pc *PodController) deleteDanglingPods(ctx context.Context, threadiness int
 			}()

 			// Add the pod's attributes to the current span.
-			addPodAttributes(span, pod)
+			ctx = addPodAttributes(ctx, span, pod)
 			// Actually delete the pod.
 			if err := pc.server.deletePod(ctx, pod.Namespace, pod.Name); err != nil {
 				span.SetStatus(ocstatus.FromError(err))