ctubbsii commented on code in PR #2077: URL: https://github.com/apache/zookeeper/pull/2077#discussion_r1358860891
########## conf/logback.xml: ########## @@ -109,6 +125,6 @@ </logger--> <root level="INFO"> - <appender-ref ref="CONSOLE" /> Review Comment: Dynamically controlling the config file seems like a complicated way of customizing the config file. It would be better to use `logback. configurationFile` system property to specify a different config file, rather than make this reference one more complicated. ########## zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md: ########## @@ -519,6 +519,13 @@ For more information about SLF4J, see For more information about Logback, see [Logback website](http://logback.qos.ch/). +#### Logging in JSON format + +Through its pluggable and flexible log framework, Zookeeper can easily be configured to log in JSON format. +An example of JSON logging using the popular [ECS schema](https://doc.wikimedia.org/ecs/) is provided as a logback appender in `logback.xml`. +You can test it by enabling the `CONSOLE-JSON` appender, e.g. by starting Zookeeper with +the system property: `-Dzookeeper.log.appender=CONSOLE-JSON`. Review Comment: JSON formatting is a niche use case. This is one of millions of possible ways of configuring logging. I'm not sure it warrants a special call out in the ZK docs. The previous paragraph already links to the logback website, if people want to do something custom with logback. ########## zookeeper-server/pom.xml: ########## @@ -87,6 +87,10 @@ <groupId>ch.qos.logback</groupId> <artifactId>logback-classic</artifactId> </dependency> + <dependency> + <groupId>co.elastic.logging</groupId> + <artifactId>logback-ecs-encoder</artifactId> + </dependency> Review Comment: While this might be useful for some... most people are probably not going to use this. It doesn't make sense to add this optional dependency to the class path for everybody. Some people may want a more minimal approach, for a smaller security footprint, and to save space in their downstream environment. ########## conf/logback.xml: ########## @@ -109,6 +125,6 @@ </logger--> <root level="INFO"> - <appender-ref ref="CONSOLE" /> Review Comment: Dynamically controlling the config file seems like a complicated way of customizing the config file. It would be better to use `logback.configurationFile` system property to specify a different config file, rather than make this reference one more complicated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
