Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/geth: enable log rotation #26843

Merged
merged 3 commits into from
Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ profile.cov
/dashboard/assets/package-lock.json

**/yarn-error.log
logs/
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ require (
golang.org/x/text v0.8.0
golang.org/x/time v0.0.0-20220922220347-f3bd1da661af
golang.org/x/tools v0.7.0
gopkg.in/natefinch/lumberjack.v2 v2.0.0
gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce
)

Expand Down
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.3/go.mod h1:KLF4gFr6DcKFZwSu
github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v0.3.0 h1:Px2UA+2RvSSvv+RvJNuUB6n7rs5Wsel4dXLe90Um2n4=
github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v0.3.0/go.mod h1:tPaiy8S5bQ+S5sOiDlINkp7+Ef339+Nz5L5XO+cnOHo=
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/BurntSushi/toml v1.2.0 h1:Rt8g24XnyGTyglgET/PRUNlrUeu9F5L+7FilkXfZgs0=
github.com/CloudyKit/fastprinter v0.0.0-20200109182630-33d98a066a53/go.mod h1:+3IMCy2vIlbG1XG/0ggNQv0SvxCAIpPM5b1nCz56Xno=
github.com/CloudyKit/jet/v3 v3.0.0/go.mod h1:HKQPgSJmdK8hdoAbKUUWajkHyHo4RaU5rMdUywE7VMo=
github.com/DataDog/zstd v1.5.2 h1:vUG4lAyuPCXO0TLbXvPv7EB7cNK1QV/luu55UHLrrn8=
Expand Down Expand Up @@ -616,6 +617,8 @@ gopkg.in/go-playground/assert.v1 v1.2.1/go.mod h1:9RXL0bg/zibRAgZUYszZSwO/z8Y/a8
gopkg.in/go-playground/validator.v8 v8.18.2/go.mod h1:RX2a/7Ha8BgOhfk7j780h4/u/RRjR0eouCJSH80/M2Y=
gopkg.in/ini.v1 v1.51.1/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce/go.mod h1:yeKp02qBN3iKW1OzL3MGk2IdtZzaj7SFntXj72NppTA=
gopkg.in/natefinch/lumberjack.v2 v2.0.0 h1:1Lc07Kr7qY4U2YPouBjpCLxpiyxIVoxqXgkXLknAOE8=
gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k=
gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce h1:+JknDZhAj8YMt7GC73Ei8pv4MzjDUNPHgQWJdtMAaDU=
gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce/go.mod h1:5AcXVHNjg+BDxry382+8OKon8SEWiKktQR07RKPsv1c=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ=
Expand Down
133 changes: 102 additions & 31 deletions internal/debug/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ import (
"fmt"
"io"
"net/http"
_ "net/http/pprof" // nolint: gosec
_ "net/http/pprof"
"os"
"path/filepath"
"runtime"

"github.com/ethereum/go-ethereum/internal/flags"
Expand All @@ -32,6 +33,7 @@ import (
"github.com/mattn/go-colorable"
"github.com/mattn/go-isatty"
"github.com/urfave/cli/v2"
"gopkg.in/natefinch/lumberjack.v2"
)

var Memsize memsizeui.Handler
Expand Down Expand Up @@ -76,6 +78,34 @@ var (
Usage: "Prepends log messages with call-site location (file and line number)",
Category: flags.LoggingCategory,
}
logRotateFlag = &cli.BoolFlag{
Name: "log.rotate",
Usage: "Enables log file rotation",
}
logMaxSizeMBsFlag = &cli.IntFlag{
Name: "log.maxsize",
Usage: "Maximum size in MBs of a single log file",
Value: 100,
Category: flags.LoggingCategory,
}
logMaxBackupsFlag = &cli.IntFlag{
Name: "log.maxbackups",
Usage: "Maximum number of log files to retain",
Value: 10,
Category: flags.LoggingCategory,
}
logMaxAgeFlag = &cli.IntFlag{
Name: "log.maxage",
Usage: "Maximum number of days to retain a log file",
Value: 30,
Category: flags.LoggingCategory,
}
logCompressFlag = &cli.BoolFlag{
Name: "log.compress",
Usage: "Compress the log files",
Value: false,
Category: flags.LoggingCategory,
}
pprofFlag = &cli.BoolFlag{
Name: "pprof",
Usage: "Enable the pprof HTTP server",
Expand Down Expand Up @@ -120,11 +150,16 @@ var (
var Flags = []cli.Flag{
verbosityFlag,
vmoduleFlag,
backtraceAtFlag,
debugFlag,
logjsonFlag,
logFormatFlag,
logFileFlag,
backtraceAtFlag,
debugFlag,
logRotateFlag,
logMaxSizeMBsFlag,
logMaxBackupsFlag,
logMaxAgeFlag,
logCompressFlag,
pprofFlag,
pprofAddrFlag,
pprofPortFlag,
Expand All @@ -148,44 +183,66 @@ func init() {
// Setup initializes profiling and logging based on the CLI flags.
// It should be called as early as possible in the program.
func Setup(ctx *cli.Context) error {
logFile := ctx.String(logFileFlag.Name)
useColor := logFile == "" && os.Getenv("TERM") != "dumb" && (isatty.IsTerminal(os.Stderr.Fd()) || isatty.IsCygwinTerminal(os.Stderr.Fd()))

var logfmt log.Format
switch ctx.String(logFormatFlag.Name) {
case "json":
var (
logfmt log.Format
output = io.Writer(os.Stderr)
logFmtFlag = ctx.String(logFormatFlag.Name)
)
switch {
case ctx.Bool(logjsonFlag.Name):
// Retain backwards compatibility with `--log.json` flag if `--log.format` not set
defer log.Warn("The flag '--log.json' is deprecated, please use '--log.format=json' instead")
logfmt = log.JSONFormat()
case "logfmt":
case logFmtFlag == "json":
logfmt = log.JSONFormat()
case logFmtFlag == "logfmt":
logfmt = log.LogfmtFormat()
case "terminal":
logfmt = log.TerminalFormat(useColor)
case "":
// Retain backwards compatibility with `--log.json` flag if `--log.format` not set
if ctx.Bool(logjsonFlag.Name) {
defer log.Warn("The flag '--log.json' is deprecated, please use '--log.format=json' instead")
logfmt = log.JSONFormat()
} else {
logfmt = log.TerminalFormat(useColor)
case logFmtFlag == "", logFmtFlag == "terminal":
useColor := (isatty.IsTerminal(os.Stderr.Fd()) || isatty.IsCygwinTerminal(os.Stderr.Fd())) && os.Getenv("TERM") != "dumb"
if useColor {
output = colorable.NewColorableStderr()
}
logfmt = log.TerminalFormat(useColor)
default:
// Unknown log format specified
return fmt.Errorf("unknown log format: %v", ctx.String(logFormatFlag.Name))
}

if logFile != "" {
var err error
logOutputStream, err = log.FileHandler(logFile, logfmt)
if err != nil {
return err
var (
stdHandler = log.StreamHandler(output, logfmt)
ostream = stdHandler
logFile = ctx.String(logFileFlag.Name)
rotation = ctx.Bool(logRotateFlag.Name)
)
if len(logFile) > 0 {
if err := validateLogLocation(filepath.Dir(logFile)); err != nil {
return fmt.Errorf("failed to initiatilize file logger: %v", err)
}
} else {
output := io.Writer(os.Stderr)
if useColor {
output = colorable.NewColorableStderr()
}
if rotation {
// Lumberjack uses <processname>-lumberjack.log in is.TempDir() if empty.
// so typically /tmp/geth-lumberjack.log on linux
if len(logFile) > 0 {
log.Info("Rotating file logging configured", "location", logFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should defer these? In case format=json is specified, it's a bit annoying to have a non-json regular log being spat out before the json-logs.

Suggested change
log.Info("Rotating file logging configured", "location", logFile)
defer log.Info("Rotating file logging configured", "location", logFile)

@fjl wdyt?

Copy link
Contributor

@fjl fjl Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your reasoning that it's not good to print a message while logging is not fully configured yet.

I'm also wondering, are these messages really necessary? I think these should be at debug level or just be removed. Geth already prints a ton of INFO messages at startup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also wondering, are these messages really necessary?

Maybe not...

Though I generally find it annoying when debugging something that does not work, and it is not possible to see "what does this application think I told it to do". And with rotation, you won't notice that it does (or does not) work until much later, when it fails to rotate.

Like, if you configure everything about the rotation, but omitted log.rotate, then it's better UX if users are able to notice immediately. (and now that I say that, I also feel that we should add "rotate","off" on the other message)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now I do print out the info in case non-default options are used. I'm happy with it now

} else {
log.Info("Rotating file logging configured", "location",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Info("Rotating file logging configured", "location",
defer log.Info("Rotating file logging configured", "location",

filepath.Join(os.TempDir(), "geth-lumberjack.log"))
}
ostream = log.MultiHandler(log.StreamHandler(&lumberjack.Logger{
Filename: logFile,
MaxSize: ctx.Int(logMaxSizeMBsFlag.Name),
MaxBackups: ctx.Int(logMaxBackupsFlag.Name),
MaxAge: ctx.Int(logMaxAgeFlag.Name),
Compress: ctx.Bool(logCompressFlag.Name),
}, logfmt), stdHandler)
} else if logFile != "" {
if logOutputStream, err := log.FileHandler(logFile, logfmt); err != nil {
return err
} else {
ostream = log.MultiHandler(logOutputStream, stdHandler)
log.Info("File logging configured", "location", logFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Info("File logging configured", "location", logFile)
defer log.Info("File logging configured", "location", logFile)

}
logOutputStream = log.StreamHandler(output, logfmt)
}
glogger.SetHandler(logOutputStream)
glogger.SetHandler(ostream)

// logging
verbosity := ctx.Int(verbosityFlag.Name)
Expand Down Expand Up @@ -263,3 +320,17 @@ func Exit() {
closer.Close()
}
}

func validateLogLocation(path string) error {
if err := os.MkdirAll(path, os.ModePerm); err != nil {
return fmt.Errorf("error creating the directory: %w", err)
}
// Check if the path is writable by trying to create a temporary file
tmp := filepath.Join(path, "tmp")
if f, err := os.Create(tmp); err != nil {
return err
} else {
f.Close()
}
return os.Remove(tmp)
}