diff --git a/changes/fix-logging-of-errors b/changes/fix-logging-of-errors new file mode 100644 index 0000000000..2c647ec10f --- /dev/null +++ b/changes/fix-logging-of-errors @@ -0,0 +1 @@ +* Fix logging of errors in fleet serve. diff --git a/server/contexts/logging/logging.go b/server/contexts/logging/logging.go index 7dc1c460ea..7f4d2a33e2 100644 --- a/server/contexts/logging/logging.go +++ b/server/contexts/logging/logging.go @@ -110,13 +110,24 @@ func (l *LoggingContext) Log(ctx context.Context, logger kitlog.Logger) { } if len(l.Errs) > 0 { - var errs []string - var internalErrs []string + // Going for string concatenation here instead of json.Marshal mostly to not have to deal with error handling + // within this method. kitlog doesn't support slices of strings + var errs string + var internalErrs string + separator := " || " for _, err := range l.Errs { if e, ok := err.(fleet.ErrWithInternal); ok { - internalErrs = append(internalErrs, e.Internal()) + if internalErrs == "" { + internalErrs = e.Internal() + } else { + internalErrs += separator + e.Internal() + } } else { - errs = append(errs, err.Error()) + if errs == "" { + errs = err.Error() + } else { + errs += separator + err.Error() + } } } if len(errs) > 0 { diff --git a/server/contexts/logging/logging_test.go b/server/contexts/logging/logging_test.go new file mode 100644 index 0000000000..53eb593a2a --- /dev/null +++ b/server/contexts/logging/logging_test.go @@ -0,0 +1,43 @@ +package logging + +import ( + "bytes" + "context" + "strings" + "testing" + + kitlog "github.com/go-kit/kit/log" + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" +) + +func TestLoggingErrs(t *testing.T) { + setupTest := func() (*bytes.Buffer, kitlog.Logger, *LoggingContext, context.Context) { + buf := new(bytes.Buffer) + logger := kitlog.NewLogfmtLogger(buf) + lc := &LoggingContext{} + ctx := NewContext(context.Background(), lc) + return buf, logger, lc, ctx + } + checkLogEnds := func(t *testing.T, logLine string, expected string) bool { + return assert.True(t, strings.HasSuffix(strings.TrimSpace(logLine), expected), logLine) + } + + t.Run("one error", func(t *testing.T) { + buf, logger, lc, ctx := setupTest() + + WithErr(ctx, errors.Wrap(errors.New("AAAA"), "BLAH")) + lc.Log(ctx, logger) + logLine := buf.String() + checkLogEnds(t, logLine, `err="BLAH: AAAA"`) + }) + t.Run("two errors", func(t *testing.T) { + buf, logger, lc, ctx := setupTest() + + WithErr(ctx, errors.Wrap(errors.New("AAAA"), "BLAH")) + WithErr(ctx, errors.Wrap(errors.New("BBBB"), "FOO")) + lc.Log(ctx, logger) + logLine := buf.String() + checkLogEnds(t, logLine, `err="BLAH: AAAA || FOO: BBBB"`) + }) +} diff --git a/server/service/integration_logger_test.go b/server/service/integration_logger_test.go index 2f39f97496..c2e76e9e9a 100644 --- a/server/service/integration_logger_test.go +++ b/server/service/integration_logger_test.go @@ -182,5 +182,5 @@ func (s *integrationLoggerTestSuite) TestEnrollAgentLogsErrors() { require.Len(t, parts, 1) logData := make(map[string]json.RawMessage) require.NoError(t, json.Unmarshal([]byte(parts[0]), &logData)) - assert.Equal(t, json.RawMessage(`["enroll failed: no matching secret found"]`), logData["err"]) + assert.Equal(t, json.RawMessage(`"enroll failed: no matching secret found"`), logData["err"]) } diff --git a/server/service/service_osquery_test.go b/server/service/service_osquery_test.go index 0c1a71869c..b78fad43c9 100644 --- a/server/service/service_osquery_test.go +++ b/server/service/service_osquery_test.go @@ -1664,8 +1664,8 @@ func TestDistributedQueriesLogsManyErrors(t *testing.T) { require.Len(t, parts, 1) logData := make(map[string]json.RawMessage) require.NoError(t, json.Unmarshal([]byte(parts[0]), &logData)) - assert.Equal(t, json.RawMessage(`["something went wrong"]`), logData["err"]) - assert.Equal(t, json.RawMessage(`["Missing authorization check"]`), logData["internal"]) + assert.Equal(t, json.RawMessage(`"something went wrong"`), logData["err"]) + assert.Equal(t, json.RawMessage(`"Missing authorization check"`), logData["internal"]) } func TestDistributedQueriesReloadsHostIfDetailsAreIn(t *testing.T) {