[-] return the error from MultiWriter.DefineMetrics(), fixes #1222 (#1224)

* Fix: return the accumulated error instead of nil #1222

* improve tests

---------

Co-authored-by: Pavlo Golub <pavlo.golub@gmail.com>

Author: Mahmoud-Ayman-Saleh

internal/sinks/multiwriter.go

@@ -77,7 +77,7 @@ func (mw *MultiWriter) DefineMetrics(metrics *metrics.Metrics) (err error) {
 			err = errors.Join(err, definer.DefineMetrics(metrics))
 		}
 	}
-	return nil
+	return
 }
 
 func (mw *MultiWriter) SyncMetric(sourceName, metricName string, op SyncOp) (err error) {

internal/sinks/multiwriter_test.go

@@ -9,14 +9,31 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-type MockWriter struct{}
+// mockWriter implements Writer and Migrator interfaces
+type mockWriter struct {
+	err               error
+	needsMigration    bool
+	needsMigrationErr error
+}
+
+func (m *mockWriter) SyncMetric(string, string, sinks.SyncOp) error {
+	return m.err
+}
+
+func (m *mockWriter) Write(metrics.MeasurementEnvelope) error {
+	return m.err
+}
+
+func (m *mockWriter) Migrate() error {
+	return m.err
+}
 
-func (mw *MockWriter) SyncMetric(_, _ string, _ sinks.SyncOp) error {
-	return nil
+func (m *mockWriter) NeedsMigration() (bool, error) {
+	return m.needsMigration, m.needsMigrationErr
 }
 
-func (mw *MockWriter) Write(_ metrics.MeasurementEnvelope) error {
-	return nil
+func (m *mockWriter) DefineMetrics(*metrics.Metrics) error {
+	return m.err
 }
 
 func TestNewMultiWriter(t *testing.T) {
@@ -66,50 +83,27 @@ func TestNewMultiWriter(t *testing.T) {
 
 func TestAddWriter(t *testing.T) {
 	mw := &sinks.MultiWriter{}
-	mockWriter := &MockWriter{}
+	mockWriter := &mockWriter{}
 	mw.AddWriter(mockWriter)
 	assert.Equal(t, 1, mw.Count())
 }
 
 func TestSyncMetrics(t *testing.T) {
 	mw := &sinks.MultiWriter{}
-	mockWriter := &MockWriter{}
+	mockWriter := &mockWriter{}
 	mw.AddWriter(mockWriter)
 	err := mw.SyncMetric("db", "metric", sinks.InvalidOp)
 	assert.NoError(t, err)
 }
 
 func TestWriteMeasurements(t *testing.T) {
 	mw := &sinks.MultiWriter{}
-	mockWriter := &MockWriter{}
+	mockWriter := &mockWriter{}
 	mw.AddWriter(mockWriter)
 	err := mw.Write(metrics.MeasurementEnvelope{})
 	assert.NoError(t, err)
 }
 
-// mockMigratableWriter implements Writer and Migrator interfaces
-type mockMigratableWriter struct {
-	migrateErr        error
-	needsMigration    bool
-	needsMigrationErr error
-}
-
-func (m *mockMigratableWriter) SyncMetric(string, string, sinks.SyncOp) error {
-	return nil
-}
-
-func (m *mockMigratableWriter) Write(metrics.MeasurementEnvelope) error {
-	return nil
-}
-
-func (m *mockMigratableWriter) Migrate() error {
-	return m.migrateErr
-}
-
-func (m *mockMigratableWriter) NeedsMigration() (bool, error) {
-	return m.needsMigration, m.needsMigrationErr
-}
-
 func TestMultiWriterMigrate(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -119,45 +113,45 @@ func TestMultiWriterMigrate(t *testing.T) {
 		{
 			name: "no migratable writers",
 			writers: []sinks.Writer{
-				&MockWriter{},
+				&mockWriter{},
 			},
 			expectError: false,
 		},
 		{
 			name: "single migratable writer success",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{},
+				&mockWriter{},
 			},
 			expectError: false,
 		},
 		{
 			name: "single migratable writer error",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{migrateErr: assert.AnError},
+				&mockWriter{err: assert.AnError},
 			},
 			expectError: true,
 		},
 		{
 			name: "multiple migratable writers success",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{},
-				&mockMigratableWriter{},
+				&mockWriter{},
+				&mockWriter{},
 			},
 			expectError: false,
 		},
 		{
 			name: "multiple writers with one error",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{},
-				&mockMigratableWriter{migrateErr: assert.AnError},
+				&mockWriter{},
+				&mockWriter{err: assert.AnError},
 			},
 			expectError: true,
 		},
 		{
 			name: "mixed writers with migration error",
 			writers: []sinks.Writer{
-				&MockWriter{},
-				&mockMigratableWriter{migrateErr: assert.AnError},
+				&mockWriter{},
+				&mockWriter{err: assert.AnError},
 			},
 			expectError: true,
 		},
@@ -179,6 +173,69 @@ func TestMultiWriterMigrate(t *testing.T) {
 	}
 }
 
+func TestDefineMetrics(t *testing.T) {
+	tests := []struct {
+		name        string
+		writers     []sinks.Writer
+		expectError bool
+	}{
+		{
+			name:        "writer without DefineMetrics",
+			writers:     []sinks.Writer{&mockWriter{}},
+			expectError: false,
+		},
+		{
+			name:        "single definer success",
+			writers:     []sinks.Writer{&mockWriter{err: nil}},
+			expectError: false,
+		},
+		{
+			name:        "single definer error",
+			writers:     []sinks.Writer{&mockWriter{err: assert.AnError}},
+			expectError: true,
+		},
+		{
+			name: "two definers errors",
+			writers: []sinks.Writer{
+				&mockWriter{err: assert.AnError},
+				&mockWriter{err: assert.AnError},
+			},
+			expectError: true,
+		},
+		{
+			name: "mixed writers error",
+			writers: []sinks.Writer{
+				&mockWriter{},
+				&mockWriter{err: assert.AnError},
+			},
+			expectError: true,
+		},
+		{
+			name: "mixed writers success",
+			writers: []sinks.Writer{
+				&mockWriter{},
+				&mockWriter{err: nil},
+			},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mw := &sinks.MultiWriter{}
+			for _, w := range tt.writers {
+				mw.AddWriter(w)
+			}
+			err := mw.DefineMetrics(&metrics.Metrics{})
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestMultiWriterNeedsMigration(t *testing.T) {
 	tests := []struct {
 		name               string
@@ -189,49 +246,49 @@ func TestMultiWriterNeedsMigration(t *testing.T) {
 		{
 			name: "no migratable writers",
 			writers: []sinks.Writer{
-				&MockWriter{},
+				&mockWriter{},
 			},
 			expectNeedsMigrate: false,
 			expectError:        false,
 		},
 		{
 			name: "single writer needs migration",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{needsMigration: true},
+				&mockWriter{needsMigration: true},
 			},
 			expectNeedsMigrate: true,
 			expectError:        false,
 		},
 		{
 			name: "single writer no migration needed",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{needsMigration: false},
+				&mockWriter{needsMigration: false},
 			},
 			expectNeedsMigrate: false,
 			expectError:        false,
 		},
 		{
 			name: "multiple writers one needs migration",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{needsMigration: false},
-				&mockMigratableWriter{needsMigration: true},
+				&mockWriter{needsMigration: false},
+				&mockWriter{needsMigration: true},
 			},
 			expectNeedsMigrate: true,
 			expectError:        false,
 		},
 		{
 			name: "error checking migration",
 			writers: []sinks.Writer{
-				&mockMigratableWriter{needsMigrationErr: assert.AnError},
+				&mockWriter{needsMigrationErr: assert.AnError},
 			},
 			expectNeedsMigrate: false,
 			expectError:        true,
 		},
 		{
 			name: "mixed writers one needs migration",
 			writers: []sinks.Writer{
-				&MockWriter{},
-				&mockMigratableWriter{needsMigration: true},
+				&mockWriter{},
+				&mockWriter{needsMigration: true},
 			},
 			expectNeedsMigrate: true,
 			expectError:        false,