You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/10/27 17:16:52 UTC
[GitHub] [bookkeeper] Shawyeok opened a new pull request, #3584: [improve] Optimize log4j configuration
Shawyeok opened a new pull request, #3584:
URL: https://github.com/apache/bookkeeper/pull/3584
### Motivation
Log output with [location information is very expensive][1], we should avoid use it in our log configuration. In most cases, we are able to find which line print specific log by logger name and log content.
### Changes
- remove log line information
- use logger name instead of caller class name
[1]: https://logging.apache.org/log4j/2.x/manual/layouts.html#LocationInformation
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shawyeok commented on pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#issuecomment-1293838462
If this pr merged, I would like to port to other release branches.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shawyeok commented on a diff in pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on code in PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#discussion_r1067232558
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
`%d{ISO8601} [%t] %-5level %logger - %m%n%ex`
@hangc0276 Sorry for late reply.
If we change log pattern, why not make one step further, isn't? So, how about this?
- `%-5p` and `%-5level` is identical but later is more readable
- `%logger` is identical with `%c`, logger name with full package name, pay a little more disk space
- the `-` after timestamp is redundant
- `%ex` is to prevent default `%xEx`, which is to add packaging information about each stack trace element, that means more cost. see more: `org.apache.logging.log4j.core.pattern.ExtendedThrowablePatternConverter`
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
`%d{ISO8601} [%t] %-5level %logger - %m%n%ex`
@hangc0276 Sorry for late reply.
If we change log pattern, why not make one step further, isn't? So, how about this?
- `%-5p` and `%-5level` is identical but later is more readable
- `%logger` is identical with `%c`, logger name with full package name, pay a little more disk space
- the `-` after timestamp is redundant
- `%ex` is to prevent default `%xEx`, which is to add packaging information about each stack trace element, that means cost more. see more: `org.apache.logging.log4j.core.pattern.ExtendedThrowablePatternConverter`
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#issuecomment-1378347110
ping @Shawyeok, would you please take a look at the comments? thanks a lot.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shawyeok commented on a diff in pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on code in PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#discussion_r1067232558
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
`%d{ISO8601} [%t] %-5level %logger - %m%n%ex`
@hangc0276 Sorry for late reply.
If we change log pattern, why not make one step further, isn't? So, how about this?
- %-5p and %-5level is identical but later is more readable
- %logger is identical with %c, logger name with full package name, pay a little more disk space
- the `-` after timestamp is redundant
- %ex is to prevent default %xEx, which is to add packaging information about each stack trace element, that means more cost. see more: `org.apache.logging.log4j.core.pattern.ExtendedThrowablePatternConverter`
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#discussion_r1068929451
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
Without `%L`, we can't identify the place where the log is from.
In some case, we will record the same info in the same class, without the line number, we can't trace the problem precisely according to the log info.
If we want to remove `%L`, we should ensure can't print same log content in the same class in different place
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#issuecomment-1378347532
@horizonzy Would you please help take a look at this PR? Thanks a lot.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shawyeok commented on a diff in pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on code in PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#discussion_r1067232558
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
`%d{ISO8601} [%t] %-5level %logger - %m%n%ex`
@hangc0276 How about this?
- %-5p and %-5level is identical but later is more readable
- %logger is identical with %c, logger name with full package name, pay a little more disk space
- the `-` after timestamp is redundant
- %ex is to prevent default %xEx, which is to add packaging information about each stack trace element, that means more cost. see more: `org.apache.logging.log4j.core.pattern.ExtendedThrowablePatternConverter`
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3584: [improve] Optimize log4j configuration
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3584:
URL: https://github.com/apache/bookkeeper/pull/3584#discussion_r1007676044
##########
conf/log4j2.xml:
##########
@@ -28,13 +28,13 @@
</Properties>
<Appenders>
<Console name="CONSOLE" target="SYSTEM_OUT">
- <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p - [%t:%c{1}] - %m%n"/>
</Console>
<File name="TRACEFILE" fileName="${sys:bookkeeper.log.dir}/bookkeeper-trace.log">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L][%ndc] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}][%ndc] - %m%n"/>
</File>
<RollingFile name="ROLLINGFILE" fileName="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}" filePattern="${sys:bookkeeper.log.dir}/${sys:bookkeeper.log.file}%d{.yyyy-MM-dd}">
- <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%C{1}@%L] - %m%n"/>
+ <PatternLayout pattern="%d{ISO8601} - %-5p [%t:%c{1}] - %m%n"/>
Review Comment:
Can we include the full package name?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org