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 <manugupt1@gmail.com>
gomods · Jan 4, 2024 · 7284004 · 7284004
1 parent 43d56f0
commit 7284004
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 55 deletions.
diff --git a/cmd/proxy/actions/app.go b/cmd/proxy/actions/app.go
@@ -3,15 +3,13 @@ package actions
 import (
 	"fmt"
 	"net/http"
-	"os"
 
 	"github.com/gomods/athens/pkg/config"
 	"github.com/gomods/athens/pkg/log"
 	mw "github.com/gomods/athens/pkg/middleware"
 	"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
@@ -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
@@ -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,42 +59,45 @@ 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.
 		}()
 	}
 
 	// Unix socket configuration, if available, takes precedence over TCP port configuration.
 	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