[#126] Changing the log level in config.json does not work #187

Eclipse-Dominator · 2023-06-02T23:38:14Z

Fixes #126

The current way of initializing loggers causes all loggers to default to INFO level, even if a different level is specified via the config.json or programmatically.

Let's create a logger named ab3 to serve as the base logger, and automatically prefix all loggers created using LogCenter#getLogger(...) with ab3. so that they all become descendants of the base logger. This allows us to change the logging level of all loggers simply by changing
the logging level of the base logger (unless those loggers have specified their own logging level).

Implications: Loggers not created using the LogCenter#getLogger(...) methods will not come under the above control mechanism unless their name starts with ab3..

A simple demo for log level = FINEST

A simple demo for log level = WARNING

There is a small change in the behavior of init, previously, changing the log level will cause all subsequent loggers to have their log level changed, however, currently, this implementation changes all loggers created from the application.

However, some classes like AddressBook etc are created before log level is being updated and I am afraid that this might defeat the purpose of changing the log levels.

Another minor thing I noticed: comment

The current code base mostly uses INFO and I believe it might be more helpful if we reclassify some of the logs to use warning and higher like changing logs caused by incorrect command to WARN and logs caused by IOException to FATAL/ERROR.

canihasreview · 2023-06-02T23:38:17Z

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

codecov · 2023-06-02T23:43:58Z

Codecov Report

Merging #187 (6e7104b) into master (c9c9512) will decrease coverage by 0.14%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master     #187      +/-   ##
============================================
- Coverage     73.96%   73.82%   -0.14%     
+ Complexity      420      415       -5     
============================================
  Files            71       71              
  Lines          1279     1276       -3     
  Branches        126      126              
============================================
- Hits            946      942       -4     
- Misses          301      302       +1     
  Partials         32       32

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`78.78% <88.88%> (-4.55%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

canihasreview · 2023-06-02T23:45:18Z

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Fix log level not affecting loggers

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

canihasreview · 2023-06-05T05:23:45Z

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Fix log level not affecting loggers

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

src/main/java/seedu/address/commons/core/LogsCenter.java

canihasreview · 2023-06-06T22:44:55Z

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Fix log level not affecting loggers

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Eclipse-Dominator · 2023-06-06T22:47:10Z

A consideration is whether we should have a method that creates loggers that ignore the current log level such as the logger that prints
===[ Initializing AddressBook ]=== and === [ Stopping Address Book ] ===,

damithc · 2023-06-08T11:42:27Z

A consideration is whether we should have a method that creates loggers that ignore the current log level such as the logger that prints
===[ Initializing AddressBook ]=== and === [ Stopping Address Book ] ===,

As per the current PR, will they be skipped if the logging level is set to higher than INFO?

damithc

Just a few questions ... to help me understand the code better.

src/main/java/seedu/address/commons/core/LogsCenter.java

Eclipse-Dominator · 2023-06-09T02:35:57Z

A consideration is whether we should have a method that creates loggers that ignore the current log level such as the logger that prints
===[ Initializing AddressBook ]=== and === [ Stopping Address Book ] ===,

As per the current PR, will they be skipped if the logging level is set to higher than INFO?

Yes, as per current PR, this will indeed be skipped

canihasreview · 2023-06-09T03:07:31Z

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/1] Fix log level not affecting loggers

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Just a follow up question

src/main/java/seedu/address/commons/core/LogsCenter.java

damithc

Some cosmetic suggestions

src/main/java/seedu/address/commons/core/LogsCenter.java

canihasreview · 2023-06-14T17:57:52Z

v5

@Eclipse-Dominator submitted v5 for review.

[v5 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/5/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Eclipse-Dominator · 2023-06-14T18:07:50Z

After some experimentation, I think using the inheritance of base loggers will lead to a much cleaner and simpler code as compared to statically storing both file and console handlers.

The new changes here still enables the following:

tweaking log level of child loggers will still function as expected unlike in before this PR
In a hypothetical scenario where the parent logger is created after the child logger, the child logger will still inherit from the parent logger as long as the child logger isn't tweaked manually.

damithc

Added a query and a suggestion.

src/main/java/seedu/address/commons/core/LogsCenter.java

damithc · 2023-06-15T15:23:10Z

src/main/java/seedu/address/commons/core/LogsCenter.java

+        Logger baseLogger = Logger.getLogger(BASE_LOGGER_NAME);
+        baseLogger.setUseParentHandlers(false);
+        removeHandlers(baseLogger);
+        baseLogger.addHandler(createConsoleHandler(Level.ALL));


Why use Level.ALL here?

This is to ensure that all the handlers will log messages regardless of their level. So we dont need to track these loggers as a global variable in the class.

Since the logger is the one generating the messages, we dont want the level of the handlers to be higher than the log level of the loggers as this will cause the messages of the loggers to be ignored by the handler. (e.g. logger set to FINEST but handler is at INFO)

Since both handlers will always all messages by generated by the logger, there is no need to purposefully set the log level of each handler. Furthermore, setting these handlers to ALL won't impact performance since they are just comparing the log levels of the messages.

damithc

Couple of things I missed out earlier

src/main/java/seedu/address/commons/core/LogsCenter.java

damithc · 2023-06-16T17:32:56Z

@se-edu/tech-team-level1 for your review ...

canihasreview · 2023-06-17T15:20:47Z

v11

@Eclipse-Dominator submitted v11 for review.

[v11 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/11/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

SPWwj

The pull request functions as anticipated and successfully addresses the issue. The code adheres to standards, following the prescribed formatting guidelines.

LGTM!

However, I noticed in the provided screenshot that the default logging level is set to 'info', even though all the messages pertain to configuration. It might be more appropriate to set the logging level to 'config'. This observation may lead to a new issue.

damithc · 2023-06-26T07:24:23Z

However, I noticed in the provided screenshot that the default logging level is set to 'info', even though all the messages pertain to configuration. It might be more appropriate to set the logging level to 'config'. This observation may lead to a new issue.

Interesting suggestion @SPWwj We can look into this separately.

damithc

I found some of the comments have the right description but they appear at the wrong place. While tweaking them, I also encountered a method that may not be the right abstraction.

src/main/java/seedu/address/commons/core/LogsCenter.java

damithc · 2023-06-26T10:05:34Z

src/main/java/seedu/address/commons/core/LogsCenter.java

+    private static Logger getBaseLogger() {
+        Logger baseLogger = Logger.getLogger("ab3");


The current role of this method doesn't feel right. It returns something that is meant to be used as the baseLogger but it has no control over how it will be used.
The following feels like a better fit for what the method does.

Suggested change

private static Logger getBaseLogger() {

Logger baseLogger = Logger.getLogger("ab3");

private void setBaseLogger() {

baseLogger = Logger.getLogger("ab3");

It can be called inside the init() method. What do you think?

I would still prefer the original method, as init() is only called after the config file is being read.
This will cause the baseLogger to be uninitialized before then.

To prevent this, LogsCenter might need to switch to using a getter method to retrieve the baseLogger (initialize it if it is not defined) which is not really ideal since baseLogger itself is a private attribute of LogsCenter

I don't think that is a major problem, compared to an abstraction that doesn't sound right. But if you want something to execute earlier, you can use a static code block. See CommandTestUtil.java for an example. Would that work? e.g.,

static { setBaseLogger(); }

canihasreview · 2023-06-30T20:49:39Z

v12

@Eclipse-Dominator submitted v12 for review.

[v12 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/12/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

SPWwj · 2023-07-01T05:30:01Z

src/main/java/seedu/address/commons/core/LogsCenter.java

+            FileHandler fileHandler = new FileHandler(LOG_FILE, MAX_FILE_SIZE_IN_BYTES, MAX_FILE_COUNT, true);
+            fileHandler.setFormatter(new SimpleFormatter());
+            fileHandler.setLevel(Level.ALL);


This could be refractor as a new function private static setFileHandler()

@SPWwj Yup, if we go for strict SLAP, this can be extracted as a separate method. But in some cases it is OK to keep two levels inside a method, to reduce unnecessary method proliferation. So, a code block preceded by a comment that explains what the code block does is as good as extracting that code block as a method, provided that code block is needed only once.

Occam's Razor 😂

canihasreview · 2023-07-03T12:28:59Z

v13

@Eclipse-Dominator submitted v13 for review.

[v13 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v12 and v13) (📈 Range-Diff between v12 and v13)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/13/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Just a few cosmetic suggestions.

src/main/java/seedu/address/commons/core/LogsCenter.java

damithc · 2023-07-03T13:33:00Z

src/main/java/seedu/address/commons/core/LogsCenter.java

+    // The logger that will be used as the parent of all other loggers created by this class.
+    private static final Logger logger;
+    private static Logger baseLogger;


Suggested change

// The logger that will be used as the parent of all other loggers created by this class.

private static final Logger logger;

private static Logger baseLogger;

private static Logger baseLogger; // to be used as the parent of all loggers

private static final Logger logger; // logger for this class

Perhaps just end-of-line comments (as suggested above) are enough here?
I put baseLogger above logger because I felt it is more intuitive to learn about baseLogger first. But either order is OK.

Any reason why one is final but the other is not?

src/main/java/seedu/address/commons/core/LogsCenter.java

canihasreview · 2023-07-10T13:03:34Z

v14

@Eclipse-Dominator submitted v14 for review.

[v14 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v13 and v14) (📈 Range-Diff between v13 and v14)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/14/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc · 2023-07-10T16:41:30Z

@se-edu/tech-team-level1 for your review ...

@SPWwj the code has changed since your last review.

damithc

No further comments from me. Thanks for the persistence with this PR @Eclipse-Dominator

src/main/java/seedu/address/commons/core/LogsCenter.java

SPWwj

LGTM, just some nitty-gritty changes to make the message more consistent.

The current way of initializing loggers causes all loggers to default to INFO level, even if a different level is specified via the config.json or programmatically. Let's create a logger named `ab3` to serve as the base logger, and automatically prefix all loggers created using LogCenter#getLogger(...) with `ab3.` so that they all become descendants of the base logger. This allows us to change the logging level of all loggers simply by changing the logging level of the base logger (unless those loggers have specified their own logging level). Implications: Loggers not created using the LogCenter#getLogger(...) methods will not come under the above control mechanism unless their name starts with `ab3.`.

canihasreview · 2023-07-11T21:52:25Z

v15

@Eclipse-Dominator submitted v15 for review.

[v15 1/1] Fix log levels not affecting loggers

(📚 Archive) (📈 Interdiff between v14 and v15) (📈 Range-Diff between v14 and v15)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/187/15/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

canihasreview bot added the s.ToReview label Jun 2, 2023

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch 2 times, most recently from ac5083b to e2e1df5 Compare June 4, 2023 14:26

damithc requested changes Jun 6, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch 2 times, most recently from 9047b48 to 2649a9f Compare June 6, 2023 22:43

canihasreview bot requested a review from damithc June 6, 2023 22:44

damithc reviewed Jun 8, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/commons/core/LogsCenter.java Show resolved Hide resolved

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch from 2649a9f to 0c8720c Compare June 9, 2023 03:06

canihasreview bot requested a review from damithc June 9, 2023 03:07

damithc reviewed Jun 9, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

damithc requested changes Jun 14, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch 4 times, most recently from 02f5871 to 075ca3a Compare June 14, 2023 17:57

canihasreview bot requested a review from damithc June 14, 2023 17:57

damithc reviewed Jun 15, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch from 075ca3a to deb33a6 Compare June 15, 2023 16:54

canihasreview bot requested a review from damithc June 16, 2023 15:49

damithc requested changes Jun 16, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch 2 times, most recently from 7ba7de2 to 8775e85 Compare June 17, 2023 15:20

canihasreview bot requested a review from damithc June 17, 2023 15:20

SPWwj approved these changes Jun 17, 2023

View reviewed changes

damithc requested changes Jun 26, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch 2 times, most recently from d89264d to a93f10a Compare June 30, 2023 20:48

canihasreview bot requested a review from damithc June 30, 2023 20:49

SPWwj reviewed Jul 1, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch from a93f10a to 7e88b47 Compare July 3, 2023 12:28

damithc reviewed Jul 3, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch from 7e88b47 to d5599da Compare July 10, 2023 13:01

canihasreview bot requested a review from damithc July 10, 2023 13:03

damithc requested a review from SPWwj July 10, 2023 16:41

damithc approved these changes Jul 10, 2023

View reviewed changes

SPWwj reviewed Jul 11, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/LogsCenter.java Outdated Show resolved Hide resolved

SPWwj suggested changes Jul 11, 2023

View reviewed changes

Eclipse-Dominator force-pushed the 126-changing-the-log-level-in-config-json-does-not-work branch from d5599da to 6e7104b Compare July 11, 2023 21:51

damithc merged commit 4592363 into se-edu:master Jul 19, 2023
14 of 15 checks passed

		private static Logger getBaseLogger() {
		Logger baseLogger = Logger.getLogger("ab3");

[#126] Changing the log level in config.json does not work #187

[#126] Changing the log level in config.json does not work #187

Conversation

Eclipse-Dominator commented Jun 2, 2023 • edited Loading

canihasreview bot commented Jun 2, 2023

codecov bot commented Jun 2, 2023 • edited Loading

Codecov Report

canihasreview bot commented Jun 2, 2023

v1

canihasreview bot commented Jun 5, 2023

v2

canihasreview bot commented Jun 6, 2023

v3

Eclipse-Dominator commented Jun 6, 2023

damithc commented Jun 8, 2023

damithc left a comment

Choose a reason for hiding this comment

Eclipse-Dominator commented Jun 9, 2023

canihasreview bot commented Jun 9, 2023

v4

damithc left a comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jun 14, 2023

v5

Eclipse-Dominator commented Jun 14, 2023 • edited Loading

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

damithc commented Jun 16, 2023

canihasreview bot commented Jun 17, 2023

v11

SPWwj left a comment

Choose a reason for hiding this comment

damithc commented Jun 26, 2023

damithc left a comment

Choose a reason for hiding this comment

damithc Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jun 30, 2023

v12

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 3, 2023

v13

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 10, 2023

v14

damithc commented Jul 10, 2023

damithc left a comment

Choose a reason for hiding this comment

SPWwj left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 11, 2023

v15

Eclipse-Dominator commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading

Eclipse-Dominator commented Jun 14, 2023 •

edited

Loading

damithc Jun 26, 2023 •

edited

Loading