logging: cleanup errdict usage (istio#46011)

For istio#44683 Partially reverts istio/pkg#192 We current have an `errdict` mechanism, which is basically just a struct of k/v pairs to insert into logs. We made this builtin to our logging system. This adds a lot of complexity, and I don't think its needed. Instead, just add a helper to the errdict to make it add labels to the logger we want, and strip out all of the first-class support in `pkg/log`. This change moves us closer to being able to just swap `slog` in
WillardHu · Jul 15, 2023 · 75671b8 · 75671b8
1 parent d07f753
commit 75671b8
Show file tree

Hide file tree

Showing 13 changed files with 144 additions and 370 deletions.
diff --git a/cni/pkg/ambient/net_linux.go b/cni/pkg/ambient/net_linux.go
@@ -777,7 +777,7 @@ func (s *Server) CreateRulesOnNode(ztunnelVeth, ztunnelIP string, captureDNS boo
 	for _, route := range routes {
 		err = execute(route.Cmd, route.Args...)
 		if err != nil {
-			log.Errorf(fmt.Errorf("failed to add route (%+v): %v", route, err))
+			log.Errorf("failed to add route (%+v): %v", route, err)
 		}
 	}
 

diff --git a/operator/pkg/controller/istiocontrolplane/istiocontrolplane_controller.go b/operator/pkg/controller/istiocontrolplane/istiocontrolplane_controller.go
@@ -178,12 +178,12 @@ var (
 		UpdateFunc: func(e event.UpdateEvent) bool {
 			oldIOP, ok := e.ObjectOld.(*iopv1alpha1.IstioOperator)
 			if !ok {
-				scope.Errorf(errdict.OperatorFailedToGetObjectInCallback, "failed to get old IstioOperator")
+				errdict.OperatorFailedToGetObjectInCallback.Log(scope).Errorf("failed to get old IstioOperator")
 				return false
 			}
 			newIOP, ok := e.ObjectNew.(*iopv1alpha1.IstioOperator)
 			if !ok {
-				scope.Errorf(errdict.OperatorFailedToGetObjectInCallback, "failed to get new IstioOperator")
+				errdict.OperatorFailedToGetObjectInCallback.Log(scope).Errorf("failed to get new IstioOperator")
 				return false
 			}
 
@@ -261,7 +261,7 @@ func (r *ReconcileIstioOperator) Reconcile(_ context.Context, request reconcile.
 			return reconcile.Result{}, nil
 		}
 		// Error reading the object - requeue the request.
-		scope.Warnf(errdict.OperatorFailedToGetObjectFromAPIServer, "error getting IstioOperator %s: %s", iopName, err)
+		errdict.OperatorFailedToGetObjectFromAPIServer.Log(scope).Warnf("error getting IstioOperator %s: %s", iopName, err)
 		metrics.CountCRFetchFail(errors.ReasonForError(err))
 		return reconcile.Result{}, err
 	}
@@ -286,7 +286,7 @@ func (r *ReconcileIstioOperator) Reconcile(_ context.Context, request reconcile.
 	// get the merged values in iop on top of the defaults for the profile given by iop.profile
 	iopMerged.Spec, err = mergeIOPSWithProfile(iopMerged)
 	if err != nil {
-		scope.Errorf(errdict.OperatorFailedToMergeUserIOP, "failed to merge base profile with user IstioOperator CR %s, %s", iopName, err)
+		errdict.OperatorFailedToMergeUserIOP.Log(scope).Errorf("failed to merge base profile with user IstioOperator CR %s, %s", iopName, err)
 		return reconcile.Result{}, err
 	}
 
@@ -329,7 +329,7 @@ func (r *ReconcileIstioOperator) Reconcile(_ context.Context, request reconcile.
 				scope.Infof("Could not remove finalizer from %s due to conflict. Operation will be retried in next reconcile attempt.", iopName)
 				return reconcile.Result{}, nil
 			}
-			scope.Errorf(errdict.OperatorFailedToRemoveFinalizer, "error removing finalizer: %s", finalizerError)
+			errdict.OperatorFailedToRemoveFinalizer.Log(scope).Errorf("error removing finalizer: %s", finalizerError)
 			return reconcile.Result{}, finalizerError
 		}
 		return reconcile.Result{}, nil
@@ -346,7 +346,7 @@ func (r *ReconcileIstioOperator) Reconcile(_ context.Context, request reconcile.
 			} else if errors.IsConflict(err) {
 				scope.Infof("Could not add finalizer to %s due to conflict. Operation will be retried in next reconcile attempt.", iopName)
 			}
-			scope.Errorf(errdict.OperatorFailedToAddFinalizer, "Failed to add finalizer to IstioOperator CR %s: %s", iopName, err)
+			errdict.OperatorFailedToAddFinalizer.Log(scope).Errorf("Failed to add finalizer to IstioOperator CR %s: %s", iopName, err)
 			return reconcile.Result{}, err
 		}
 	}
@@ -372,7 +372,7 @@ func (r *ReconcileIstioOperator) Reconcile(_ context.Context, request reconcile.
 	}
 	err = util.ValidateIOPCAConfig(r.kubeClient, iopMerged)
 	if err != nil {
-		scope.Errorf(errdict.OperatorFailedToConfigure, "failed to apply IstioOperator resources. Error %s", err)
+		errdict.OperatorFailedToConfigure.Log(scope).Errorf("failed to apply IstioOperator resources. Error %s", err)
 		return reconcile.Result{}, err
 	}
 	helmReconcilerOptions := &helmreconciler.Options{

diff --git a/pkg/log/default.go b/pkg/log/default.go
@@ -23,13 +23,13 @@ func registerDefaultScope() *Scope {
 var defaultScope = registerDefaultScope()
 
 // Fatal outputs a message at fatal level.
-func Fatal(fields ...any) {
-	defaultScope.Fatal(fields...)
+func Fatal(fields any) {
+	defaultScope.Fatal(fields)
 }
 
 // Fatalf uses fmt.Sprintf to construct and log a message at fatal level.
-func Fatalf(args ...any) {
-	defaultScope.Fatalf(args...)
+func Fatalf(format string, args ...any) {
+	defaultScope.Fatalf(format, args...)
 }
 
 // FatalEnabled returns whether output of messages using this scope is currently enabled for fatal-level output.
@@ -43,8 +43,8 @@ func Error(fields any) {
 }
 
 // Errorf uses fmt.Sprintf to construct and log a message at error level.
-func Errorf(args ...any) {
-	defaultScope.Errorf(args...)
+func Errorf(format string, args ...any) {
+	defaultScope.Errorf(format, args...)
 }
 
 // ErrorEnabled returns whether output of messages using this scope is currently enabled for error-level output.
@@ -58,8 +58,8 @@ func Warn(fields any) {
 }
 
 // Warnf uses fmt.Sprintf to construct and log a message at warn level.
-func Warnf(args ...any) {
-	defaultScope.Warnf(args...)
+func Warnf(format string, args ...any) {
+	defaultScope.Warnf(format, args...)
 }
 
 // WarnEnabled returns whether output of messages using this scope is currently enabled for warn-level output.
@@ -73,8 +73,8 @@ func Info(fields any) {
 }
 
 // Infof uses fmt.Sprintf to construct and log a message at info level.
-func Infof(args ...any) {
-	defaultScope.Infof(args...)
+func Infof(format string, args ...any) {
+	defaultScope.Infof(format, args...)
 }
 
 // InfoEnabled returns whether output of messages using this scope is currently enabled for info-level output.
@@ -88,8 +88,8 @@ func Debug(fields any) {
 }
 
 // Debugf uses fmt.Sprintf to construct and log a message at debug level.
-func Debugf(args ...any) {
-	defaultScope.Debugf(args...)
+func Debugf(format string, args ...any) {
+	defaultScope.Debugf(format, args...)
 }
 
 // DebugEnabled returns whether output of messages using this scope is currently enabled for debug-level output.

diff --git a/pkg/log/default_test.go b/pkg/log/default_test.go
@@ -18,8 +18,6 @@ import (
 	"regexp"
 	"strconv"
 	"testing"
-
-	"istio.io/istio/pkg/structured"
 )
 
 func testOptions() *Options {
@@ -195,31 +193,6 @@ func TestDefault(t *testing.T) {
 	}
 }
 
-func TestErrorDictionary(t *testing.T) {
-	ie := &structured.Error{
-		MoreInfo:    "MoreInfo",
-		Impact:      "Impact",
-		Action:      "Action",
-		LikelyCause: "LikelyCause",
-	}
-	lines, err := captureStdout(func() {
-		if err := Configure(DefaultOptions()); err != nil {
-			t.Errorf("Got err '%v', expecting success", err)
-		}
-
-		Infof(ie, "Hello")
-		_ = Sync()
-	})
-	if err != nil {
-		t.Errorf("Got error '%v', expected success", err)
-	}
-
-	expected := "Hello	moreInfo=MoreInfo impact=Impact action=Action likelyCause=LikelyCause"
-	if match, _ := regexp.MatchString(expected, lines[0]); !match {
-		t.Errorf("Got '%v', expected a match with '%v'", lines[0], expected)
-	}
-}
-
 func TestEnabled(t *testing.T) {
 	cases := []struct {
 		level        Level