Return log levels from admin.SetLoggerLevel (#2250)

StephenButtolph · web-flow · commit c174c6230528 · 2023-11-02T20:20:03.000Z
diff --git a/api/admin/client.go b/api/admin/client.go
@@ -25,7 +25,7 @@ type Client interface {
 	GetChainAliases(ctx context.Context, chainID string, options ...rpc.Option) ([]string, error)
 	Stacktrace(context.Context, ...rpc.Option) error
 	LoadVMs(context.Context, ...rpc.Option) (map[ids.ID][]string, map[ids.ID]string, error)
-	SetLoggerLevel(ctx context.Context, loggerName, logLevel, displayLevel string, options ...rpc.Option) error
+	SetLoggerLevel(ctx context.Context, loggerName, logLevel, displayLevel string, options ...rpc.Option) (map[string]LogAndDisplayLevels, error)
 	GetLoggerLevel(ctx context.Context, loggerName string, options ...rpc.Option) (map[string]LogAndDisplayLevels, error)
 	GetConfig(ctx context.Context, options ...rpc.Option) (interface{}, error)
 }
@@ -96,7 +96,7 @@ func (c *client) SetLoggerLevel(
 	logLevel,
 	displayLevel string,
 	options ...rpc.Option,
-) error {
+) (map[string]LogAndDisplayLevels, error) {
 	var (
 		logLevelArg     logging.Level
 		displayLevelArg logging.Level
@@ -105,28 +105,30 @@ func (c *client) SetLoggerLevel(
 	if len(logLevel) > 0 {
 		logLevelArg, err = logging.ToLevel(logLevel)
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
 	if len(displayLevel) > 0 {
 		displayLevelArg, err = logging.ToLevel(displayLevel)
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
-	return c.requester.SendRequest(ctx, "admin.setLoggerLevel", &SetLoggerLevelArgs{
+	res := &LoggerLevelReply{}
+	err = c.requester.SendRequest(ctx, "admin.setLoggerLevel", &SetLoggerLevelArgs{
 		LoggerName:   loggerName,
 		LogLevel:     &logLevelArg,
 		DisplayLevel: &displayLevelArg,
-	}, &api.EmptyReply{}, options...)
+	}, res, options...)
+	return res.LoggerLevels, err
 }
 
 func (c *client) GetLoggerLevel(
 	ctx context.Context,
 	loggerName string,
 	options ...rpc.Option,
 ) (map[string]LogAndDisplayLevels, error) {
-	res := &GetLoggerLevelReply{}
+	res := &LoggerLevelReply{}
 	err := c.requester.SendRequest(ctx, "admin.getLoggerLevel", &GetLoggerLevelArgs{
 		LoggerName: loggerName,
 	}, res, options...)
diff --git a/api/admin/client_test.go b/api/admin/client_test.go
@@ -63,8 +63,8 @@ func (mc *mockClient) SendRequest(_ context.Context, _ string, _ interface{}, re
 	case *LoadVMsReply:
 		response := mc.response.(*LoadVMsReply)
 		*p = *response
-	case *GetLoggerLevelReply:
-		response := mc.response.(*GetLoggerLevelReply)
+	case *LoggerLevelReply:
+		response := mc.response.(*LoggerLevelReply)
 		*p = *response
 	case *interface{}:
 		response := mc.response.(*interface{})
@@ -212,54 +212,72 @@ func TestReloadInstalledVMs(t *testing.T) {
 
 func TestSetLoggerLevel(t *testing.T) {
 	type test struct {
-		name         string
-		logLevel     string
-		displayLevel string
-		serviceErr   error
-		clientErr    error
+		name            string
+		logLevel        string
+		displayLevel    string
+		serviceResponse map[string]LogAndDisplayLevels
+		serviceErr      error
+		clientErr       error
 	}
 	tests := []test{
 		{
 			name:         "Happy path",
 			logLevel:     "INFO",
 			displayLevel: "INFO",
-			serviceErr:   nil,
-			clientErr:    nil,
+			serviceResponse: map[string]LogAndDisplayLevels{
+				"Happy path": {LogLevel: logging.Info, DisplayLevel: logging.Info},
+			},
+			serviceErr: nil,
+			clientErr:  nil,
 		},
 		{
-			name:         "Service errors",
-			logLevel:     "INFO",
-			displayLevel: "INFO",
-			serviceErr:   errTest,
-			clientErr:    errTest,
+			name:            "Service errors",
+			logLevel:        "INFO",
+			displayLevel:    "INFO",
+			serviceResponse: nil,
+			serviceErr:      errTest,
+			clientErr:       errTest,
 		},
 		{
-			name:         "Invalid log level",
-			logLevel:     "invalid",
-			displayLevel: "INFO",
-			serviceErr:   nil,
-			clientErr:    logging.ErrUnknownLevel,
+			name:            "Invalid log level",
+			logLevel:        "invalid",
+			displayLevel:    "INFO",
+			serviceResponse: nil,
+			serviceErr:      nil,
+			clientErr:       logging.ErrUnknownLevel,
 		},
 		{
-			name:         "Invalid display level",
-			logLevel:     "INFO",
-			displayLevel: "invalid",
-			serviceErr:   nil,
-			clientErr:    logging.ErrUnknownLevel,
+			name:            "Invalid display level",
+			logLevel:        "INFO",
+			displayLevel:    "invalid",
+			serviceResponse: nil,
+			serviceErr:      nil,
+			clientErr:       logging.ErrUnknownLevel,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			require := require.New(t)
+
 			c := client{
-				requester: NewMockClient(&api.EmptyReply{}, tt.serviceErr),
+				requester: NewMockClient(
+					&LoggerLevelReply{
+						LoggerLevels: tt.serviceResponse,
+					},
+					tt.serviceErr,
+				),
 			}
-			err := c.SetLoggerLevel(
+			res, err := c.SetLoggerLevel(
 				context.Background(),
 				"",
 				tt.logLevel,
 				tt.displayLevel,
 			)
-			require.ErrorIs(t, err, tt.clientErr)
+			require.ErrorIs(err, tt.clientErr)
+			if tt.clientErr != nil {
+				return
+			}
+			require.Equal(tt.serviceResponse, res)
 		})
 	}
 }
@@ -296,7 +314,7 @@ func TestGetLoggerLevel(t *testing.T) {
 
 			c := client{
 				requester: NewMockClient(
-					&GetLoggerLevelReply{
+					&LoggerLevelReply{
 						LoggerLevels: tt.serviceResponse,
 					},
 					tt.serviceErr,
diff --git a/api/admin/service.go b/api/admin/service.go
@@ -224,13 +224,21 @@ func (a *Admin) Stacktrace(_ *http.Request, _ *struct{}, _ *api.EmptyReply) erro
 	return perms.WriteFile(stacktraceFile, stacktrace, perms.ReadWrite)
 }
 
-// See SetLoggerLevel
 type SetLoggerLevelArgs struct {
 	LoggerName   string         `json:"loggerName"`
 	LogLevel     *logging.Level `json:"logLevel"`
 	DisplayLevel *logging.Level `json:"displayLevel"`
 }
 
+type LogAndDisplayLevels struct {
+	LogLevel     logging.Level `json:"logLevel"`
+	DisplayLevel logging.Level `json:"displayLevel"`
+}
+
+type LoggerLevelReply struct {
+	LoggerLevels map[string]LogAndDisplayLevels `json:"loggerLevels"`
+}
+
 // SetLoggerLevel sets the log level and/or display level for loggers.
 // If len([args.LoggerName]) == 0, sets the log/display level of all loggers.
 // Otherwise, sets the log/display level of the loggers named in that argument.
@@ -240,7 +248,7 @@ type SetLoggerLevelArgs struct {
 // Sets the display level of these loggers to args.LogLevel.
 // If args.DisplayLevel == nil, doesn't set the display level of these loggers.
 // If args.DisplayLevel != nil, must be a valid string representation of a log level.
-func (a *Admin) SetLoggerLevel(_ *http.Request, args *SetLoggerLevelArgs, _ *api.EmptyReply) error {
+func (a *Admin) SetLoggerLevel(_ *http.Request, args *SetLoggerLevelArgs, reply *LoggerLevelReply) error {
 	a.Log.Debug("API called",
 		zap.String("service", "admin"),
 		zap.String("method", "setLoggerLevel"),
@@ -256,14 +264,7 @@ func (a *Admin) SetLoggerLevel(_ *http.Request, args *SetLoggerLevelArgs, _ *api
 	a.lock.Lock()
 	defer a.lock.Unlock()
 
-	var loggerNames []string
-	if len(args.LoggerName) > 0 {
-		loggerNames = []string{args.LoggerName}
-	} else {
-		// Empty name means all loggers
-		loggerNames = a.LogFactory.GetLoggerNames()
-	}
-
+	loggerNames := a.getLoggerNames(args.LoggerName)
 	for _, name := range loggerNames {
 		if args.LogLevel != nil {
 			if err := a.LogFactory.SetLogLevel(name, *args.LogLevel); err != nil {
@@ -276,26 +277,18 @@ func (a *Admin) SetLoggerLevel(_ *http.Request, args *SetLoggerLevelArgs, _ *api
 			}
 		}
 	}
-	return nil
-}
 
-type LogAndDisplayLevels struct {
-	LogLevel     logging.Level `json:"logLevel"`
-	DisplayLevel logging.Level `json:"displayLevel"`
+	var err error
+	reply.LoggerLevels, err = a.getLogLevels(loggerNames)
+	return err
 }
 
-// See GetLoggerLevel
 type GetLoggerLevelArgs struct {
 	LoggerName string `json:"loggerName"`
 }
 
-// See GetLoggerLevel
-type GetLoggerLevelReply struct {
-	LoggerLevels map[string]LogAndDisplayLevels `json:"loggerLevels"`
-}
-
 // GetLogLevel returns the log level and display level of all loggers.
-func (a *Admin) GetLoggerLevel(_ *http.Request, args *GetLoggerLevelArgs, reply *GetLoggerLevelReply) error {
+func (a *Admin) GetLoggerLevel(_ *http.Request, args *GetLoggerLevelArgs, reply *LoggerLevelReply) error {
 	a.Log.Debug("API called",
 		zap.String("service", "admin"),
 		zap.String("method", "getLoggerLevels"),
@@ -305,30 +298,11 @@ func (a *Admin) GetLoggerLevel(_ *http.Request, args *GetLoggerLevelArgs, reply
 	a.lock.RLock()
 	defer a.lock.RUnlock()
 
-	reply.LoggerLevels = make(map[string]LogAndDisplayLevels)
-	var loggerNames []string
-	// Empty name means all loggers
-	if len(args.LoggerName) > 0 {
-		loggerNames = []string{args.LoggerName}
-	} else {
-		loggerNames = a.LogFactory.GetLoggerNames()
-	}
+	loggerNames := a.getLoggerNames(args.LoggerName)
 
-	for _, name := range loggerNames {
-		logLevel, err := a.LogFactory.GetLogLevel(name)
-		if err != nil {
-			return err
-		}
-		displayLevel, err := a.LogFactory.GetDisplayLevel(name)
-		if err != nil {
-			return err
-		}
-		reply.LoggerLevels[name] = LogAndDisplayLevels{
-			LogLevel:     logLevel,
-			DisplayLevel: displayLevel,
-		}
-	}
-	return nil
+	var err error
+	reply.LoggerLevels, err = a.getLogLevels(loggerNames)
+	return err
 }
 
 // GetConfig returns the config that the node was started with.
@@ -375,3 +349,30 @@ func (a *Admin) LoadVMs(r *http.Request, _ *struct{}, reply *LoadVMsReply) error
 	reply.NewVMs, err = ids.GetRelevantAliases(a.VMManager, loadedVMs)
 	return err
 }
+
+func (a *Admin) getLoggerNames(loggerName string) []string {
+	if len(loggerName) == 0 {
+		// Empty name means all loggers
+		return a.LogFactory.GetLoggerNames()
+	}
+	return []string{loggerName}
+}
+
+func (a *Admin) getLogLevels(loggerNames []string) (map[string]LogAndDisplayLevels, error) {
+	loggerLevels := make(map[string]LogAndDisplayLevels)
+	for _, name := range loggerNames {
+		logLevel, err := a.LogFactory.GetLogLevel(name)
+		if err != nil {
+			return nil, err
+		}
+		displayLevel, err := a.LogFactory.GetDisplayLevel(name)
+		if err != nil {
+			return nil, err
+		}
+		loggerLevels[name] = LogAndDisplayLevels{
+			LogLevel:     logLevel,
+			DisplayLevel: displayLevel,
+		}
+	}
+	return loggerLevels, nil
+}
