*: make log level configurable #10947

gyuho · 2019-07-29T02:51:35Z

Address #10946.

etcd --logger zap --log-level warn

{"level":"warn","ts":"2019-07-28T21:50:24.137-0500","caller":"etcdmain/etcd.go:119","msg":"'data-dir' was empty; using default","data-dir":"default.etcd"}
{"level":"warn","ts":"2019-07-28T21:50:24.242-0500","caller":"auth/store.go:1317","msg":"simple token is not cryptographically signed"}
{"level":"warn","ts":"2019-07-28T21:50:24.269-0500","caller":"etcdserver/metrics.go:186","msg":"failed to get file descriptor usage","error":"cannot get FDUsage on darwin"}

^C{"level":"warn","ts":"2019-07-28T21:50:32.216-0500","caller":"grpclog/grpclog.go:60","msg":"transport: http2Server.HandleStreams failed to read frame: read tcp 127.0.0.1:2379->127.0.0.1:52750: use of closed network connection"}

In 3.5, we can deprecate --debug flag, in favor of --log-level=debug.

Will update CHANGELOG in a separate PR.

codecov-io · 2019-07-29T03:29:49Z

Codecov Report

Merging #10947 into master will increase coverage by 0.14%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master   #10947      +/-   ##
==========================================
+ Coverage    63.6%   63.74%   +0.14%     
==========================================
  Files         400      401       +1     
  Lines       37415    37470      +55     
==========================================
+ Hits        23796    23885      +89     
+ Misses      12001    11945      -56     
- Partials     1618     1640      +22

Impacted Files	Coverage Δ
pkg/logutil/zap.go	`78.57% <ø> (ø)`	⬆️
pkg/logutil/zap_raft.go	`50% <100%> (+2.94%)`	⬆️
raft/logger.go	`50% <100%> (+4.28%)`	⬆️
embed/config.go	`50.84% <100%> (+0.13%)`	⬆️
etcdmain/config.go	`82.95% <100%> (+0.07%)`	⬆️
pkg/logutil/log_level.go	`22.22% <22.22%> (ø)`
embed/config_logging.go	`27.62% <36.84%> (+0.61%)`	⬆️
etcdserver/raft.go	`77.83% <80%> (+0.26%)`	⬆️
client/keys.go	`60.3% <0%> (-31.16%)`	⬇️
etcdserver/api/v3rpc/util.go	`51.61% <0%> (-16.13%)`	⬇️
... and 26 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 936c506...a900ef5. Read the comment docs.

jingyih · 2019-07-29T21:09:33Z

embed/config_logging.go

+	if cfg.Debug {
+		fmt.Fprintf(os.Stderr, "[WARNING] Deprecated '--debug' flag is set to %v (use '--log-level=debug' instead\n", cfg.Debug)
+	}
+	if cfg.Debug && cfg.LogLevel != "debug" {


Should this just be a warning? This setting is actually conflicting with itself.

Could you clarify? This is a warning when something like etcd --debug --log-level warning is configured. With this change, the log level will be overwritten by --log-level warning.

I see. So --log-level, when provided, will overwrite --debug.

jingyih · 2019-07-29T21:13:37Z

etcdmain/help.go

+
+
+TO BE DEPRECATED:
+


nit: remove the first empty line?

gyuho · 2019-07-29T21:26:57Z

@jingyih PTAL. Thx.

jingyih

Looks good. Added one more question.

jingyih · 2019-07-29T21:33:44Z

embed/config_logging.go

+	if cfg.Debug {
+		fmt.Fprintf(os.Stderr, "[WARNING] Deprecated '--debug' flag is set to %v (use '--log-level=debug' instead\n", cfg.Debug)
+	}
+	if cfg.Debug && cfg.LogLevel != "debug" {


I see. So --log-level, when provided, will overwrite --debug.

jingyih · 2019-07-29T21:34:52Z

embed/config_logging.go

-			if cfg.Debug {
-				copied.Level = zap.NewAtomicLevelAt(zap.DebugLevel)
+			copied.Level = zap.NewAtomicLevelAt(logutil.ConvertToZapLevel(cfg.LogLevel))
+			if cfg.Debug || cfg.LogLevel == "debug" {


If LogLevel is not "debug", it should overwrite Debug flag here?

Debug is only used here, so no need overwrite :)

So under configuration etcd --debug --log-level info, grpc tracking is enabled?

etcd --debug --log-level info, grpc tracking is enabled?

Yes, because cfg.Debug will evaluate to true.

Sorry I think I am still confused. As an example, etcd --debug --log-level info sets the logging level to "info", but grpc tracking is enabled. This seems to be different than the previous behavior, where tracing is only enabled if logging level is set to "debug"?

@jingyih

As an example, etcd --debug --log-level info sets the logging level to "info", but grpc tracking is enabled. This seems to be different than the previous behavior,

I clarified with comments in the code.

Basically, we want to keep the backward compatibility since --log-level is the new flag in v3.4. For instance, etcd --debug --log-level warning in v3.4 should work in the same way as etcd --debug in v3.3, thus enabling the gRPC tracing even when --log-level warning was meant to disable gRPC tracing. If --debug is passed with non-debug --log-level, the user will be warned about its inconsistent log configuration in v3.4.

Let me know.

Oh I misread earlier. Thanks!

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

Make log level configurable, and deprecate "debug" flag in v3.5. And adds more warnings on flags that's being deprecated in v3.5. Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

To make it consistent Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

jingyih · 2019-07-29T22:53:03Z

LGTM

gyuho mentioned this pull request Jul 29, 2019

default log level to fatal #10946

Closed

gyuho force-pushed the log-level branch from d018e4e to ef781a0 Compare July 29, 2019 02:54

gyuho mentioned this pull request Jul 29, 2019

Release etcd 3.4 #10943

Closed

19 tasks

gyuho force-pushed the log-level branch from ef781a0 to 61132e6 Compare July 29, 2019 02:57

gyuho added the Release-Note label Jul 29, 2019

gyuho requested a review from jingyih July 29, 2019 16:58

jingyih reviewed Jul 29, 2019

View reviewed changes

gyuho force-pushed the log-level branch from d1d91f1 to a900ef5 Compare July 29, 2019 21:24

jingyih reviewed Jul 29, 2019

View reviewed changes

gyuho added 7 commits July 29, 2019 15:43

embed: do not expose "zapLoggerBuilder"

348b0d4

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

pkg/logutil: add log level utilities

e911f90

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

embed: add "LogLevel", deprecate "Debug" in v3.5

b9de4bd

Make log level configurable, and deprecate "debug" flag in v3.5. And adds more warnings on flags that's being deprecated in v3.5. Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

etcdmain: add "--log-level" flag

a37f344

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

Documentation/op-guide: add "--log-level" flag

abba542

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

etcdserver: make raft log configured by top level logger

c6e3401

To make it consistent Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

raft: use mutex in "SetLogger" to avoid race conditions in tests

4e43a08

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

gyuho force-pushed the log-level branch from a900ef5 to 4e43a08 Compare July 29, 2019 22:43

gyuho merged commit 6e766ac into etcd-io:master Jul 29, 2019

gyuho deleted the log-level branch July 29, 2019 23:06

grv231 mentioned this pull request Oct 31, 2019

Change etcd logging level in kops kubernetes/kops#7859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: make log level configurable #10947

*: make log level configurable #10947

gyuho commented Jul 29, 2019 •

edited

Loading

codecov-io commented Jul 29, 2019 •

edited

Loading

jingyih Jul 29, 2019

gyuho Jul 29, 2019

jingyih Jul 29, 2019

jingyih Jul 29, 2019

gyuho Jul 29, 2019

gyuho commented Jul 29, 2019

jingyih left a comment

jingyih Jul 29, 2019

jingyih Jul 29, 2019

gyuho Jul 29, 2019

jingyih Jul 29, 2019

gyuho Jul 29, 2019

jingyih Jul 29, 2019

gyuho Jul 29, 2019 •

edited

Loading

jingyih Jul 29, 2019

jingyih commented Jul 29, 2019

*: make log level configurable #10947

*: make log level configurable #10947

Conversation

gyuho commented Jul 29, 2019 • edited Loading

codecov-io commented Jul 29, 2019 • edited Loading

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

Choose a reason for hiding this comment

gyuho commented Jul 29, 2019

jingyih 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingyih commented Jul 29, 2019

gyuho commented Jul 29, 2019 •

edited

Loading

codecov-io commented Jul 29, 2019 •

edited

Loading

gyuho Jul 29, 2019 •

edited

Loading