From 29b3533df0af108f57591af190b7986e47439152 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Sun, 4 Feb 2024 16:27:36 +0530 Subject: [PATCH 1/9] Add unit tests for anonymizer/app/writer Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/.nocover | 1 - cmd/anonymizer/app/writer/writer_test.go | 149 +++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) delete mode 100644 cmd/anonymizer/app/writer/.nocover create mode 100644 cmd/anonymizer/app/writer/writer_test.go diff --git a/cmd/anonymizer/app/writer/.nocover b/cmd/anonymizer/app/writer/.nocover deleted file mode 100644 index 5d2db86d7c0..00000000000 --- a/cmd/anonymizer/app/writer/.nocover +++ /dev/null @@ -1 +0,0 @@ -nobn-critical test utility diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go new file mode 100644 index 00000000000..f71fd5275e9 --- /dev/null +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -0,0 +1,149 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package writer + +import ( + "os" + "os/exec" + "testing" + "time" + + "github.com/crossdock/crossdock-go/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/model" +) + +var tags = []model.KeyValue{ + model.Bool("error", true), + model.String("http.method", "POST"), + model.Bool("foobar", true), +} + +var traceID = model.NewTraceID(1, 2) + +var span = &model.Span{ + TraceID: traceID, + SpanID: model.NewSpanID(1), + Process: &model.Process{ + ServiceName: "serviceName", + Tags: tags, + }, + OperationName: "operationName", + Tags: tags, + Logs: []model.Log{ + { + Timestamp: time.Now(), + Fields: []model.KeyValue{ + model.String("logKey", "logValue"), + }, + }, + }, + Duration: time.Second * 5, + StartTime: time.Unix(300, 0), +} + +func TestNew(t *testing.T) { + nopLogger := zap.NewNop() + tempDir := t.TempDir() + + tests := []struct { + name string + config Config + }{ + { + name: "successfully create writer", + config: Config{ + MaxSpansCount: 10, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + }, + }, + { + name: "captured.json doesn't exist", + config: Config{ + CapturedFile: tempDir + "/nonexistent_directory/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + }, + }, + { + name: "anonymized.json doesn't exist", + config: Config{ + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/nonexistent_directory/anonymized.json", + MappingFile: tempDir + "/mapping.json", + }, + }, + } + + t.Run(tests[0].name, func(t *testing.T) { + _, err := New(tests[0].config, nopLogger) + require.NoError(t, err) + }) + t.Run(tests[1].name, func(t *testing.T) { + _, err := New(tests[1].config, nopLogger) + require.Error(t, err) + }) + t.Run(tests[2].name, func(t *testing.T) { + _, err := New(tests[2].config, nopLogger) + require.Error(t, err) + }) +} + +func TestWriter_WriteSpan(t *testing.T) { + nopLogger := zap.NewNop() + tempDir := t.TempDir() + + config := Config{ + MaxSpansCount: 101, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + + writer, err := New(config, nopLogger) + require.NoError(t, err) + + for i := 0; i <= 99; i++ { + err = writer.WriteSpan(span) + assert.NoError(t, err) + } +} + +func TestWriter_WriteSpan_Exits(t *testing.T) { + if os.Getenv("BE_SUBPROCESS") == "1" { + tempDir := t.TempDir() + config := Config{ + MaxSpansCount: 1, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + + writer, err := New(config, zap.NewNop()) + require.NoError(t, err) + + err = writer.WriteSpan(span) + require.NoError(t, err) + + err = writer.WriteSpan(span) + if err == nil { + t.Fatal("expected an error but got none") + } + + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestWriter_WriteSpan_Exits") + cmd.Env = append(os.Environ(), "BE_SUBPROCESS=1") + err := cmd.Run() + // The process should have exited with status 1, but exec.Command returns + // an *ExitError when the process exits with a non-zero status. + if err != nil { + t.Fatalf("process ran with err %v, want exit status 1", err) + } +} From ab5ba418b9bcb9f9d98fa25445b45b65400da625 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Sun, 4 Feb 2024 16:40:03 +0530 Subject: [PATCH 2/9] Fix errors from make lint test Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index f71fd5275e9..1de94e6b121 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/crossdock/crossdock-go/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" @@ -110,7 +109,7 @@ func TestWriter_WriteSpan(t *testing.T) { for i := 0; i <= 99; i++ { err = writer.WriteSpan(span) - assert.NoError(t, err) + require.NoError(t, err) } } From 76af9d7611bcd9e7895189130037de69c0c19d81 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Tue, 6 Feb 2024 12:56:05 +0530 Subject: [PATCH 3/9] Fix code quality Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 72 ++++++++---------------- 1 file changed, 24 insertions(+), 48 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index 1de94e6b121..165cb2a8141 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -48,49 +48,30 @@ func TestNew(t *testing.T) { nopLogger := zap.NewNop() tempDir := t.TempDir() - tests := []struct { - name string - config Config - }{ - { - name: "successfully create writer", - config: Config{ - MaxSpansCount: 10, - CapturedFile: tempDir + "/captured.json", - AnonymizedFile: tempDir + "/anonymized.json", - MappingFile: tempDir + "/mapping.json", - }, - }, - { - name: "captured.json doesn't exist", - config: Config{ - CapturedFile: tempDir + "/nonexistent_directory/captured.json", - AnonymizedFile: tempDir + "/anonymized.json", - MappingFile: tempDir + "/mapping.json", - }, - }, - { - name: "anonymized.json doesn't exist", - config: Config{ - CapturedFile: tempDir + "/captured.json", - AnonymizedFile: tempDir + "/nonexistent_directory/anonymized.json", - MappingFile: tempDir + "/mapping.json", - }, - }, + config := Config{ + MaxSpansCount: 10, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", } + _, err := New(config, nopLogger) + require.NoError(t, err) - t.Run(tests[0].name, func(t *testing.T) { - _, err := New(tests[0].config, nopLogger) - require.NoError(t, err) - }) - t.Run(tests[1].name, func(t *testing.T) { - _, err := New(tests[1].config, nopLogger) - require.Error(t, err) - }) - t.Run(tests[2].name, func(t *testing.T) { - _, err := New(tests[2].config, nopLogger) - require.Error(t, err) - }) + config = Config{ + CapturedFile: tempDir + "/nonexistent_directory/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + _, err = New(config, nopLogger) + require.Error(t, err) + + config = Config{ + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/nonexistent_directory/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + _, err = New(config, nopLogger) + require.Error(t, err) } func TestWriter_WriteSpan(t *testing.T) { @@ -129,10 +110,7 @@ func TestWriter_WriteSpan_Exits(t *testing.T) { err = writer.WriteSpan(span) require.NoError(t, err) - err = writer.WriteSpan(span) - if err == nil { - t.Fatal("expected an error but got none") - } + require.Error(t, writer.WriteSpan(span)) return } @@ -142,7 +120,5 @@ func TestWriter_WriteSpan_Exits(t *testing.T) { err := cmd.Run() // The process should have exited with status 1, but exec.Command returns // an *ExitError when the process exits with a non-zero status. - if err != nil { - t.Fatalf("process ran with err %v, want exit status 1", err) - } + require.NoError(t, err) } From 32bc420ac8eb43a03b541157c36a1366614a0559 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Thu, 8 Feb 2024 03:09:19 +0530 Subject: [PATCH 4/9] Add tags to tests and improve error handling in WriteSpan function Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer.go | 3 +- cmd/anonymizer/app/writer/writer_test.go | 103 +++++++++++------------ 2 files changed, 49 insertions(+), 57 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer.go b/cmd/anonymizer/app/writer/writer.go index dee49e1735d..d1fbab00b54 100644 --- a/cmd/anonymizer/app/writer/writer.go +++ b/cmd/anonymizer/app/writer/writer.go @@ -17,6 +17,7 @@ package writer import ( "bytes" "encoding/json" + "errors" "fmt" "os" "sync" @@ -130,7 +131,7 @@ func (w *Writer) WriteSpan(msg *model.Span) error { if w.config.MaxSpansCount > 0 && w.spanCount >= w.config.MaxSpansCount { w.logger.Info("Saved enough spans, exiting...") w.Close() - os.Exit(0) + return errors.New("saved MaxSpansCount") } return nil diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index 165cb2a8141..b992fcd191f 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -4,8 +4,6 @@ package writer import ( - "os" - "os/exec" "testing" "time" @@ -48,54 +46,58 @@ func TestNew(t *testing.T) { nopLogger := zap.NewNop() tempDir := t.TempDir() - config := Config{ - MaxSpansCount: 10, - CapturedFile: tempDir + "/captured.json", - AnonymizedFile: tempDir + "/anonymized.json", - MappingFile: tempDir + "/mapping.json", - } - _, err := New(config, nopLogger) - require.NoError(t, err) - - config = Config{ - CapturedFile: tempDir + "/nonexistent_directory/captured.json", - AnonymizedFile: tempDir + "/anonymized.json", - MappingFile: tempDir + "/mapping.json", - } - _, err = New(config, nopLogger) - require.Error(t, err) - - config = Config{ - CapturedFile: tempDir + "/captured.json", - AnonymizedFile: tempDir + "/nonexistent_directory/anonymized.json", - MappingFile: tempDir + "/mapping.json", - } - _, err = New(config, nopLogger) - require.Error(t, err) + t.Run("no error", func(t *testing.T) { + config := Config{ + MaxSpansCount: 10, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + _, err := New(config, nopLogger) + require.NoError(t, err) + }) + + t.Run("CapturedFile does not exist", func(t *testing.T) { + config := Config{ + CapturedFile: tempDir + "/nonexistent_directory/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + _, err := New(config, nopLogger) + require.ErrorContains(t, err, "cannot create output file") + }) + + t.Run("AnonymizedFile does not exist", func(t *testing.T) { + config := Config{ + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/nonexistent_directory/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } + _, err := New(config, nopLogger) + require.ErrorContains(t, err, "cannot create output file") + }) } func TestWriter_WriteSpan(t *testing.T) { nopLogger := zap.NewNop() - tempDir := t.TempDir() - - config := Config{ - MaxSpansCount: 101, - CapturedFile: tempDir + "/captured.json", - AnonymizedFile: tempDir + "/anonymized.json", - MappingFile: tempDir + "/mapping.json", - } - - writer, err := New(config, nopLogger) - require.NoError(t, err) + t.Run("write span", func(t *testing.T) { + tempDir := t.TempDir() + config := Config{ + MaxSpansCount: 10, + CapturedFile: tempDir + "/captured.json", + AnonymizedFile: tempDir + "/anonymized.json", + MappingFile: tempDir + "/mapping.json", + } - for i := 0; i <= 99; i++ { - err = writer.WriteSpan(span) + writer, err := New(config, nopLogger) require.NoError(t, err) - } -} -func TestWriter_WriteSpan_Exits(t *testing.T) { - if os.Getenv("BE_SUBPROCESS") == "1" { + for i := 0; i < 9; i++ { + err = writer.WriteSpan(span) + require.NoError(t, err) + } + }) + t.Run("write span with MaxSpansCount", func(t *testing.T) { tempDir := t.TempDir() config := Config{ MaxSpansCount: 1, @@ -108,17 +110,6 @@ func TestWriter_WriteSpan_Exits(t *testing.T) { require.NoError(t, err) err = writer.WriteSpan(span) - require.NoError(t, err) - - require.Error(t, writer.WriteSpan(span)) - - return - } - - cmd := exec.Command(os.Args[0], "-test.run=TestWriter_WriteSpan_Exits") - cmd.Env = append(os.Environ(), "BE_SUBPROCESS=1") - err := cmd.Run() - // The process should have exited with status 1, but exec.Command returns - // an *ExitError when the process exits with a non-zero status. - require.NoError(t, err) + require.ErrorContains(t, err, "saved MaxSpansCount") + }) } From b2bfc3e80d0e59274c68627c8e6f834767a318de Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Thu, 8 Feb 2024 03:13:15 +0530 Subject: [PATCH 5/9] Ensure writer is closed after running tests Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index b992fcd191f..33f82a57ce2 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -96,6 +96,7 @@ func TestWriter_WriteSpan(t *testing.T) { err = writer.WriteSpan(span) require.NoError(t, err) } + defer writer.Close() }) t.Run("write span with MaxSpansCount", func(t *testing.T) { tempDir := t.TempDir() @@ -111,5 +112,6 @@ func TestWriter_WriteSpan(t *testing.T) { err = writer.WriteSpan(span) require.ErrorContains(t, err, "saved MaxSpansCount") + defer writer.Close() }) } From b25f8ce71ed53ad34f2e26f93e447fa6f1817339 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Fri, 9 Feb 2024 02:15:20 +0530 Subject: [PATCH 6/9] Define constant error and perform os exit in main.go Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer.go | 4 +++- cmd/anonymizer/main.go | 13 ++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer.go b/cmd/anonymizer/app/writer/writer.go index d1fbab00b54..5b22a371c4b 100644 --- a/cmd/anonymizer/app/writer/writer.go +++ b/cmd/anonymizer/app/writer/writer.go @@ -29,6 +29,8 @@ import ( "github.com/jaegertracing/jaeger/model" ) +var ErrMaxSpansCountReached = errors.New("max spans count reached") + // Config contains parameters to NewWriter. type Config struct { MaxSpansCount int `yaml:"max_spans_count" name:"max_spans_count"` @@ -131,7 +133,7 @@ func (w *Writer) WriteSpan(msg *model.Span) error { if w.config.MaxSpansCount > 0 && w.spanCount >= w.config.MaxSpansCount { w.logger.Info("Saved enough spans, exiting...") w.Close() - return errors.New("saved MaxSpansCount") + return ErrMaxSpansCountReached } return nil diff --git a/cmd/anonymizer/main.go b/cmd/anonymizer/main.go index 259321247b0..c3e7af863f8 100644 --- a/cmd/anonymizer/main.go +++ b/cmd/anonymizer/main.go @@ -15,6 +15,7 @@ package main import ( + "errors" "fmt" "os" @@ -53,7 +54,7 @@ func main() { }, } - writer, err := writer.New(conf, logger) + writerObj, err := writer.New(conf, logger) if err != nil { logger.Fatal("error while creating writer object", zap.Error(err)) } @@ -69,9 +70,15 @@ func main() { } for _, span := range spans { - writer.WriteSpan(&span) + err := writerObj.WriteSpan(&span) + if err != nil { + logger.Fatal("error while writing span", zap.Error(err)) + if errors.Is(err, writer.ErrMaxSpansCountReached) { + os.Exit(0) + } + } } - writer.Close() + writerObj.Close() uiCfg := uiconv.Config{ CapturedFile: conf.AnonymizedFile, From 01f899e7f8462c5feaefe6ca9055052240900f52 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Fri, 9 Feb 2024 13:47:07 +0530 Subject: [PATCH 7/9] Fix error handling Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 2 +- cmd/anonymizer/main.go | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index 33f82a57ce2..52952a730c3 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -111,7 +111,7 @@ func TestWriter_WriteSpan(t *testing.T) { require.NoError(t, err) err = writer.WriteSpan(span) - require.ErrorContains(t, err, "saved MaxSpansCount") + require.ErrorContains(t, err, "max spans count reached") defer writer.Close() }) } diff --git a/cmd/anonymizer/main.go b/cmd/anonymizer/main.go index c3e7af863f8..d1a538195c0 100644 --- a/cmd/anonymizer/main.go +++ b/cmd/anonymizer/main.go @@ -54,7 +54,7 @@ func main() { }, } - writerObj, err := writer.New(conf, logger) + w, err := writer.New(conf, logger) if err != nil { logger.Fatal("error while creating writer object", zap.Error(err)) } @@ -70,15 +70,14 @@ func main() { } for _, span := range spans { - err := writerObj.WriteSpan(&span) - if err != nil { - logger.Fatal("error while writing span", zap.Error(err)) + if err := w.WriteSpan(&span); err != nil { if errors.Is(err, writer.ErrMaxSpansCountReached) { + logger.Error("error while writing span", zap.Error(err)) os.Exit(0) } } } - writerObj.Close() + w.Close() uiCfg := uiconv.Config{ CapturedFile: conf.AnonymizedFile, From 121969e0e700d2cf9a66f0683d2566856f4795f0 Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Fri, 9 Feb 2024 22:30:39 +0530 Subject: [PATCH 8/9] Fix errors and logging Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 2 +- cmd/anonymizer/main.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index 52952a730c3..8cda6cfb35b 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -111,7 +111,7 @@ func TestWriter_WriteSpan(t *testing.T) { require.NoError(t, err) err = writer.WriteSpan(span) - require.ErrorContains(t, err, "max spans count reached") + require.ErrorIs(t, err, ErrMaxSpansCountReached) defer writer.Close() }) } diff --git a/cmd/anonymizer/main.go b/cmd/anonymizer/main.go index d1a538195c0..0e2949146f1 100644 --- a/cmd/anonymizer/main.go +++ b/cmd/anonymizer/main.go @@ -72,9 +72,10 @@ func main() { for _, span := range spans { if err := w.WriteSpan(&span); err != nil { if errors.Is(err, writer.ErrMaxSpansCountReached) { - logger.Error("error while writing span", zap.Error(err)) + logger.Info("max spans count reached") os.Exit(0) } + logger.Error("error while writing span", zap.Error(err)) } } w.Close() From 2c56c76f9cd8662200406919d33a9a94ff0a59ff Mon Sep 17 00:00:00 2001 From: Ashutosh Srivastava Date: Fri, 9 Feb 2024 22:52:50 +0530 Subject: [PATCH 9/9] Fix defer statements Signed-off-by: Ashutosh Srivastava --- cmd/anonymizer/app/writer/writer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/anonymizer/app/writer/writer_test.go b/cmd/anonymizer/app/writer/writer_test.go index 8cda6cfb35b..73391682e30 100644 --- a/cmd/anonymizer/app/writer/writer_test.go +++ b/cmd/anonymizer/app/writer/writer_test.go @@ -91,12 +91,12 @@ func TestWriter_WriteSpan(t *testing.T) { writer, err := New(config, nopLogger) require.NoError(t, err) + defer writer.Close() for i := 0; i < 9; i++ { err = writer.WriteSpan(span) require.NoError(t, err) } - defer writer.Close() }) t.Run("write span with MaxSpansCount", func(t *testing.T) { tempDir := t.TempDir() @@ -109,9 +109,9 @@ func TestWriter_WriteSpan(t *testing.T) { writer, err := New(config, zap.NewNop()) require.NoError(t, err) + defer writer.Close() err = writer.WriteSpan(span) require.ErrorIs(t, err, ErrMaxSpansCountReached) - defer writer.Close() }) }