diff --git a/changes/25759-illegal-argument-errors b/changes/25759-illegal-argument-errors new file mode 100644 index 000000000000..f9b6ca55ed95 --- /dev/null +++ b/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. diff --git a/server/contexts/logging/logging.go b/server/contexts/logging/logging.go index 056f6a81ad87..846e7d23a629 100644 --- a/server/contexts/logging/logging.go +++ b/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 +} diff --git a/server/docs/patterns.md b/server/docs/patterns.md index ef71aca09ffe..0ac33d423f0c 100644 --- a/server/docs/patterns.md +++ b/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/). diff --git a/server/fleet/errors.go b/server/fleet/errors.go index 77098617bc73..8dc616bdb54a 100644 --- a/server/fleet/errors.go +++ b/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, diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 0d2965c99713..3b277fcf9816 100644 --- a/server/service/apple_mdm.go +++ b/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: