Ericsson
Define logging levels formally
Define the logging levels for ecChronos. The folloowing should be done:
- Define log levels. That is, what is the criteria for each level (info, warn, debug etc).
- Add this information to the CONTRIBUTING document.
- Clean up the code according to the definition.
- Fix logback so debug is not enabled by default.
Are these a good starting point?
https://www.javatpoint.com/log4j-logging-levels
My suggestion.
First, the basic levels (and order) are as follows: all > trace > debug > info > warn > error > off
Second, the details for each level:
- all: All levels will be logged. Example: -
- trace: Detailed debugging (flows, request/response, details, etc.). Will have a performance impact and is therefore not for production unless it is a planned trouble shooting activity. Mainly used during development. Example: Every method call is logged in detail for a certain request and response flow. Used data is logged in detail.
- debug: Simple debug logging which can be turned on and used in production if necessary (should have no impact on performance). The logs at this level should be of the type a developer might need to spot a quick fix to a problem or to at least isolate the problem further. Example: Specific events with contextual details.
- info: For logging the normal flow and operation of the service(s). Example: Service health, progress of requests/responses etc.
- warn: Behavior in the service(s) which are unexpected and potentially could lead to problems but were handled for the moment. However, service(s) as such are working normally and as expected. Example: A primary service switching to a secondary one, connection retries, reverting to defaults etc. Should always be investigated.
- error: A service fail in the sense it cannot response to requests or data processed cannot be trusted/used. Example: Connection attempts that ultimate fail. Crucial resources not available.
- off: No levels will be logged. Example: -
Third; The use of the throttling logger should be constrained to events expected to produce intensive output. It should not be used in a generic manner, such as "throttle all debug" etc.
Fourth; The use of isEnabled logging methods should only be used in situations where a performance penalty will be incurred due to unnecessary heavy processing not logged. These could be necessary because of the PMD at the moment though. Also, use only parameterized messages (e.g. "Hello {}!", world) when logging. Don't do String concatenations (e.g. "Hello " + "world!").
gone through the codebase and there are these number of log entries debug: 47 error: 52 info: 17 (seems low according to the definition above) trace: 18 warn: 42
log.debug.txt log.error.txt log.info.txt log.trace.txt log.warn.txt
I agree, Johans suggesting seams fine. But just to expand slightly on isEnabled(), if you have to do some thing like ("Hello {}!", random_method()), this is a case where I think isEnabled() could be a good thing.
Yes, all this looks good from my point of view, including Tommy's last comment.
I agree, Johans suggesting seams fine. But just to expand slightly on isEnabled(), if you have to do some thing like ("Hello {}!", random_method()), this is a case where I think isEnabled() could be a good thing.
Yep. That's the idea of the fourth point. Perhaps it should be expressed more clearly than I did. ;-)
Looking at the code in some places LOG.is..Enabled are used and in some places it is not. I assume we want it to be used everywhere for trace, debug, info etc.?
git grep -i -n "LOG.is"
application/src/main/java/com/ericsson/bss/cassandra/ecchronos/application/ECChronosInternals.java:234: if (LOG.isTraceEnabled()) core/src/main/java/com/ericsson/bss/cassandra/ecchronos/core/repair/state/RepairStateImpl.java:175: if (LOG.isDebugEnabled()) core/src/main/java/com/ericsson/bss/cassandra/ecchronos/core/repair/state/RepairStateImpl.java:192: if (LOG.isDebugEnabled()) core/src/main/java/com/ericsson/bss/cassandra/ecchronos/core/repair/state/RepairStateImpl.java:212: if (LOG.isInfoEnabled()) core/src/test/resources/cassandra.yaml:379:# and the CommitLog is simply synced every commitlog_sync_period_in_ms
if there are no complaints on the descriptions on the log levels before the end of this week the current ones wll be used/kept
Or should the guideline in CONTRIBUTION.md be that if building the log message requires calculations or collecting data the is....Enabled should be used.
Ett exempel från nuvarande kod. Alla log meddelande som byggs upp på liknande sätt borde ju använda isEnabled().
if (LOG.isDebugEnabled()) { long next = repairedAt + runIntervalInMs; if (old != null) { next -= old.getEstimatedRepairTime(); } LOG.debug("Table {} partially repaired at {}, next repair at/after {}", myTableReference, MY_DATE_FORMAT.get().format(new Date(repairedAt)), MY_DATE_FORMAT.get().format(new Date(next))); }
borde kanske finnas med i CONTRIBUTION.md också. som exempel
Ett exempel från nuvarande kod. Alla log meddelande som byggs upp på liknande sätt borde ju använda isEnabled().
if (LOG.isDebugEnabled()) { long next = repairedAt + runIntervalInMs; if (old != null) { next -= old.getEstimatedRepairTime(); } LOG.debug("Table {} partially repaired at {}, next repair at/after {}", myTableReference, MY_DATE_FORMAT.get().format(new Date(repairedAt)), MY_DATE_FORMAT.get().format(new Date(next))); }
borde kanske finnas med i CONTRIBUTION.md också. som exempel
I think that this is definitely an example where we should use isEnabled. Maybe it should also be evaluated if it should be DEBUG or TRACE according to the definition.
What can be seen in other Similar Ericsson products the configurable loglevel are DEBUG, INFO, WARN, and ERROR. then I assume all trace should be changed to debug to align with this.
What can be seen in other Similar Ericsson products the configurable loglevel are DEBUG, INFO, WARN, and ERROR. then I assume all trace should be changed to debug to align with this.
We should follow the rules we define in this project, if you like to propose a new definition you have to write it here so everyone can read.