Illegal argument errors will no longer be logged at the ERROR level (#…

…25761)

For #25759 

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files)
for more information.
- [x] A detailed QA plan exists on the associated ticket (if it isn't
there, work with the product group's QA engineer to add it)
- [x] Manual QA for all new/changed functionality

changes/25759-illegal-argument-errors

@@ -0,0 +1 @@
+Illegal argument errors will no longer be logged at the ERROR level on the server. Since these are client errors, they will be logged at the DEBUG level instead. This will reduce the amount of noise in the server logs and help debugging other issues.

server/contexts/logging/logging.go

@@ -53,14 +53,6 @@ func WithNoUser(ctx context.Context) context.Context {
 	return ctx
 }
 
-// WithNoError returns a context with logging.SkipError set to true so error won't be logged at level=error
-func WithNoError(ctx context.Context) context.Context {
-	if logCtx, ok := FromContext(ctx); ok {
-		logCtx.SetSkipError()
-	}
-	return ctx
-}
-
 // WithExtras returns a context with logging.Extras set as the values provided
 func WithExtras(ctx context.Context, extras ...interface{}) context.Context {
 	if logCtx, ok := FromContext(ctx); ok {
@@ -87,7 +79,6 @@ type LoggingContext struct {
 	Extras     []interface{}
 	SkipUser   bool
 	ForceLevel func(kitlog.Logger) kitlog.Logger
-	SkipError  bool
 }
 
 func (l *LoggingContext) SetForceLevel(level func(kitlog.Logger) kitlog.Logger) {
@@ -108,12 +99,6 @@ func (l *LoggingContext) SetSkipUser() {
 	l.SkipUser = true
 }
 
-func (l *LoggingContext) SetSkipError() {
-	l.l.Lock()
-	defer l.l.Unlock()
-	l.SkipError = true
-}
-
 func (l *LoggingContext) SetStartTime() {
 	l.l.Lock()
 	defer l.l.Unlock()
@@ -132,7 +117,7 @@ func (l *LoggingContext) Log(ctx context.Context, logger kitlog.Logger) {
 	defer l.l.Unlock()
 
 	switch {
-	case len(l.Errs) > 0 && !l.SkipError:
+	case l.setLevelError():
 		logger = level.Error(logger)
 	case l.ForceLevel != nil:
 		logger = l.ForceLevel(logger)
@@ -208,3 +193,18 @@ func (l *LoggingContext) Log(ctx context.Context, logger kitlog.Logger) {
 
 	_ = logger.Log(keyvals...)
 }
+
+func (l *LoggingContext) setLevelError() bool {
+	if len(l.Errs) == 0 {
+		return false
+	}
+
+	if len(l.Errs) == 1 {
+		var ew fleet.ErrWithIsClientError
+		if errors.As(l.Errs[0], &ew) && ew.IsClientError() {
+			return false
+		}
+	}
+
+	return true
+}

server/docs/patterns.md

@@ -18,6 +18,8 @@ Validate API inputs and return a 4XX status code if invalid. If you did not do a
 
 Inputs corresponding to sortable or indexed DB fields should be preprocessed (trim spaces, normalize Unicode, etc.). Use utility method `fleet.Preprocess(input string) string`. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=4055688254267958899).
 
+Invalid inputs should NOT log a server error. Server errors should be reserved for unexpected/serious issues. [`InvalidArgumentError` implements `IsClientError`](https://github.com/fleetdm/fleet/blob/529f4ed725117d99d668318aad23c9e1575fa7ee/server/fleet/errors.go#L134) method to indicate that it is a client error. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=6515110653090875786&highlights=%5B%7B%22type%22%3A%22SHARE%22%2C%22from%22%3A340%2C%22to%22%3A1578%7D%5D).
+
 ### JSON unmarshaling
 
 `PATCH` API calls often need to distinguish between a field being set to `null` and a field not being present in the JSON. Use the structs from `optjson` package to handle this. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=4055688254267958899). [JSON unmarshaling article and example](https://victoronsoftware.com/posts/go-json-unmarshal/).

server/fleet/errors.go

@@ -63,6 +63,14 @@ type ErrWithRetryAfter interface {
 	RetryAfter() int
 }
 
+// ErrWithIsClientError is an interface for errors that explicitly specify
+// whether they are client errors or not. By default, errors are treated as
+// server errors.
+type ErrWithIsClientError interface {
+	error
+	IsClientError() bool
+}
+
 type invalidArgWithStatusError struct {
 	InvalidArgumentError
 	code int
@@ -102,7 +110,7 @@ func (e *ErrorWithUUID) UUID() string {
 }
 
 // InvalidArgumentError is the error returned when invalid data is presented to
-// a service method.
+// a service method. It is a client error.
 type InvalidArgumentError struct {
 	Errors []InvalidArgument
 
@@ -123,6 +131,10 @@ func NewInvalidArgumentError(name, reason string) *InvalidArgumentError {
 	return &invalid
 }
 
+func (e InvalidArgumentError) IsClientError() bool {
+	return true
+}
+
 func (e *InvalidArgumentError) Append(name, reason string) {
 	e.Errors = append(e.Errors, InvalidArgument{
 		name:   name,

server/service/apple_mdm.go

@@ -2378,8 +2378,6 @@ func bootstrapPackageMetadataEndpoint(ctx context.Context, request interface{},
 	meta, err := svc.GetMDMAppleBootstrapPackageMetadata(ctx, req.TeamID, req.ForUpdate)
 	switch {
 	case fleet.IsNotFound(err):
-		// Don't log this response as error -- it's expected to happen when the bootstrap package is missing, which is a common case.
-		logging.WithNoError(ctx)
 		return bootstrapPackageMetadataResponse{Err: fleet.NewInvalidArgumentError("team_id",
 			"bootstrap package for this team does not exist").WithStatus(http.StatusNotFound)}, nil
 	case err != nil: