algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. #3903

winder · 2022-04-21T12:12:15Z

Summary

Add an implicit option to write log to stdout by setting LogSizeLimit to 0.

Test Plan

Tested manually.

brianolson · 2022-04-21T12:49:59Z

I'm not sure what the prior behavior would have been with LogSizeLimit:0.
Would it be better anyway to have an explicit boolean --log-to-stdout ?
(But this change is clean if we decide to go this way)

winder · 2022-04-21T12:53:41Z

I'm not sure what the prior behavior would have been with LogSizeLimit:0. Would it be better anyway to have an explicit boolean --log-to-stdout ? (But this change is clean if we decide to go this way)

What do you think about having both? The --log-to-stdout could overrides the LogSizeLimit variable.

The previous behavior here is basically a bug, all calls to log write an error to stdout:

Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 415
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 443
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 395
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 395
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273
Failed to write to log, CyclicFileWriter: input too long to write. Len = 389
Failed to write to log, CyclicFileWriter: input too long to write. Len = 273

brianolson · 2022-04-21T13:12:34Z

I like the explicit option better. And yeah, it'd override because 'cyclic' doesn't make sense for stdout.

winder · 2022-04-21T13:31:48Z

@brianolson added a flag. Also converted algod to use cobra, but I can revert that part.

brianolson

Curious what the new algod -h looks like with cobra. Old for comparison:

$ algod -h
Usage of algod:
  -G    Print genesis ID
  -b    Display the git branch behind the build
  -c    Display and release channel behind the build
  -d string
        Root Algorand daemon data path
  -g string
        Genesis configuration file
  -l string
        Override config.EndpointAddress (REST listening address) with ip:port
  -p string
        Override phonebook with peer ip:port (or semicolon separated list: ip:port;ip:port;ip:port...)
  -s string
        Telemetry Session GUID to use
  -seed string
        input to math/rand.Seed()
  -t string
        Override telemetry setting if supported (Use "true", "false", "0" or "1"
  -v    Display and write current build version and exit
  -x    Initialize the ledger and exit

brianolson · 2022-04-21T14:32:00Z

cmd/algod/main.go

+	command.Flags().StringVarP(&args.listenIP, "listenIP", "l", "", "Override config.EndpointAddress (REST listening address) with ip:port")
+	command.Flags().StringVarP(&args.sessionGUID, "sessionGUID", "s", "", "Telemetry Session GUID to use")
+	command.Flags().StringVarP(&args.telemetryOverride, "telemetryOverride", "t", "", `Override telemetry setting if supported (Use "true", "false", "0" or "1"`)
+	command.Flags().StringVarP(&args.seed, "seed", "", "", "input to math/rand.Seed()")


I think this needs to respond to "-seed" to be compatible with flag. But every other flag option only had the natural single letter -x and fits with -x/--xtralongname

That might be a deal breaker here, that is the one part of the interface that had to change.

brianolson · 2022-04-21T14:35:34Z

daemon/algod/server.go

-	logWriter := logging.MakeCyclicFileWriter(liveLog, archive, cfg.LogSizeLimit, maxLogAge)
+
+	var logWriter io.Writer
+	if cfg.LogSizeLimit > 0 {


if args.logToStdout || cfg.LogSizeLimit <= 0 { ?

The main function overrides cfg.LogSizeLimit, so this is still the right condition.

This reverts commit 303231f.

winder · 2022-04-21T15:31:15Z

I reverted the cobra changes, since you were curious this was the help output:

algod allows a node to participate in the agreement protocol, submit and confirm transactions, and view the state of the Algorand Ledger.

Usage:
  algod [flags]

Flags:
  -b, --branchCheck                Display the git branch behind the build
  -c, --channelCheck               Display and release channel behind the build
  -d, --dataDir string             Root Algorand daemon data path
  -g, --genesisFile string         Genesis configuration file
  -h, --help                       help for algod
  -x, --initAndExit                Initialize the ledger and exit
  -l, --listenIP string            Override config.EndpointAddress (REST listening address) with ip:port
  -o, --logToStdout                Write to stdout instead of node.log by overriding config.LogSizeLimit to 0
  -p, --peerOverride string        Override phonebook with peer ip:port (or semicolon separated list: ip:port;ip:port;ip:port...)
  -G, --printGenesis               Print genesis ID
      --seed string                input to math/rand.Seed()
  -s, --sessionGUID string         Telemetry Session GUID to use
  -t, --telemetryOverride string   Override telemetry setting if supported (Use "true", "false", "0" or "1"
  -v, --version                    Display and write current build version and exit

codecov-commenter · 2022-04-21T16:13:07Z

Codecov Report

Merging #3903 (35679f0) into master (12ded27) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3903      +/-   ##
==========================================
- Coverage   50.09%   50.07%   -0.03%     
==========================================
  Files         394      394              
  Lines       68456    68462       +6     
==========================================
- Hits        34294    34279      -15     
- Misses      30465    30482      +17     
- Partials     3697     3701       +4

Impacted Files	Coverage Δ
cmd/algod/main.go	`0.00% <0.00%> (ø)`
config/localTemplate.go	`42.85% <ø> (ø)`
daemon/algod/server.go	`5.06% <0.00%> (-0.14%)`	⬇️
network/wsPeer.go	`68.61% <0.00%> (-3.06%)`	⬇️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
util/metrics/gauge.go	`65.38% <0.00%> (-2.57%)`	⬇️
util/metrics/counter.go	`86.20% <0.00%> (-2.30%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
catchup/service.go	`69.62% <0.00%> (ø)`
ledger/acctupdates.go	`69.16% <0.00%> (+0.65%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ded27...35679f0. Read the comment docs.

brianolson · 2022-04-21T17:10:26Z

cmd/algod/main.go

@@ -50,10 +50,11 @@ var versionCheck = flag.Bool("v", false, "Display and write current build versio
 var branchCheck = flag.Bool("b", false, "Display the git branch behind the build")
 var channelCheck = flag.Bool("c", false, "Display and release channel behind the build")
 var initAndExit = flag.Bool("x", false, "Initialize the ledger and exit")
+var logToStdout = flag.Bool("o", false, "Write to stdout instead of node.log by overriding config.LogSizeLimit to 0")


Oh, but now I'm imagining the convention -o foo to log to file foo and -o - for stdout.

doc would note "defaults to {data dir}/node.log"

I'd rather not add that feature unless there's a real reason that it would be useful. I kinda want to remove most of these flags, not sure they're actually used anymore.

I guess picking a different letter would address that concern but all the good letters are taken already.

Write to stdout when LogSizeLimit is 0.

2d8d8b9

winder requested review from algobolson and algojohnlee April 21, 2022 12:12

winder changed the title ~~Write to stdout when LogSizeLimit is 0.~~ algod: Write to stdout when LogSizeLimit is 0. Apr 21, 2022

winder self-assigned this Apr 21, 2022

winder added Team Lamprey Enhancement labels Apr 21, 2022

winder added 3 commits April 21, 2022 09:15

Use cobra instead of flags.

2e6a3a2

New logToStdout algod option.

6f89895

Add long parameter names so that short commands use a single dash.

4b7337b

Fix test error.

303231f

winder force-pushed the will/stdout-logger branch from 99c903f to 303231f Compare April 21, 2022 14:02

brianolson reviewed Apr 21, 2022

View reviewed changes

winder added 2 commits April 21, 2022 11:28

Simple flag instead of cobra.

2dde9e7

Revert "Fix test error."

3aabd7d

This reverts commit 303231f.

Fix typo.

35679f0

winder requested a review from brianolson April 21, 2022 15:55

winder changed the title ~~algod: Write to stdout when LogSizeLimit is 0.~~ algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. Apr 21, 2022

brianolson reviewed Apr 21, 2022

View reviewed changes

winder requested a review from cce April 22, 2022 16:21

cce approved these changes Apr 22, 2022

View reviewed changes

winder merged commit 452af1c into algorand:master Apr 23, 2022

winder deleted the will/stdout-logger branch April 23, 2022 18:03

Algo-devops-service mentioned this pull request Apr 26, 2022

go-algorand 0.0.0-beta Release PR #3922

Closed

This was referenced Apr 27, 2022

go-algorand 3.6.0-beta Release PR #3923

Merged

go-algorand 3.6.0-beta Release PR #3934

Merged

This was referenced May 2, 2022

go-algorand 3.6.1-beta Release PR #3943

Merged

go-algorand 3.6.2-beta Release PR #3947

Merged

Algo-devops-service mentioned this pull request May 7, 2022

go-algorand 3.6.2-stable Release PR #3960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. #3903

algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. #3903

winder commented Apr 21, 2022

brianolson commented Apr 21, 2022

winder commented Apr 21, 2022

brianolson commented Apr 21, 2022

winder commented Apr 21, 2022

brianolson left a comment

brianolson Apr 21, 2022

winder Apr 21, 2022

brianolson Apr 21, 2022

winder Apr 21, 2022

winder commented Apr 21, 2022

codecov-commenter commented Apr 21, 2022 •

edited

brianolson Apr 21, 2022

brianolson Apr 21, 2022

winder Apr 21, 2022

cce Apr 22, 2022

algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. #3903

algod: Write to stdout when config.LogSizeLimit is 0 or -o is passed to algod. #3903

Conversation

winder commented Apr 21, 2022

Summary

Test Plan

brianolson commented Apr 21, 2022

winder commented Apr 21, 2022

brianolson commented Apr 21, 2022

winder commented Apr 21, 2022

brianolson left a 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

winder commented Apr 21, 2022

codecov-commenter commented Apr 21, 2022 • edited

Codecov Report

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

codecov-commenter commented Apr 21, 2022 •

edited