From 2864a65064bab0661a252a6614bc565114fc13bd Mon Sep 17 00:00:00 2001 From: GitHub Copilot Date: Sun, 24 May 2026 05:08:41 +0200 Subject: [PATCH 1/3] refactor: remove package-level loggers, thread via struct fields and context - Delete internal/log package (abtrlog); inline setup logic into arbiter.go as a setupLoggers() helper function - Add logger logr.Logger field to abtr struct; populate from Run() - Convert standalone startModules() to method on abtr - Change setupReporter() to return (report.Reporter, error) and pass a.logger to the yaml reporter - Fix defaultOpts() bug: was setting InfoLogPath twice, never ErrorLogPath - yamlreport.New() now returns (report.Reporter, error); errors when opts.Logger.GetSink() == nil (zero-value / not provided) - Add Logger logr.Logger field to yamlreport.Opts and reporter struct - Replace zerologr.Info() global call in internal/otel with logr.FromContextOrDiscard(ctx).Info() (context transport) - Update yaml_test.go to supply an explicit logger via funcr Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- arbiter.go | 129 ++++++++++++++++++++++------------- internal/log/log.go | 80 ---------------------- internal/otel/instrument.go | 4 +- pkg/report/yaml/yaml.go | 28 ++++---- pkg/report/yaml/yaml_test.go | 12 +++- 5 files changed, 110 insertions(+), 143 deletions(-) delete mode 100644 internal/log/log.go diff --git a/arbiter.go b/arbiter.go index 51b30db..82630f4 100644 --- a/arbiter.go +++ b/arbiter.go @@ -12,7 +12,6 @@ import ( "time" "github.com/go-logr/logr" - abtrlog "github.com/maansaake/arbiter/internal/log" "github.com/maansaake/arbiter/pkg/module" "github.com/maansaake/arbiter/pkg/report" "github.com/maansaake/arbiter/pkg/report/collection" @@ -25,6 +24,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/trebent/envparser" + "github.com/trebent/zerologr" ) type ( @@ -38,6 +38,8 @@ type ( reportPath string // interactive is set when an interactive TUI reporting is used. interactive bool + // logger is used for info-level logging. + logger logr.Logger // errorLogger is the logger used for error logs by the reporter. errorLogger logr.Logger } @@ -51,27 +53,26 @@ type ( } ) +const ( + defaultInfoLogPath = "info.log" + defaultErrorLogPath = "error.log" + defaultDuration = time.Minute * 5 + defaultReportPath = "report.yaml" + defaultInteractive = false +) + // defaultOpts sets zero-value fields to their defaults. func (o *Opts) defaultOpts() { if o.ErrorLogPath == "" { - o.InfoLogPath = abtrlog.DefaultErrorLogPath + o.ErrorLogPath = defaultErrorLogPath } - if o.ErrorLogPath == "" { - o.InfoLogPath = abtrlog.DefaultInfoLogPath + if o.InfoLogPath == "" { + o.InfoLogPath = defaultInfoLogPath } } -const ( - defaultDuration = time.Minute * 5 - defaultReportPath = "report.yaml" - defaultInteractive = false -) - var ( - // logger is the package logger for the arbiter package. - logger logr.Logger //nolint:gochecknoglobals // package-level state for arbiter - // rootCmd holds the cobra root command for Usage access. //nolint:gochecknoglobals // package-level command for Usage access rootCmd *cobra.Command @@ -123,21 +124,17 @@ func Run(modules module.Modules, opts *Opts) error { SilenceUsage: true, } - errorLogger, err := abtrlog.Setup(&abtrlog.Opts{ - Verbosity: logVerbosity.Value(), - ErrorLogPath: opts.ErrorLogPath, - InfoLogPath: opts.InfoLogPath, - }) + infoLogger, errorLogger, err := setupLoggers(opts, logVerbosity.Value()) if err != nil { return fmt.Errorf("failed to build loggers: %w", err) } - logger = abtrlog.GetLogger() abtr := &abtr{ opts: opts, duration: defaultDuration, reportPath: defaultReportPath, interactive: defaultInteractive, + logger: infoLogger, errorLogger: errorLogger, } @@ -240,32 +237,31 @@ func (a *abtr) buildRunnerFlagSet() *pflag.FlagSet { return runnerFlagSet } -// run runs the input modules and starts generating traffic. Creates a traffic -// model based on the modules opertation settings. Aborts on SIGINT, SIGTERM -// or when the test duration runs out. Will immediately exit if any module -// returns an error from its call to Run(). func (a *abtr) run(metadata module.Metadata) error { - logger.Info("Starting modules") + a.logger.Info("Starting modules") - if err := startModules(metadata); err != nil { - logger.Error(err, "Start failure") + if err := a.startModules(metadata); err != nil { + a.logger.Error(err, "Start failure") return err } - logger.Info("All modules started") + a.logger.Info("All modules started") // Start signal interceptor for SIGINT and SIGTERM signalCtx, signalCancel := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer signalCancel() - reporter := a.setupReporter( + reporter, err := a.setupReporter( metadata, signalCtx, signalCancel, ) + if err != nil { + return err + } // Traffic context with a timeout of the test's >>> duration <<< timeoutCtx, timeoutCancel := context.WithTimeout(signalCtx, a.duration) defer timeoutCancel() - logger.Info("Traffic will run for: " + a.duration.String()) + a.logger.Info("Traffic will run for: " + a.duration.String()) // The reporter runs in its own context to allow reporting to finalize separately from traffic and module // shutdown. @@ -274,26 +270,26 @@ func (a *abtr) run(metadata module.Metadata) error { reporter.Start(reporterCtx) sched := traffic.New(&traffic.Opts{ - Logger: logger, + Logger: a.logger, WorkerLimit: workerLimit.Value(), }) // Run traffic. if err := sched.Run(timeoutCtx, metadata, reporter); err != nil { reporter.ReportError(err) // Report is done in case of early traffic failure, to highlight issues in the TUI. - logger.Error(err, "Failed to start traffic") + a.logger.Error(err, "Failed to start traffic") return err } - logger.Info("Awaiting completion (SIGINT/SIGTERM or duration timeout)") + a.logger.Info("Awaiting completion (SIGINT/SIGTERM or duration timeout)") select { case <-signalCtx.Done(): - logger.Info("Got stop signal") + a.logger.Info("Got stop signal") // no need to call timeoutCancel() here since the traffic context is a child of the signal context, // so will be cancelled automatically. // timeoutCancel() case <-timeoutCtx.Done(): - logger.Info("Deadline exceeded") + a.logger.Info("Deadline exceeded") // Needed to terminate the parent context, in case other's are reliant on it. signalCancel() } @@ -302,35 +298,35 @@ func (a *abtr) run(metadata module.Metadata) error { // to be returned at the end of the function. var stopErr error if stopErr = sched.Stop(); stopErr != nil { - logger.Error(stopErr, "Error when stopping traffic") + a.logger.Error(stopErr, "Error when stopping traffic") stopErr = fmt.Errorf("%w: traffic stop: %w", ErrStopping, stopErr) } // Now that traffic has been stopped, we can stop the reporter to allow it to finalise the report. reporterCancel() - logger.Info("Stopping modules") + a.logger.Info("Stopping modules") for _, m := range metadata { if moduleStopErr := m.Stop(); moduleStopErr != nil { - logger.Error(moduleStopErr, "Module stop reported an error", "module", m.Name()) + a.logger.Error(moduleStopErr, "Module stop reported an error", "module", m.Name()) stopErr = errors.Join(stopErr, fmt.Errorf("module %s stop: %w", m.Name(), moduleStopErr)) } } - logger.Info("Finalising report") + a.logger.Info("Finalising report") reporterStopErr := reporter.Finalise() if reporterStopErr != nil { - logger.Error(reporterStopErr, "Error when finalising report") + a.logger.Error(reporterStopErr, "Error when finalising report") stopErr = errors.Join(stopErr, fmt.Errorf("reporter stop: %w", reporterStopErr)) } return stopErr } -// Starts the input modules and logs any errors. -func startModules(meta []*module.Meta) error { +// startModules starts the input modules and logs any errors. +func (a *abtr) startModules(meta []*module.Meta) error { for _, m := range meta { - logger.Info("Starting", "module", m.Name()) + a.logger.Info("Starting", "module", m.Name()) if err := m.Run(); err != nil { return fmt.Errorf("failed to start module %s: %w", m.Name(), err) } @@ -339,7 +335,7 @@ func startModules(meta []*module.Meta) error { return nil } -// Creates the reporter(s). In interactive mode a collection reporter is +// setupReporter creates the reporter(s). In interactive mode a collection reporter is // returned that fans out to both a YAML reporter and the live TUI reporter. // trafficCancel is called by the interactive reporter when the user requests an early // stop (e.g. Ctrl-C inside the TUI), triggering the same shutdown path as @@ -349,11 +345,15 @@ func (a *abtr) setupReporter( metadata module.Metadata, //nolint:revive // the traffic context is special and not releated to the function really trafficCtx context.Context, trafficCancel func(), -) report.Reporter { - yamlR := yamlreport.New(&yamlreport.Opts{ +) (report.Reporter, error) { + yamlR, err := yamlreport.New(&yamlreport.Opts{ Path: a.reportPath, + Logger: a.logger, ErrorLogger: a.errorLogger, }) + if err != nil { + return nil, err + } if a.interactive { return collection.New( @@ -364,8 +364,43 @@ func (a *abtr) setupReporter( trafficCtx, trafficCancel, ), - ) + ), nil + } + + return yamlR, nil +} + +// setupLoggers initialises the info and error loggers from the provided options. +// It returns the info logger, the error logger, and any error encountered. +func setupLoggers(opts *Opts, verbosity int) (logr.Logger, logr.Logger, error) { + errorFile, err := os.Create(opts.ErrorLogPath) + if err != nil { + return logr.Logger{}, logr.Logger{}, err } - return yamlR + var infoLogger logr.Logger + if opts.InfoLogPath == "" { + infoLogger = logr.Discard() + } else { + infoFile, err := os.Create(opts.InfoLogPath) //nolint:govet // shad + if err != nil { + return logr.Logger{}, logr.Logger{}, err + } + + infoLogger = zerologr.New(&zerologr.Opts{ + Output: infoFile, + Console: true, + Caller: true, + V: verbosity, + }).WithName("abtr") + } + + errorLogger := zerologr.New(&zerologr.Opts{ + Output: errorFile, + Console: true, + Caller: false, + V: verbosity, + }) + + return infoLogger, errorLogger, nil } diff --git a/internal/log/log.go b/internal/log/log.go deleted file mode 100644 index 29760aa..0000000 --- a/internal/log/log.go +++ /dev/null @@ -1,80 +0,0 @@ -package abtrlog - -import ( - "os" - - "github.com/go-logr/logr" - "github.com/trebent/zerologr" -) - -type Opts struct { - Verbosity int - ErrorLogPath string - InfoLogPath string -} - -const ( - DefaultInfoLogPath = "info.log" - DefaultErrorLogPath = "error.log" -) - -//nolint:gochecknoglobals // package-level state for loggers -var logger logr.Logger - -// Setup sets up the loggers based on the provided options. It sets the package -// logger and returns an error logger for use by an arbiter reporter. -func Setup(opts *Opts) (logr.Logger, error) { - if opts == nil { - opts = &Opts{ - Verbosity: 0, - ErrorLogPath: "error.log", - InfoLogPath: "info.log", - } - } - - if opts.Verbosity < 0 { - opts.Verbosity = 0 - } - - if opts.ErrorLogPath == "" { - opts.ErrorLogPath = "error.log" - } - - // Set up the logger based on the provided options. - errorFile, err := os.Create(opts.ErrorLogPath) - if err != nil { - return logr.Logger{}, err - } - - if opts.InfoLogPath == "" { - logger = logr.Discard() - } else { - //nolint:govet // shad - infoFile, err := os.Create(opts.InfoLogPath) - if err != nil { - return logr.Logger{}, err - } - - logger = zerologr.New(&zerologr.Opts{ - Output: infoFile, - Console: true, - Caller: true, - V: opts.Verbosity, - }).WithName("abtr") - } - - errorLogger := zerologr.New(&zerologr.Opts{ - Output: errorFile, - Console: true, - Caller: false, - V: opts.Verbosity, - }) - - return errorLogger, nil -} - -// GetLogger returns the package logger for use by other components. Setup should be called first -// to initialize the logger before calling this function. -func GetLogger() logr.Logger { - return logger -} diff --git a/internal/otel/instrument.go b/internal/otel/instrument.go index 2f65bb3..8fdbd0b 100644 --- a/internal/otel/instrument.go +++ b/internal/otel/instrument.go @@ -4,7 +4,7 @@ import ( "context" "errors" - "github.com/trebent/zerologr" + "github.com/go-logr/logr" "go.opentelemetry.io/contrib/exporters/autoexport" "go.opentelemetry.io/contrib/instrumentation/runtime" "go.opentelemetry.io/otel" @@ -26,7 +26,7 @@ func Instrument( var shutdownFuncs []func(context.Context) error shutdown := func(ctx context.Context) error { - zerologr.Info("Shutting down OpenTelemetry SDK") + logr.FromContextOrDiscard(ctx).Info("Shutting down OpenTelemetry SDK") var err error for _, fn := range shutdownFuncs { diff --git a/pkg/report/yaml/yaml.go b/pkg/report/yaml/yaml.go index d2ad747..d8cde44 100644 --- a/pkg/report/yaml/yaml.go +++ b/pkg/report/yaml/yaml.go @@ -2,11 +2,11 @@ package yamlreport import ( "context" + "errors" "os" "time" "github.com/go-logr/logr" - abtrlog "github.com/maansaake/arbiter/internal/log" "github.com/maansaake/arbiter/pkg/module" "github.com/maansaake/arbiter/pkg/report" "gopkg.in/yaml.v3" @@ -23,6 +23,8 @@ type ( // The buffer size sets the number of buffered report calls that are yet // to be handled. Values < 1 will be ignored. Buffer int + // Logger is the logger used for info-level logging by the reporter. + Logger logr.Logger // ErrorLogger is a logger for the reporter to log errors to. ErrorLogger logr.Logger } @@ -32,6 +34,8 @@ type ( path string // The YAML report. report *Report + // logger is used for info-level logging. + logger logr.Logger // errorLogger is used to log errors from failed operations. errorLogger logr.Logger // Synchronizer channel to limit access to the report to 1 thread. Also @@ -41,16 +45,15 @@ type ( } ) -var ( - logger logr.Logger //nolint:gochecknoglobals // package-level state for YAML reporter - _ report.Reporter = &reporter{} -) +var _ report.Reporter = &reporter{} const yamlIndent = 2 // New creates a new YAML reporter. -func New(opts *Opts) report.Reporter { - logger = abtrlog.GetLogger() +func New(opts *Opts) (report.Reporter, error) { + if opts.Logger.GetSink() == nil { + return nil, errors.New("logger must be provided") + } var start time.Time var buffer int @@ -71,18 +74,19 @@ func New(opts *Opts) report.Reporter { Start: start, Modules: make(map[string]*ModuleReport), }, + logger: opts.Logger, errorLogger: opts.ErrorLogger, path: opts.Path, synchronizer: make(chan func(), buffer), stopped: make(chan struct{}), } - return reporter + return reporter, nil } // Start the YAML reporter and run until the context is cancelled. func (r *reporter) Start(ctx context.Context) { - logger.Info("Starting reporter") + r.logger.Info("Starting reporter") go func() { for { @@ -90,7 +94,7 @@ func (r *reporter) Start(ctx context.Context) { case f := <-r.synchronizer: f() case <-ctx.Done(): - logger.Info("Reporter context closed, flushing synchronizer", "len", len(r.synchronizer)) + r.logger.Info("Reporter context closed, flushing synchronizer", "len", len(r.synchronizer)) // TODO: test buffer emptying out: @@ -103,7 +107,7 @@ func (r *reporter) Start(ctx context.Context) { break out } } - logger.Info("Synchronizer flushed, stopping reporter") + r.logger.Info("Synchronizer flushed, stopping reporter") close(r.stopped) return } @@ -127,7 +131,7 @@ func (r *reporter) ReportOp(mod, op string, res *module.Result, err error) { func (r *reporter) Finalise() error { // Await synchronizer, no value expected <-r.stopped - logger.Info("Synchronizer stopped, writing report") + r.logger.Info("Synchronizer stopped, writing report") r.report.End = time.Now() r.report.Duration = r.report.End.Sub(r.report.Start) diff --git a/pkg/report/yaml/yaml_test.go b/pkg/report/yaml/yaml_test.go index d3215d7..73f38bf 100644 --- a/pkg/report/yaml/yaml_test.go +++ b/pkg/report/yaml/yaml_test.go @@ -7,13 +7,21 @@ import ( "testing" "time" + "github.com/go-logr/logr/funcr" "github.com/maansaake/arbiter/pkg/module" "gopkg.in/yaml.v3" ) func TestYAMLReporter(t *testing.T) { reportPath := "report.yaml" - i := New(&Opts{Buffer: 100, Path: reportPath}) + i, err := New(&Opts{ + Buffer: 100, + Path: reportPath, + Logger: funcr.New(func(_, _ string) {}, funcr.Options{}), + }) + if err != nil { + t.Fatal("unexpected error creating reporter:", err) + } yamlReporter := i.(*reporter) ctx := context.Background() @@ -34,7 +42,7 @@ func TestYAMLReporter(t *testing.T) { cancel() - err := yamlReporter.Finalise() + err = yamlReporter.Finalise() if err != nil { t.Fatal("error on finalise", err) } From ac1d8331c1d02c847a19a6451214eb6aebc53c42 Mon Sep 17 00:00:00 2001 From: GitHub Copilot Date: Sun, 24 May 2026 05:12:38 +0200 Subject: [PATCH 2/3] refactor: remove error return from yamlreport.New() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logger is passed by value, not a pointer, so a zero-value logger is always 'provided' — validating the sink is not meaningful. Revert New() to returning report.Reporter directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- arbiter.go | 16 +++++----------- pkg/report/yaml/yaml.go | 9 ++------- pkg/report/yaml/yaml_test.go | 7 ++----- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/arbiter.go b/arbiter.go index 82630f4..768bf23 100644 --- a/arbiter.go +++ b/arbiter.go @@ -250,13 +250,10 @@ func (a *abtr) run(metadata module.Metadata) error { signalCtx, signalCancel := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer signalCancel() - reporter, err := a.setupReporter( + reporter := a.setupReporter( metadata, signalCtx, signalCancel, ) - if err != nil { - return err - } // Traffic context with a timeout of the test's >>> duration <<< timeoutCtx, timeoutCancel := context.WithTimeout(signalCtx, a.duration) @@ -345,15 +342,12 @@ func (a *abtr) setupReporter( metadata module.Metadata, //nolint:revive // the traffic context is special and not releated to the function really trafficCtx context.Context, trafficCancel func(), -) (report.Reporter, error) { - yamlR, err := yamlreport.New(&yamlreport.Opts{ +) report.Reporter { + yamlR := yamlreport.New(&yamlreport.Opts{ Path: a.reportPath, Logger: a.logger, ErrorLogger: a.errorLogger, }) - if err != nil { - return nil, err - } if a.interactive { return collection.New( @@ -364,10 +358,10 @@ func (a *abtr) setupReporter( trafficCtx, trafficCancel, ), - ), nil + ) } - return yamlR, nil + return yamlR } // setupLoggers initialises the info and error loggers from the provided options. diff --git a/pkg/report/yaml/yaml.go b/pkg/report/yaml/yaml.go index d8cde44..0ff8084 100644 --- a/pkg/report/yaml/yaml.go +++ b/pkg/report/yaml/yaml.go @@ -2,7 +2,6 @@ package yamlreport import ( "context" - "errors" "os" "time" @@ -50,11 +49,7 @@ var _ report.Reporter = &reporter{} const yamlIndent = 2 // New creates a new YAML reporter. -func New(opts *Opts) (report.Reporter, error) { - if opts.Logger.GetSink() == nil { - return nil, errors.New("logger must be provided") - } - +func New(opts *Opts) report.Reporter { var start time.Time var buffer int if opts.Buffer > 0 { @@ -81,7 +76,7 @@ func New(opts *Opts) (report.Reporter, error) { stopped: make(chan struct{}), } - return reporter, nil + return reporter } // Start the YAML reporter and run until the context is cancelled. diff --git a/pkg/report/yaml/yaml_test.go b/pkg/report/yaml/yaml_test.go index 73f38bf..7a070a0 100644 --- a/pkg/report/yaml/yaml_test.go +++ b/pkg/report/yaml/yaml_test.go @@ -14,14 +14,11 @@ import ( func TestYAMLReporter(t *testing.T) { reportPath := "report.yaml" - i, err := New(&Opts{ + i := New(&Opts{ Buffer: 100, Path: reportPath, Logger: funcr.New(func(_, _ string) {}, funcr.Options{}), }) - if err != nil { - t.Fatal("unexpected error creating reporter:", err) - } yamlReporter := i.(*reporter) ctx := context.Background() @@ -42,7 +39,7 @@ func TestYAMLReporter(t *testing.T) { cancel() - err = yamlReporter.Finalise() + err := yamlReporter.Finalise() if err != nil { t.Fatal("error on finalise", err) } From 8950af4fb602f172c2d7c8b69b383a74e1ef6aee Mon Sep 17 00:00:00 2001 From: GitHub Copilot Date: Sun, 24 May 2026 05:15:18 +0200 Subject: [PATCH 3/3] fix: correct indentation in yaml reporter (goimports) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/report/yaml/yaml.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/report/yaml/yaml.go b/pkg/report/yaml/yaml.go index 0ff8084..acae83e 100644 --- a/pkg/report/yaml/yaml.go +++ b/pkg/report/yaml/yaml.go @@ -102,7 +102,7 @@ func (r *reporter) Start(ctx context.Context) { break out } } - r.logger.Info("Synchronizer flushed, stopping reporter") + r.logger.Info("Synchronizer flushed, stopping reporter") close(r.stopped) return }