Add zap encoding configurable #13339

ardaguclu · 2021-09-10T08:53:10Z

Json encoding is the default zap encoding value and can not be changeable.
This PR enables configuring zap encoding to console via new flag log-format.

ardaguclu · 2021-09-10T08:54:00Z

/assign @lilic

Could you please take a look?, thanks.

lilic

A couple of comments, thanks for the PR!

@hexfusion @ptabor

lilic · 2021-09-10T11:59:10Z

client/v3/logger.go

@@ -51,18 +51,28 @@ func etcdClientDebugLevel() zapcore.Level {
 		return zapcore.InfoLevel
 	}
 	var l zapcore.Level
-	if err := l.Set(envLevel); err == nil {
+	if err := l.Set(envLevel); err != nil {
 		log.Printf("Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'")


I think this should be something like, as its not necessarily deprecated:

Suggested change

log.Printf("Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'")

log.Printf("Unsupported env ETCD_CLIENT_DEBUG value. Using default level: 'info'")

lilic · 2021-09-10T12:09:45Z

client/v3/logger.go

 		log.Printf("Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'")
 		return zapcore.InfoLevel
 	}
 	return l
 }

+func etcdClientLogFormat() string {
+	envFormat := os.Getenv("ETCD_CLIENT_LOG_FORMAT")


Let's document this in the

etcd/client/v3/doc.go

Lines 102 to 104 in e2d67f2

// The grpc load balancer is registered statically and is shared across etcd clients.

// To enable detailed load balancer logging, set the ETCD_CLIENT_DEBUG environment

// variable. E.g. "ETCD_CLIENT_DEBUG=1".

as well add a changelog entry similar to how the ETCD_CLIENT_DEBUG was added.

lilic · 2021-09-10T12:11:42Z

client/v3/logger.go

+
+	return "json"
+}
+
 // CreateDefaultZapLoggerConfig creates a logger config that is configurable using env variable:
 // ETCD_CLIENT_DEBUG= debug|info|warn|error|dpanic|panic|fatal|true (true=info)


Also same as above, please add what options ETCD_CLIENT_LOG_FORMAT has.

ardaguclu · 2021-09-10T12:27:28Z

Thanks for review @lilic, I applied your comments.

lilic

Thanks!

CHANGELOG-3.5.md

codecov-commenter · 2021-09-11T10:09:21Z

Codecov Report

Merging #13339 (c2937d7) into main (c2937d7) will not change coverage.
The diff coverage is n/a.

❗ Current head c2937d7 differs from pull request most recent head 168b868. Consider uploading reports for the commit 168b868 to get more accurate results

@@           Coverage Diff           @@
##             main   #13339   +/-   ##
=======================================
  Coverage   39.49%   39.49%           
=======================================
  Files         460      460           
  Lines       39141    39141           
=======================================
  Hits        15459    15459           
  Misses      22009    22009           
  Partials     1673     1673

Flag	Coverage Δ
all	`39.49% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out 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 c2937d7...168b868. Read the comment docs.

lilic

One small comment, otherwise lgtm.

lilic · 2021-09-13T09:38:01Z

CHANGELOG-3.6.md

@@ -28,3 +28,6 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0).
 - Package `mvcc/buckets` was moved to `storage/schema`
 - Package `wal` was moved to `storage/wal`
 - Package `datadir` was moved to `storage/datadir`
+
+### Package `clientv3`


Small nit, new line missing after this.

lilic

Thank you!

@ptabor @hexfusion please take a look, thanks!

hexfusion

My feelings here are that we should not introduce a new orphaned ENV configuration and ensure a similar UX to the existing logging configs. IE

--log-format=console

ardaguclu · 2021-09-22T08:47:35Z

Thanks for review @hexfusion. Do you mean something recently pushed?

hexfusion · 2021-09-22T10:40:37Z

Thanks for review @hexfusion. Do you mean something recently pushed?

Yes :) #13295 (comment) was to change server log format. So let's start here, thank you.

ardaguclu · 2021-09-22T11:11:27Z

@hexfusion, one last thing; do I have to update proto files?

hexfusion · 2021-09-22T12:37:33Z

@hexfusion, one last thing; do I have to update proto files?

No API changes should be required to my knowledge.

Json encoding is the default zap encoding value and can not be changeable. This PR enables configuring zap encoding to console via new flag `log-format`.

hexfusion · 2021-09-22T14:38:47Z

server/embed/config_logging.go

+			var encoder zapcore.Encoder
+			if logutil.ConvertToZapFormat(cfg.LogFormat) == logutil.ConsoleLogFormat {
+				encoder = zapcore.NewConsoleEncoder(logutil.DefaultZapLoggerConfig.EncoderConfig)
+			} else {


I might have missed this but do we validate the logFormat? IE --log-format=konsole

I don't think so, because ConvertToZapFormat handles this https://github.com/etcd-io/etcd/pull/13339/files#diff-47d0d38c1df3e69bbdfb072164f8ee599204de9b3871d41d0c11b7ffed1b619cR25

It returns json other than anything console including the empty string.

I think we should consider returning a clear error to admin that the config was not valid vs defaulting silently.

Indeed, you are right. I'll add validation and warning user if log-format value is incorrect.

ardaguclu · 2021-10-08T08:02:56Z

Could we merge this?, what do you think?

lilic

@hexfusion @ptabor please take a look again.

ptabor

LGTM. Thank you.

lilic reviewed Sep 10, 2021

View reviewed changes

CHANGELOG-3.5.md Outdated Show resolved Hide resolved

ardaguclu force-pushed the support-zap-console-encoding branch from 67a3c5f to 168b868 Compare September 10, 2021 13:12

lilic reviewed Sep 13, 2021

View reviewed changes

ardaguclu force-pushed the support-zap-console-encoding branch from 168b868 to 83a43dc Compare September 13, 2021 09:46

lilic approved these changes Sep 13, 2021

View reviewed changes

hexfusion reviewed Sep 22, 2021

View reviewed changes

ardaguclu changed the title ~~Add configuring zap encoding "console" with ETCD_CLIENT_LOG_FORMAT~~ Add --log-format parameter instead ENV var Sep 22, 2021

ardaguclu changed the title ~~Add --log-format parameter instead ENV var~~ Add zap encoding configurable Sep 22, 2021

ardaguclu force-pushed the support-zap-console-encoding branch from b25404b to f83eca2 Compare September 22, 2021 08:45

ardaguclu force-pushed the support-zap-console-encoding branch from f83eca2 to c6ce677 Compare September 22, 2021 12:47

Add zap encoding configurable

e647995

Json encoding is the default zap encoding value and can not be changeable. This PR enables configuring zap encoding to console via new flag `log-format`.

ardaguclu force-pushed the support-zap-console-encoding branch from c6ce677 to e647995 Compare September 22, 2021 12:49

hexfusion reviewed Sep 22, 2021

View reviewed changes

ardaguclu added 2 commits September 22, 2021 19:45

Return error when log-format is invalid

ec252d0

Handle empty log-format gracefully

3c6e09f

ptabor requested a review from serathius September 25, 2021 15:36

Merge branch 'main' into support-zap-console-encoding

d7fa802

serathius approved these changes Oct 8, 2021

View reviewed changes

lilic approved these changes Oct 8, 2021

View reviewed changes

ptabor approved these changes Oct 8, 2021

View reviewed changes

hexfusion merged commit 691dcd5 into etcd-io:main Oct 20, 2021

hexfusion mentioned this pull request Nov 15, 2021

Impossible to configure logging format when using zap logger #13295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zap encoding configurable #13339

Add zap encoding configurable #13339

ardaguclu commented Sep 10, 2021 •

edited

Loading

ardaguclu commented Sep 10, 2021

lilic left a comment

lilic Sep 10, 2021

lilic Sep 10, 2021

lilic Sep 10, 2021

ardaguclu commented Sep 10, 2021

lilic left a comment

codecov-commenter commented Sep 11, 2021

lilic left a comment

lilic Sep 13, 2021

lilic left a comment

hexfusion left a comment

ardaguclu commented Sep 22, 2021

hexfusion commented Sep 22, 2021

ardaguclu commented Sep 22, 2021

hexfusion commented Sep 22, 2021

hexfusion Sep 22, 2021

ardaguclu Sep 22, 2021

ardaguclu Sep 22, 2021

hexfusion Sep 22, 2021

ardaguclu Sep 22, 2021

ardaguclu commented Oct 8, 2021

lilic left a comment

ptabor left a comment

	log.Printf("Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'")
	log.Printf("Unsupported env ETCD_CLIENT_DEBUG value. Using default level: 'info'")

	// The grpc load balancer is registered statically and is shared across etcd clients.
	// To enable detailed load balancer logging, set the ETCD_CLIENT_DEBUG environment
	// variable. E.g. "ETCD_CLIENT_DEBUG=1".

Add zap encoding configurable #13339

Add zap encoding configurable #13339

Conversation

ardaguclu commented Sep 10, 2021 • edited Loading

ardaguclu commented Sep 10, 2021

lilic 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

ardaguclu commented Sep 10, 2021

lilic left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 11, 2021

Codecov Report

lilic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilic left a comment

Choose a reason for hiding this comment

hexfusion left a comment

Choose a reason for hiding this comment

ardaguclu commented Sep 22, 2021

hexfusion commented Sep 22, 2021

ardaguclu commented Sep 22, 2021

hexfusion commented Sep 22, 2021

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

ardaguclu commented Oct 8, 2021

lilic left a comment

Choose a reason for hiding this comment

ptabor left a comment

Choose a reason for hiding this comment

ardaguclu commented Sep 10, 2021 •

edited

Loading