Write logs to disk by default #2082

meain · 2023-01-10T09:54:30Z

Description

This changes the behavior of logs and writes it to disk instead of stdout by default. It also adds a new flag --log-file to specify the filename for the log file.

Does this PR need a docs update or release note?

✅ Yes, it's included
🕐 Yes, but in a later PR
⛔ No

Type of change

Issue(s)

fixes Log to disk by default #2076

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

meain · 2023-01-10T10:02:52Z

src/pkg/logger/logger.go


 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 )

+var defaultLogLocation = "corso-" + time.Now().UTC().Format("2006-01-02T15-04-05Z") + ".log"


Default log location is currently $PWD/corso-<timestamp>.log.

Works for me. @gmatev you happy with this? Alternatively, we could co-locate it with the config file location by default. Or perhaps a ~/.corso folder.

FYI, if we are changing to a different location, we will also have to consider what it will be in Windows.

Another option would be stick to XDG base directory specification in linux and store it under $XDG_STATE_HOME which would be $HOME/.loca/state/corso/corso-<timestamp>.log.

As a follow-up, we should probably consider moving the config file to $XDG_CONFIG_HOME, which would be $HOME/.config/corso/config.toml.

There is a relatively vocal group of people(myself kinda included) advocating against polluting home directory on linux: https://www.reddit.com/r/linux/comments/971m0z/im_tired_of_folders_littering_my_home_directory/

Agree that this should not be home directory or local directory. Suggest we emulate https://kopia.io/docs/advanced/logging/

meain · 2023-01-10T10:03:34Z

src/pkg/logger/logger.go

+		return "info", defaultLogLocation
+	}
+
+	if logfile == "-" {


They can still use cobra specific stdout, but this is a more common convention.

ryanfkeepers

some concerns about the writer itself, otherwise looks good.

ryanfkeepers · 2023-01-10T16:27:01Z

src/pkg/logger/logger.go


 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 )

+var defaultLogLocation = "corso-" + time.Now().UTC().Format("2006-01-02T15-04-05Z") + ".log"


Suggested change

var defaultLogLocation = "corso-" + time.Now().UTC().Format("2006-01-02T15-04-05Z") + ".log"

var defaultLogLocation = "corso-" + common.FormatNow(common.SimpleDateTime) + ".log"

: in common.SimpleDateTime is an invalid char for files on Windows(zap also does not seem to like it). This format is similar to that but : replaced with -. I can create a new constant if you think that is necessary.

ryanfkeepers · 2023-01-10T16:28:01Z

src/pkg/logger/logger.go


 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 )

+var defaultLogLocation = "corso-" + time.Now().UTC().Format("2006-01-02T15-04-05Z") + ".log"


Works for me. @gmatev you happy with this? Alternatively, we could co-locate it with the config file location by default. Or perhaps a ~/.corso folder.

ryanfkeepers · 2023-01-10T16:35:02Z

src/pkg/logger/logger.go

+	}
+
+	if logfile == "-" {
+		logfile = "stdout"


This should be stderr, not stdout. Stdout will cause logging to interfere with end-of-run application prints, preventing users from piping the results to scripts (ex: corso ... --json | jq {...}).

This is more of a extra option as - is a common convention for stdin / stdout. If a user wants to pipe to stderr they can use "stderr" as the filename.

ryanfkeepers

Approving to unblock this. Prior comments still need to be addressed.

ntolia

Please also:

Modify docs/support/bugs-and-features/ to include information on where to find the log file to share
Modify docs/setup/configuration/ and add a new section (Log files) and specify the default location as well as a reference to the flag.

Changes made

aviator-app · 2023-01-13T07:23:50Z

Aviator status

Aviator will automatically update this comment as the status of the PR changes.

This PR was merged using Aviator.

sonarcloud · 2023-01-13T07:24:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

vkamra · 2023-01-13T07:52:40Z

website/docs/setup/configuration.md

+Log file location can be overridden by setting the `--log-file` flag.
+
+:::info
+You can use `stdout` or `stderr` as the `--log-file` location to redirect the logs to "stdout" and "stderr" respectively.


Instead of You can use ... - change to Use stdout ...

So "Use stdout or stderr as the --log-file location to redirect the logs to "stdout" or "stderr".

Log to disk by default

d5eaf6d

meain requested review from vkamra and ryanfkeepers January 10, 2023 09:54

meain self-assigned this Jan 10, 2023

meain temporarily deployed to Testing January 10, 2023 09:54 — with GitHub Actions Inactive

Add changelog entry

af48b2e

meain temporarily deployed to Testing January 10, 2023 09:55 — with GitHub Actions Inactive

meain temporarily deployed to Testing January 10, 2023 09:56 — with GitHub Actions Inactive

meain had a problem deploying to Testing January 10, 2023 09:56 — with GitHub Actions Failure

meain had a problem deploying to Testing January 10, 2023 09:56 — with GitHub Actions Error

Add stdout to vale accept list

779e995

meain temporarily deployed to Testing January 10, 2023 10:01 — with GitHub Actions Inactive

meain temporarily deployed to Testing January 10, 2023 10:02 — with GitHub Actions Inactive

meain commented Jan 10, 2023

View reviewed changes

meain temporarily deployed to Testing January 10, 2023 10:06 — with GitHub Actions Inactive

ryanfkeepers reviewed Jan 10, 2023

View reviewed changes

ryanfkeepers approved these changes Jan 10, 2023

View reviewed changes

ntolia previously requested changes Jan 11, 2023

View reviewed changes

Fix fallback for logger creation

655e631

meain temporarily deployed to Testing January 11, 2023 09:26 — with GitHub Actions Inactive

meain had a problem deploying to Testing January 11, 2023 09:27 — with GitHub Actions Failure

meain force-pushed the logs-to-disk branch from a52acf4 to 1bc464a Compare January 11, 2023 11:43

meain temporarily deployed to Testing January 11, 2023 11:44 — with GitHub Actions Inactive

meain had a problem deploying to Testing January 11, 2023 11:44 — with GitHub Actions Error

meain had a problem deploying to Testing January 11, 2023 11:44 — with GitHub Actions Failure

Fix lint errors

20e3476

meain temporarily deployed to Testing January 11, 2023 11:49 — with GitHub Actions Inactive

meain temporarily deployed to Testing January 11, 2023 11:50 — with GitHub Actions Inactive

vkamra approved these changes Jan 13, 2023

View reviewed changes

meain added the mergequeue label Jan 13, 2023

Merge branch 'main' into logs-to-disk

56f8d72

aviator-app bot temporarily deployed to Testing January 13, 2023 07:23 Inactive

aviator-app bot temporarily deployed to Testing January 13, 2023 07:24 Inactive

aviator-app bot merged commit 01d8d08 into main Jan 13, 2023

aviator-app bot deleted the logs-to-disk branch January 13, 2023 07:44

vkamra reviewed Jan 13, 2023

View reviewed changes

meain mentioned this pull request Jan 13, 2023

log observed messages #2073

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write logs to disk by default #2082

Write logs to disk by default #2082

meain commented Jan 10, 2023 •

edited

Loading

meain Jan 10, 2023

ryanfkeepers Jan 10, 2023

meain Jan 11, 2023

meain Jan 11, 2023

vkamra Jan 11, 2023

meain Jan 10, 2023

ryanfkeepers left a comment

ryanfkeepers Jan 10, 2023

meain Jan 11, 2023 •

edited

Loading

ryanfkeepers Jan 10, 2023

ryanfkeepers Jan 10, 2023

meain Jan 11, 2023

ryanfkeepers left a comment

ntolia left a comment

aviator-app bot commented Jan 13, 2023 •

edited

Loading

sonarcloud bot commented Jan 13, 2023

vkamra Jan 13, 2023

vkamra Jan 13, 2023

	var defaultLogLocation = "corso-" + time.Now().UTC().Format("2006-01-02T15-04-05Z") + ".log"
	var defaultLogLocation = "corso-" + common.FormatNow(common.SimpleDateTime) + ".log"

Write logs to disk by default #2082

Write logs to disk by default #2082

Conversation

meain commented Jan 10, 2023 • edited Loading

Description

Does this PR need a docs update or release note?

Type of change

Issue(s)

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanfkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meain Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanfkeepers left a comment

Choose a reason for hiding this comment

ntolia left a comment

Choose a reason for hiding this comment

aviator-app bot commented Jan 13, 2023 • edited Loading

Aviator status

sonarcloud bot commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meain commented Jan 10, 2023 •

edited

Loading

meain Jan 11, 2023 •

edited

Loading

aviator-app bot commented Jan 13, 2023 •

edited

Loading