From e45ccce0a0fbf66f898d6b2d7497f0c05885174f Mon Sep 17 00:00:00 2001 From: Keepers Date: Mon, 4 Nov 2024 16:26:00 -0700 Subject: [PATCH] change file setting semantics 1. Always default logging to stderr. 2. allow users to overwrite output to stdout or a file. However, to provide defaults, this setting is unexported and has no hooks for globally declared files or env varibles. Users must instead pass a file to the settings struct via a method instead. This PR is to solve concerns about the capacity for discrete logging files to appear on the same machine when the intent is for initialization to use the same file every time. It requires users to be more aware of the file locations they're working with in order to set up clog correctly. --- clog/logger.go | 13 +++-- clog/logger_test.go | 2 - clog/settings.go | 112 ++++++++++++++---------------------------- clog/settings_test.go | 47 ++++++++++++++++++ 4 files changed, 94 insertions(+), 80 deletions(-) create mode 100644 clog/settings_test.go diff --git a/clog/logger.go b/clog/logger.go index 264a6fa..1d7e387 100644 --- a/clog/logger.go +++ b/clog/logger.go @@ -52,11 +52,18 @@ func genLogger(set Settings) *zap.SugaredLogger { } ) + // set the file handling + toFile := Stderr + + if len(set.fileOverride) > 0 { + toFile = set.fileOverride + } + switch set.Format { // JSON means each row should appear as a single json object. case FormatToJSON: zcfg = setLevel(zap.NewProductionConfig(), set.Level) - zcfg.OutputPaths = []string{set.File} + zcfg.OutputPaths = []string{toFile} // by default we'll use the columnar non-json format, which uses tab // separated values within each line, and may contain multiple json objs. default: @@ -65,12 +72,12 @@ func genLogger(set Settings) *zap.SugaredLogger { zcfg.EncoderConfig.EncodeTime = zapcore.TimeEncoderOfLayout(time.StampMilli) // when printing to stdout/stderr, colorize things! - if set.File == Stderr || set.File == Stdout { + if toFile == Stderr || toFile == Stdout { zcfg.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder } } - zcfg.OutputPaths = []string{set.File} + zcfg.OutputPaths = []string{toFile} zlog, err := zcfg.Build(zopts...) if err != nil { diff --git a/clog/logger_test.go b/clog/logger_test.go index 4d7cc9e..2c7b3c8 100644 --- a/clog/logger_test.go +++ b/clog/logger_test.go @@ -20,14 +20,12 @@ func (suite *LoggerUnitSuite) TestSettings_ensureDefaults() { t := suite.T() s := clog.Settings{} - require.Empty(t, s.File, "file") require.Empty(t, s.Level, "level") require.Empty(t, s.Format, "format") require.Empty(t, s.SensitiveInfoHandling, "piialg") require.Empty(t, s.OnlyLogDebugIfContainsLabel, "debug filter") s = s.EnsureDefaults() - require.NotEmpty(t, s.File, "file") require.NotEmpty(t, s.Level, "level") require.NotEmpty(t, s.Format, "format") require.NotEmpty(t, s.SensitiveInfoHandling, "piialg") diff --git a/clog/settings.go b/clog/settings.go index 773c6ff..d986945 100644 --- a/clog/settings.go +++ b/clog/settings.go @@ -3,7 +3,6 @@ package clog import ( "os" "path/filepath" - "time" "golang.org/x/exp/slices" @@ -51,25 +50,20 @@ const ( // configuration // --------------------------------------------------- -// Default location for writing log files. -var defaultLogFileDir = filepath.Join(os.Getenv("HOME"), "Library", "Logs") - -// ResolvedLogFile is the first log file established by the caller. -// It gets eagerly populated on the first act of ensuring settings -// defaults, which normally occurs during the Init call. -// -// If Init gets called more than once, or different settings are -// ensured, it's possible to override this value by manually specifying -// the log file in the settings used for that action. But if no file -// is provided, the default will fall back to this resolved file first. -var ResolvedLogFile string - // Settings records the user's preferred logging settings. type Settings struct { - // core settings - File string // what file to log to (alt: stderr, stdout) - Format logFormat // whether to format as text (console) or json (cloud) - Level logLevel // what level to log at + // the log file isn't exposed to end users because we + // want to ensure a default of StdErr until they call + // one of the file override hooks. + fileOverride string + + // Format defines the output structure, standard design is + // as text (human-at-a-console) or json (automation). + Format logFormat + // Level determines the minimum logging level. Anything + // below this level (following standard semantics) will + // not get logged. + Level logLevel // more fiddly bits SensitiveInfoHandling sensitiveInfoHandlingAlgo // how to obscure pii @@ -80,6 +74,31 @@ type Settings struct { OnlyLogDebugIfContainsLabel []string } +// LogToStdOut swaps the log output from Stderr to Stdout. +func (s Settings) LogToStdOut() Settings { + s.fileOverride = Stdout + return s +} + +// LogToFile defines a system file to write all logs onto. +func (s Settings) LogToFile(pathToFile string) (Settings, error) { + if len(pathToFile) == 0 { + return s, clues.New("missing filepath for logging") + } + + logdir := filepath.Dir(pathToFile) + + err := os.MkdirAll(logdir, 0o755) + if err != nil { + return s, clues.Wrap(err, "ensuring log file dir exists"). + With("log_dir", logdir) + } + + s.fileOverride = pathToFile + + return s, nil +} + // EnsureDefaults sets any non-populated settings to their default value. // exported for testing without circular dependencies. func (s Settings) EnsureDefaults() Settings { @@ -100,66 +119,9 @@ func (s Settings) EnsureDefaults() Settings { set.SensitiveInfoHandling = ShowSensitiveInfoInPlainText } - if len(set.File) == 0 { - set.File = GetLogFileOrDefault("") - } - - if len(ResolvedLogFile) == 0 { - ResolvedLogFile = set.File - } - return set } -// Returns the default location for log file storage. -func defaultLogLocation() string { - return filepath.Join( - defaultLogFileDir, - "clog", - time.Now().UTC().Format("2006-01-02T15-04-05Z")+".log") -} - -// GetLogFileOrDefault finds the log file in the users local system. -// Uses the env var declaration, if populated, else defaults to stderr. -// If this has already been called once before, uses the result of that -// prior call. -func GetLogFileOrDefault(useThisFile string) string { - if len(ResolvedLogFile) > 0 { - return ResolvedLogFile - } - - // start by preferring the file given to us by the caller. - r := useThisFile - - // if no file was provided, look for a configured location using - // the default ENV. - if len(r) == 0 { - r = os.Getenv(clogLogFileEnv) - } - - // if no file was provided, fall back to the default file location. - if len(r) == 0 { - r = defaultLogLocation() - } - - // direct to Stdout if provided '-'. - if r == "-" { - r = Stdout - } - - // if outputting to a file, make sure we can access the file. - if r != Stdout && r != Stderr { - logdir := filepath.Dir(r) - - err := os.MkdirAll(logdir, 0o755) - if err != nil { - return Stderr - } - } - - return r -} - func setCluesSecretsHash(alg sensitiveInfoHandlingAlgo) { switch alg { case HashSensitiveInfo: diff --git a/clog/settings_test.go b/clog/settings_test.go new file mode 100644 index 0000000..97fdede --- /dev/null +++ b/clog/settings_test.go @@ -0,0 +1,47 @@ +package clog + +import ( + "path/filepath" + "testing" + + "github.com/alcionai/clues" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSettings_LogToFile(t *testing.T) { + tempDir := t.TempDir() + + table := []struct { + name string + input string + expectErr require.ErrorAssertionFunc + expectOverride string + }{ + { + name: "empty", + input: "", + expectErr: require.Error, + expectOverride: "", + }, + { + name: "doesn't exist", + input: filepath.Join(tempDir, "foo", "bar", "baz", "log.log"), + expectErr: require.NoError, + expectOverride: filepath.Join(tempDir, "foo", "bar", "baz", "log.log"), + }, + { + name: "exists", + input: filepath.Join(tempDir, "log.log"), + expectErr: require.NoError, + expectOverride: filepath.Join(tempDir, "log.log"), + }, + } + for _, test := range table { + t.Run(test.name, func(t *testing.T) { + set, err := Settings{}.LogToFile(test.input) + test.expectErr(t, err, clues.ToCore(err)) + assert.Equal(t, test.expectOverride, set.fileOverride) + }) + } +}