From 7284004d0529c13fc52f0309a47689e9b48cff97 Mon Sep 17 00:00:00 2001 From: Mike Seplowitz Date: Thu, 4 Jan 2024 05:11:29 -0500 Subject: [PATCH] Set up and use logrus logger in main (#1819) * Set up and use logrus logger in main Also return errors more consistently from other functions. * Updated wording styles * Prefer human-readable descriptions to method names * Wrapped errors use gerund forms, e.g. "doing x: %w" * Log traces start with a capital letter * Fix style on standard log failure cases --------- Co-authored-by: Manu Gupta --- cmd/proxy/actions/app.go | 50 ++++++++++++++------------------------- cmd/proxy/actions/auth.go | 20 +++++++++------- cmd/proxy/main.go | 40 ++++++++++++++++++++----------- 3 files changed, 55 insertions(+), 55 deletions(-) diff --git a/cmd/proxy/actions/app.go b/cmd/proxy/actions/app.go index e37022469..74df44abf 100644 --- a/cmd/proxy/actions/app.go +++ b/cmd/proxy/actions/app.go @@ -3,7 +3,6 @@ package actions import ( "fmt" "net/http" - "os" "github.com/gomods/athens/pkg/config" "github.com/gomods/athens/pkg/log" @@ -11,7 +10,6 @@ import ( "github.com/gomods/athens/pkg/module" "github.com/gomods/athens/pkg/observ" "github.com/gorilla/mux" - "github.com/sirupsen/logrus" "github.com/unrolled/secure" "go.opencensus.io/plugin/ochttp" ) @@ -22,38 +20,33 @@ const Service = "proxy" // App is where all routes and middleware for the proxy // should be defined. This is the nerve center of your // application. -func App(conf *config.Config) (http.Handler, error) { - // ENV is used to help switch settings based on where the - // application is being run. Default is "development". - ENV := conf.GoEnv - +func App(logger *log.Logger, conf *config.Config) (http.Handler, error) { if conf.GithubToken != "" { if conf.NETRCPath != "" { - fmt.Println("Cannot provide both GithubToken and NETRCPath. Only provide one.") - os.Exit(1) + return nil, fmt.Errorf("cannot provide both GithubToken and NETRCPath") } - netrcFromToken(conf.GithubToken) + if err := netrcFromToken(conf.GithubToken); err != nil { + return nil, fmt.Errorf("creating netrc from token: %w", err) + } } // mount .netrc to home dir // to have access to private repos. - initializeAuthFile(conf.NETRCPath) + if err := initializeAuthFile(conf.NETRCPath); err != nil { + return nil, fmt.Errorf("initializing auth file from netrc: %w", err) + } // mount .hgrc to home dir // to have access to private repos. - initializeAuthFile(conf.HGRCPath) - - logLvl, err := logrus.ParseLevel(conf.LogLevel) - if err != nil { - return nil, err + if err := initializeAuthFile(conf.HGRCPath); err != nil { + return nil, fmt.Errorf("initializing auth file from hgrc: %w", err) } - lggr := log.New(conf.CloudRuntime, logLvl) r := mux.NewRouter() r.Use( mw.WithRequestID, - mw.LogEntryMiddleware(lggr), + mw.LogEntryMiddleware(logger), mw.RequestLogger, secure.New(secure.Options{ SSLRedirect: conf.ForceSSL, @@ -82,10 +75,10 @@ func App(conf *config.Config) (http.Handler, error) { conf.TraceExporter, conf.TraceExporterURL, Service, - ENV, + conf.GoEnv, ) if err != nil { - lggr.Infof("%s", err) + logger.Info(err) } else { defer flushTraces() } @@ -95,7 +88,7 @@ func App(conf *config.Config) (http.Handler, error) { // was specified by the user. flushStats, err := observ.RegisterStatsExporter(r, conf.StatsExporter, Service) if err != nil { - lggr.Infof("%s", err) + logger.Info(err) } else { defer flushStats() } @@ -108,7 +101,7 @@ func App(conf *config.Config) (http.Handler, error) { if !conf.FilterOff() { mf, err := module.NewFilter(conf.FilterFile) if err != nil { - lggr.Fatal(err) + return nil, fmt.Errorf("creating new filter: %w", err) } r.Use(mw.NewFilterMiddleware(mf, conf.GlobalEndpoint)) } @@ -126,22 +119,15 @@ func App(conf *config.Config) (http.Handler, error) { store, err := GetStorage(conf.StorageType, conf.Storage, conf.TimeoutDuration(), client) if err != nil { - err = fmt.Errorf("error getting storage configuration: %w", err) - return nil, err + return nil, fmt.Errorf("getting storage configuration: %w", err) } proxyRouter := r if subRouter != nil { proxyRouter = subRouter } - if err := addProxyRoutes( - proxyRouter, - store, - lggr, - conf, - ); err != nil { - err = fmt.Errorf("error adding proxy routes: %w", err) - return nil, err + if err := addProxyRoutes(proxyRouter, store, logger, conf); err != nil { + return nil, fmt.Errorf("adding proxy routes: %w", err) } h := &ochttp.Handler{ diff --git a/cmd/proxy/actions/auth.go b/cmd/proxy/actions/auth.go index c4daedc6b..455d2febf 100644 --- a/cmd/proxy/actions/auth.go +++ b/cmd/proxy/actions/auth.go @@ -2,7 +2,6 @@ package actions import ( "fmt" - "log" "os" "path/filepath" "runtime" @@ -14,40 +13,43 @@ import ( // initializeAuthFile checks if provided auth file is at a pre-configured path // and moves to home directory -- note that this will override whatever // .netrc/.hgrc file you have in your home directory. -func initializeAuthFile(path string) { +func initializeAuthFile(path string) error { if path == "" { - return + return nil } fileBts, err := os.ReadFile(filepath.Clean(path)) if err != nil { - log.Fatalf("could not read %s: %v", path, err) + return fmt.Errorf("reading %s: %w", path, err) } hdir, err := homedir.Dir() if err != nil { - log.Fatalf("could not get homedir: %v", err) + return fmt.Errorf("getting home dir: %w", err) } fileName := transformAuthFileName(filepath.Base(path)) rcp := filepath.Join(hdir, fileName) if err := os.WriteFile(rcp, fileBts, 0o600); err != nil { - log.Fatalf("could not write to file: %v", err) + return fmt.Errorf("writing to auth file: %w", err) } + + return nil } // netrcFromToken takes a github token and creates a .netrc // file for you, overriding whatever might be already there. -func netrcFromToken(tok string) { +func netrcFromToken(tok string) error { fileContent := fmt.Sprintf("machine github.com login %s\n", tok) hdir, err := homedir.Dir() if err != nil { - log.Fatalf("netrcFromToken: could not get homedir: %v", err) + return fmt.Errorf("getting homedir: %w", err) } rcp := filepath.Join(hdir, getNETRCFilename()) if err := os.WriteFile(rcp, []byte(fileContent), 0o600); err != nil { - log.Fatalf("netrcFromToken: could not write to file: %v", err) + return fmt.Errorf("writing to netrc file: %w", err) } + return nil } func transformAuthFileName(authFileName string) string { diff --git a/cmd/proxy/main.go b/cmd/proxy/main.go index ffbc327a3..3b55c0d05 100644 --- a/cmd/proxy/main.go +++ b/cmd/proxy/main.go @@ -5,7 +5,7 @@ import ( "errors" "flag" "fmt" - "log" + stdlog "log" "net" "net/http" _ "net/http/pprof" @@ -17,6 +17,8 @@ import ( "github.com/gomods/athens/internal/shutdown" "github.com/gomods/athens/pkg/build" "github.com/gomods/athens/pkg/config" + athenslog "github.com/gomods/athens/pkg/log" + "github.com/sirupsen/logrus" ) var ( @@ -32,12 +34,19 @@ func main() { } conf, err := config.Load(*configFile) if err != nil { - log.Fatalf("could not load config file: %v", err) + stdlog.Fatalf("Could not load config file: %v", err) } - handler, err := actions.App(conf) + logLvl, err := logrus.ParseLevel(conf.LogLevel) if err != nil { - log.Fatal(err) + stdlog.Fatalf("Could not parse log level %q: %v", conf.LogLevel, err) + } + + logger := athenslog.New(conf.CloudRuntime, logLvl) + + handler, err := actions.App(logger, conf) + if err != nil { + logger.WithError(err).Fatal("Could not create App") } srv := &http.Server{ @@ -50,23 +59,24 @@ func main() { sigint := make(chan os.Signal, 1) signal.Notify(sigint, shutdown.GetSignals()...) s := <-sigint - log.Printf("Received signal (%s): Shutting down server", s) + logger.WithField("signal", s).Infof("Received signal, shutting down server") // We received an interrupt signal, shut down. ctx, cancel := context.WithTimeout(context.Background(), time.Second*time.Duration(conf.ShutdownTimeout)) defer cancel() if err := srv.Shutdown(ctx); err != nil { - log.Fatal(err) + logger.WithError(err).Fatal("Could not shut down server") } close(idleConnsClosed) }() if conf.EnablePprof { go func() { - // pprof to be exposed on a different port than the application for security matters, not to expose profiling data and avoid DoS attacks (profiling slows down the service) + // pprof to be exposed on a different port than the application for security matters, + // not to expose profiling data and avoid DoS attacks (profiling slows down the service) // https://www.farsightsecurity.com/txt-record/2016/10/28/cmikk-go-remote-profiling/ - log.Printf("Starting `pprof` at port %v", conf.PprofPort) - log.Fatal(http.ListenAndServe(conf.PprofPort, nil)) //nolint:gosec // This should not be exposed to the world. + logger.WithField("port", conf.PprofPort).Infof("starting pprof") + logger.Fatal(http.ListenAndServe(conf.PprofPort, nil)) //nolint:gosec // This should not be exposed to the world. }() } @@ -74,18 +84,20 @@ func main() { var ln net.Listener if conf.UnixSocket != "" { - log.Printf("Starting application at Unix domain socket %v", conf.UnixSocket) + logger := logger.WithField("unixSocket", conf.UnixSocket) + logger.Info("Starting application") ln, err = net.Listen("unix", conf.UnixSocket) if err != nil { - log.Fatalf("error listening on Unix domain socket %v: %v", conf.UnixSocket, err) + logger.WithError(err).Fatal("Could not listen on Unix domain socket") } } else { - log.Printf("Starting application at port %v", conf.Port) + logger := logger.WithField("tcpPort", conf.Port) + logger.Info("Starting application") ln, err = net.Listen("tcp", conf.Port) if err != nil { - log.Fatalf("error listening on TCP port %v: %v", conf.Port, err) + logger.WithError(err).Fatal("Could not listen on TCP port") } } @@ -96,7 +108,7 @@ func main() { } if !errors.Is(err, http.ErrServerClosed) { - log.Fatal(err) + logger.WithError(err).Fatal("Could not start server") } <-idleConnsClosed