From d387f56a1c3fe10abd39d7171379ec1997d04edf Mon Sep 17 00:00:00 2001 From: abhaygoudannavar Date: Fri, 1 May 2026 09:25:35 +0000 Subject: [PATCH 1/3] fix(metrics): fallback to mock writer on file open failure When timestamps are enabled but the target file cannot be opened, NewZerologMetrics returned nil. This caused a nil pointer dereference on subsequent metrics.Capture() calls. This fix gracefully degrades to a no-op mockWriter and logs a warning. Adds a regression test to verify the fallback behavior. Fixes #595 Signed-off-by: abhaygoudannavar --- internal/metrics/metrics.go | 4 +++- internal/metrics/metrics_test.go | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 69a815fc6..cab21baec 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -18,6 +18,7 @@ import ( "os" "github.com/rs/zerolog" + "github.com/sirupsen/logrus" ) type Writer interface { @@ -57,7 +58,8 @@ func NewZerologMetrics(enabled bool, target string, containerID string) Writer { zerolog.TimeFieldFormat = zerolog.TimeFormatUnixNano file, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) if err != nil { - return nil + logrus.Warnf("Failed to open metrics file %s: %v. Metrics will be disabled.", target, err) + return &mockWriter{} } logger := zerolog.New(file).Level(zerolog.InfoLevel).With().Timestamp().Logger() return &zerologMetrics{ diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index cb4230d7d..e5f541dc3 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -52,3 +52,15 @@ func TestZerologMetricsMetadata(t *testing.T) { t.Errorf("timestampOrder = %v, want 0", got) } } + +func TestZerologMetricsInvalidFileDoesNotPanic(t *testing.T) { + // Provide a path to a directory that definitely does not exist + invalidPath := "/does/not/exist/timestamps.log" + + // Create the metrics writer with timestamps enabled but an invalid path + writer := NewZerologMetrics(true, invalidPath, "container-test") + + // This call would panic with a nil pointer dereference without the fix. + // With the fix, it will safely execute the mockWriter's no-op Capture(). + writer.Capture(TS00) +} From cfe614812a5688a069b40ad35cd4c3edeaca7901 Mon Sep 17 00:00:00 2001 From: abhaygoudannavar Date: Fri, 15 May 2026 14:14:10 +0000 Subject: [PATCH 2/3] test(metrics): explicitly check for non-nil writer instead of crash testing Signed-off-by: abhaygoudannavar --- internal/metrics/metrics_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index e5f541dc3..31d096f45 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -56,11 +56,11 @@ func TestZerologMetricsMetadata(t *testing.T) { func TestZerologMetricsInvalidFileDoesNotPanic(t *testing.T) { // Provide a path to a directory that definitely does not exist invalidPath := "/does/not/exist/timestamps.log" - + // Create the metrics writer with timestamps enabled but an invalid path writer := NewZerologMetrics(true, invalidPath, "container-test") - - // This call would panic with a nil pointer dereference without the fix. - // With the fix, it will safely execute the mockWriter's no-op Capture(). - writer.Capture(TS00) + + if writer == nil { + t.Fatal("expected non-nil writer (fallback to mockWriter), got nil") + } } From 8238532a9354f98cb79c299e9521f0b7d1f6c856 Mon Sep 17 00:00:00 2001 From: abhaygoudannavar Date: Wed, 20 May 2026 18:03:22 +0000 Subject: [PATCH 3/3] refactor(metrics): use testify assert/require in tests Replace manual if/t.Errorf checks with testify assert.Equal and require.NoError/require.NotNil as requested by maintainer. Signed-off-by: abhaygoudannavar --- internal/metrics/metrics_test.go | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index 31d096f45..a8e5b6d2b 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -20,6 +20,8 @@ import ( "testing" "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestZerologMetricsMetadata(t *testing.T) { @@ -35,22 +37,13 @@ func TestZerologMetricsMetadata(t *testing.T) { line := buf.String() var m map[string]any - if err := json.Unmarshal([]byte(line), &m); err != nil { - t.Fatalf("failed to parse log: %v", err) - } + err := json.Unmarshal([]byte(line), &m) + require.NoError(t, err, "failed to parse log output") - if got := m["containerID"]; got != "container00" { - t.Errorf("containerID = %v, want container00", got) - } - if got := m["timestampID"]; got != "TS00" { - t.Errorf("timestampID = %v, want TS00", got) - } - if got := m["timestampName"]; got != "CR.invoked" { - t.Errorf("timestampName = %v, want CR.invoked", got) - } - if got := int(m["timestampOrder"].(float64)); got != 0 { - t.Errorf("timestampOrder = %v, want 0", got) - } + assert.Equal(t, "container00", m["containerID"], "containerID mismatch") + assert.Equal(t, "TS00", m["timestampID"], "timestampID mismatch") + assert.Equal(t, "CR.invoked", m["timestampName"], "timestampName mismatch") + assert.Equal(t, float64(0), m["timestampOrder"], "timestampOrder mismatch") } func TestZerologMetricsInvalidFileDoesNotPanic(t *testing.T) { @@ -60,7 +53,5 @@ func TestZerologMetricsInvalidFileDoesNotPanic(t *testing.T) { // Create the metrics writer with timestamps enabled but an invalid path writer := NewZerologMetrics(true, invalidPath, "container-test") - if writer == nil { - t.Fatal("expected non-nil writer (fallback to mockWriter), got nil") - } + require.NotNil(t, writer, "expected non-nil writer (fallback to mockWriter), got nil") }