Skip to content

refactor(worker): use *logging.Logger directly instead of custom inteface #720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexluong merged 1 commit into from
Mar 4, 2026
Merged

refactor(worker): use *logging.Logger directly instead of custom inteface #720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions internal/worker/logger_test.go
View file
Open in desktop

This file was deleted.

13 changes: 3 additions & 10 deletions internal/worker/supervisor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,16 @@ import (
"sync"
"time"

"github.com/hookdeck/outpost/internal/logging"
"go.uber.org/zap"
)

// Logger is a minimal logging interface for structured logging with zap.
type Logger interface {
Info(msg string, fields ...zap.Field)
Error(msg string, fields ...zap.Field)
Debug(msg string, fields ...zap.Field)
Warn(msg string, fields ...zap.Field)
}

// WorkerSupervisor manages and supervises multiple workers.
// It tracks their health and handles graceful shutdown.
type WorkerSupervisor struct {
workers map[string]Worker
health *HealthTracker
logger Logger
logger *logging.Logger
shutdownTimeout time.Duration // 0 means no timeout
}

Expand All @@ -40,7 +33,7 @@ func WithShutdownTimeout(timeout time.Duration) SupervisorOption {
}

// NewWorkerSupervisor creates a new WorkerSupervisor.
func NewWorkerSupervisor(logger Logger, opts ...SupervisorOption) *WorkerSupervisor {
func NewWorkerSupervisor(logger *logging.Logger, opts ...SupervisorOption) *WorkerSupervisor {
r := &WorkerSupervisor{
workers: make(map[string]Worker),
health: NewHealthTracker(),
Expand Down
80 changes: 13 additions & 67 deletions internal/worker/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import (
"testing"
"time"

"github.com/hookdeck/outpost/internal/util/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
)

// Mock worker for testing
Expand Down Expand Up @@ -50,55 +50,6 @@ func (m *mockWorker) WasStarted() bool {
return m.started
}

// Mock logger for testing
type mockLogger struct {
mu sync.Mutex
messages []string
}

func newMockLogger() *mockLogger {
return &mockLogger{
messages: []string{},
}
}

func (l *mockLogger) log(level, msg string, fields ...zap.Field) {
l.mu.Lock()
defer l.mu.Unlock()
l.messages = append(l.messages, fmt.Sprintf("[%s] %s", level, msg))
}

func (l *mockLogger) Info(msg string, fields ...zap.Field) {
l.log("INFO", msg, fields...)
}

func (l *mockLogger) Error(msg string, fields ...zap.Field) {
l.log("ERROR", msg, fields...)
}

func (l *mockLogger) Debug(msg string, fields ...zap.Field) {
l.log("DEBUG", msg, fields...)
}

func (l *mockLogger) Warn(msg string, fields ...zap.Field) {
l.log("WARN", msg, fields...)
}

func (l *mockLogger) Contains(substr string) bool {
l.mu.Lock()
defer l.mu.Unlock()
for _, msg := range l.messages {
if contains(msg, substr) {
return true
}
}
return false
}

func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || s[1:len(s)-1] != s[1:len(s)-1] && contains(s[1:], substr)))
}

// Tests

func TestHealthTracker_MarkHealthy(t *testing.T) {
Expand Down Expand Up @@ -174,18 +125,17 @@ func TestHealthTracker_NoErrorExposed(t *testing.T) {
}

func TestWorkerSupervisor_RegisterWorker(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

worker := newMockWorker("test-worker", nil)
supervisor.Register(worker)

assert.Len(t, supervisor.workers, 1)
assert.True(t, logger.Contains("worker registered"))
}

func TestWorkerSupervisor_RegisterDuplicateWorker(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

worker1 := newMockWorker("test-worker", nil)
Expand All @@ -199,7 +149,7 @@ func TestWorkerSupervisor_RegisterDuplicateWorker(t *testing.T) {
}

func TestWorkerSupervisor_Run_HealthyWorkers(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

worker1 := newMockWorker("worker-1", func(ctx context.Context) error {
Expand Down Expand Up @@ -250,7 +200,7 @@ func TestWorkerSupervisor_Run_HealthyWorkers(t *testing.T) {
}

func TestWorkerSupervisor_Run_FailedWorker(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

healthyWorker := newMockWorker("healthy", func(ctx context.Context) error {
Expand Down Expand Up @@ -301,7 +251,7 @@ func TestWorkerSupervisor_Run_FailedWorker(t *testing.T) {
}

func TestWorkerSupervisor_Run_AllWorkersExit(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

// Both workers exit on their own (not from context cancellation)
Expand Down Expand Up @@ -339,13 +289,10 @@ func TestWorkerSupervisor_Run_AllWorkersExit(t *testing.T) {
workers := status["workers"].(map[string]WorkerHealth)
assert.Equal(t, WorkerStatusFailed, workers["worker-1"].Status)
assert.Equal(t, WorkerStatusFailed, workers["worker-2"].Status)

// Verify log message
assert.True(t, logger.Contains("all workers have exited"))
}

func TestWorkerSupervisor_Run_ContextCanceled(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

worker := newMockWorker("worker-1", func(ctx context.Context) error {
Expand Down Expand Up @@ -373,15 +320,14 @@ func TestWorkerSupervisor_Run_ContextCanceled(t *testing.T) {
}

func TestWorkerSupervisor_Run_NoWorkers(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

ctx := context.Background()
err := supervisor.Run(ctx)

assert.Error(t, err)
assert.Contains(t, err.Error(), "no workers registered")
assert.True(t, logger.Contains("no workers registered"))
}

func TestHealthTracker_ConcurrentAccess(t *testing.T) {
Expand Down Expand Up @@ -425,7 +371,7 @@ func TestHealthTracker_ConcurrentAccess(t *testing.T) {
}

func TestWorkerSupervisor_Run_VariableShutdownTiming(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

// Worker that shuts down quickly (50ms)
Expand Down Expand Up @@ -483,7 +429,7 @@ func TestWorkerSupervisor_Run_VariableShutdownTiming(t *testing.T) {
}

func TestWorkerSupervisor_Run_VerySlowShutdown_NoTimeout(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger) // No timeout

// Worker that takes a very long time to shutdown (2 seconds)
Expand Down Expand Up @@ -528,7 +474,7 @@ func TestWorkerSupervisor_Run_VerySlowShutdown_NoTimeout(t *testing.T) {
}

func TestWorkerSupervisor_Run_ShutdownTimeout(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
// Set shutdown timeout to 500ms
supervisor := NewWorkerSupervisor(logger, WithShutdownTimeout(500*time.Millisecond))

Expand Down Expand Up @@ -573,7 +519,7 @@ func TestWorkerSupervisor_Run_ShutdownTimeout(t *testing.T) {
}

func TestWorkerSupervisor_Run_ShutdownTimeout_FastWorkers(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
// Set shutdown timeout to 2s
supervisor := NewWorkerSupervisor(logger, WithShutdownTimeout(2*time.Second))

Expand Down Expand Up @@ -617,7 +563,7 @@ func TestWorkerSupervisor_Run_ShutdownTimeout_FastWorkers(t *testing.T) {
}

func TestWorkerSupervisor_Run_StuckWorker(t *testing.T) {
logger := newMockLogger()
logger := testutil.CreateTestLogger(t)
supervisor := NewWorkerSupervisor(logger)

// Worker that never shuts down (ignores context cancellation)
Expand Down
Loading