[
https://issues.apache.org/jira/browse/LOG4J2-695?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14049807#comment-14049807
]
Remko Popma edited comment on LOG4J2-695 at 7/2/14 10:30 AM:
-------------------------------------------------------------
Hmm... your request goes a bit beyond the scope of fixing bugs in Log4j. I'm
basically giving you free consulting... All right then, constructive criticism.
You better sit down first though, you may not like what I have to say...
First, please tell me that your development teams will be allowed to use the
"normal" Log4j2 API in addition to the CustomLogger, and the CustomLogger is
intended for optional use to send events to splunk. If your teams are forced to
use CustomLogger for _all_ their logging needs, you may want to get good life
insurance because your teams will really, really not like it. They will ask
questions like this:
* Why is there no trace or debug capability any more? Why provide a logger that
can only log info/warn/error/fatal? In my applications I only use INFO at
startup, and WARN/ERROR if there is a problem. By then it is too late and your
log file will not contain any detail on what lead to this error. This will make
it very difficult for your teams to solve issues, making the logging almost
useless...
* One of the most important functions of logging is error handling: a stack
trace in the log file is a huge, huge help in finding and fixing bugs.
CustomLogger has stripped out the methods that take a {{Throwable}} argument.
If your teams cannot use the normal API to log stack traces they will not thank
you for this.
* Similarly, the log4j API has methods for parameterized logging: e.g.
{{logger.info("Calculation result is {}, based on input1={}, input2={} and
input3={}", result, input1, input2, input3);}} That feature has been removed
from CustomLogger. Again, I think this is a step back, not an improvement.
I think the kind of log events you want to send to splunk are more rare than
the normal logging that developers need. I would keep these separate:
CustomLogger for splunk, normal Log4j for normal logging.
If my fears are correct and your teams will only be allowed to use
CustomLogger, I would strongly suggest that you at least:
* revive the trace and debug levels - developers need to do more logging that
what you want to put into splunk
* revive the ability to log stack traces
* revive the ability to have parameterized logging
* you can discourage (but why? they are simply different use cases) non-splunk
logging by marking the methods with the @deprecated annotation. Putting a
customlogger_warning="deprecated method not recommended for prod" message in
the log is not very useful in my opinion.
Now, implementation style:
* The class declared as lowercase {{customLogger}}. The java convention is for
class names to start with an uppercase letter. The file is uppercase
{{CustomLogger.java}}, however. (Does that compile?)
* Some private methods start with uppercase. The Java convention is for methods
to start with lowercase. This compiles but looks sloppy.
* Some variable names use underscore to separate words, e.g. {{app_name}},
{{event_name}}. The Java convention is to use camelCase, so that should be
{{appName}}, {{eventName}}.
* {{static final int stringBuilderInitialCapacity}} is using camelCase. The
Java convention for constants is to use all uppercase, so that would be
something like {{INITIAL_CAPACITY}}.
* The {{validate...}} methods could be rewritten as one-liners: {{return action
!= null && action.length() >= 5;}}
* Result.success and Result.failure -> Result.SUCCESS, Result.FAILURE
(constants)
Performance:
* The current logic creates the formattedString regardless of whether the level
is enabled. I would recommend first checking {{if (logger.isEnabled(level,
...))}} before creating the formattedString object.
* Your biggest bottleneck will be the call to {{UUID.randomUUID()}} for every
log event. A quick test in a profiler should prove this. Note that this is done
in the application thread, so async loggers will not help here. Do you really
need this? If you configure a PatternLayout with %sequenceNumber every log
message will get a unique number in your log file with almost no overhead.
was (Author: rem...@yahoo.com):
Hmm... your request goes a bit beyond the scope of fixing bugs in Log4j. I'm
basically giving you free consulting... All right then, constructive criticism.
You better sit down first though, you may not like what I have to say...
First, please tell me that your development teams will be allowed to use the
"normal" Log4j2 API in addition to the CustomLogger, and the CustomLogger is
intended for optional use to send events to splunk. If your teams are forced to
use CustomLogger for _all_ their logging needs, you may want to get good life
insurance because your teams will really, really not like it. They will ask
questions like this:
* Why is there no trace or debug capability any more? Why provide a logger that
can only log info/warn/error/fatal? In my applications I only use INFO at
startup, and WARN/ERROR if there is a problem. By then it is too late and your
log file will not contain any detail on what lead to this error. This will make
it very difficult for your teams to solve issues, making the logging almost
useless...
* One of the most important functions of logging is error handling: a stack
trace in the log file is a huge, huge help in finding and fixing bugs.
CustomLogger has stripped out the methods that take a {{Throwable}} argument.
If your teams cannot use the normal API to log stack traces they will not thank
you for this.
* Similarly, the log4j API has methods for parameterized logging: e.g.
{{logger.info("Calculation result is {}, based on input1={}, input2={} and
input3={}", result, input1, input2, input3);}} That feature has been removed
from CustomLogger. Again, I think this is a step back, not an improvement.
I think the kind of log events you want to send to splunk are more rare than
the normal logging that developers need. I would keep these separate:
CustomLogger for splunk, normal Log4j for normal logging.
If my fears are correct and your teams will only be allowed to use
CustomLogger, I would strongly suggest that you at least:
* revive the trace and debug levels - developers need to do more logging that
what you want to put into splunk
* revive the ability to log stack traces
* revive the ability to have parameterized logging
* you can discourage (but why? they are simply different use cases) non-splunk
logging by marking the methods with the @deprecated annotation. Putting a
customlogger_warning="deprecated method not recommended for prod" message in
the log is not very useful in my opinion.
Now, implementation style:
* The class declared as lowercase {{customLogger}}. The java convention is for
class names to start with an uppercase letter. The file is uppercase
{{CustomLogger.java}}, however. (Does that compile?)
* Some private methods start with uppercase. The Java convention is for methods
to start with lowercase. This compiles but looks sloppy.
* Some variable names use underscore to separate words, e.g. {{app_name}},
{{event_name}}. The Java convention is to use camelCase, so that should be
{{appName}}, {{eventName}}.
* {{static final int stringBuilderInitialCapacity}} is using camelCase. The
Java convention for constants is to use all uppercase, so that would be
something like {{INITIAL_CAPACITY}}.
* The {{validate...}} methods could be rewritten as one-liners: {{return action
!= null && action.length() >= 5;}}
* Result.success and Result.failure -> Result.SUCCESS, Result.FAILURE
(constants)
Performance:
* The current logic creates the formattedString regardless of whether the level
is enabled. I would recommend first checking {{if (logger.isEnabled(level, ...)
}} before creating the formattedString object.
* Your biggest bottleneck will be the call to {{UUID.randomUUID()}} for every
log event. A quick test in a profiler should prove this. Note that this is done
in the application thread, so async loggers will not help here. Do you really
need this? If you configure a PatternLayout with %sequenceNumber every log
message will get a unique number in your log file with almost no overhead.
> Custom Logger with restrictions on existing methods
> ---------------------------------------------------
>
> Key: LOG4J2-695
> URL: https://issues.apache.org/jira/browse/LOG4J2-695
> Project: Log4j 2
> Issue Type: Bug
> Components: API
> Reporter: SIBISH BASHEER
> Labels: customlogger
> Attachments: AppAsyncMain.java, CustomLogger.java, CustomLogger.java,
> final code custom logger.zip
>
>
> I have been looking at the Custom/Extended logger discussions. But none of
> them seems to fulfil what i am looking for.
> 1) I want custom methods as below:
> {code}
> private static CustomLogger logger =
> CustomLogger.getLogger(AppAsyncMain.class);
>
> logger.info( transaction_id, app_name + event_name +
> "inside the loop" + "inside the loop of
> the sample app" +
> "success" + "looped in" + "loop_count" +
> String.valueOf(i));
> {code}
>
> log:
> {code}
> 2014-06-30 16:09:28,268 log_level="INFO" thread_name="main"
> class_name="com.custom.samplelog4j.AppAsyncMain"
> transaction_id="79ea1071-9565-405a-aa18-75d271694bf2"
> event_id="dd5c69c0-4400-41fd-8a2e-5d538d8e8c9b" app="Sample Logging SDK App"
> event_name="Sample Event" action="start of sample app" desc="start of api"
> result="success" reason="start" token="abcdefg" alias="a...@gmail.com"
> {code}
>
> 2) I want to show warning in existing logger methods so the teams using the
> custom logger doesn't use these methods other than for testing:
> {code}
> logger.info("start of statement");
> {code}
>
> log:
> {code}
> 2014-06-30 16:12:31,065 log_level="INFO" thread_name="main"
> class_name="com.custom.samplelog4j2.AppAsyncMain" start of statement
> customlogger_warning="method not recommended for production use"
> {code}
>
> 3) Custom validations for the fields:
> {code}
> private static String validateFields(String app_name, String event_name,
> String action, String desc, Result result, String
> reason) {
> String validateStatus = "";
> if (!ValidateAppName(app_name)) {
> validateStatus = "app_name";
> } else if (!ValidateEventName(event_name)) {
> validateStatus = "event_name";
> } else if (!ValidateAction(action)) {
> validateStatus = "action";
> } else if (!ValidateDesc(desc)) {
> validateStatus = "desc";
> } else if (!ValidateReason(result, reason)) {
> validateStatus = "reason";
> }
> return validateStatus;
> }
> {code}
> Options tried:
> 1.
> * extended ExtendedLoggerWrapper
> * created the map of the Custom logger
> * This option was failing because of "writing to a closed appender"
> * Attached is the code "CustomLogger.java"
>
> 2. Modified the AbstractLogger in Trunk and added the below methods:
> {code}
> @Override
> public void info(final String message) {
> String updtMessage = message + " amexlogger_error=\"Incorrect method
> used\"";
> logIfEnabled(FQCN, Level.INFO, null, updtMessage, (Throwable) null);
> }
> public void info(final String transactionId, final String app_name, final
> String event_name, final String action, final String desc, final String
> result, final String reason, final String... moreFields) {
> String message = "transaction_id=" + transactionId + " " + "app_name="
> + app_name + " " + "event_name=" + event_name + " " + "action=" + action;
>
> logIfEnabled(FQCN, Level.INFO, null, message, (Throwable) null);
> }
> {code}
> I don't want to modify the methods inside the log4j-api.
>
> Please help me with the correct approach on how to use log4j2 for this
> usecase.
> Thanks
> Sibish
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org
