Write failed files (errors) to an errors.log and summarize to console by yma955 · Pull Request #126 · Commonjava/charon

yma955 · 2021-12-21T10:41:11Z

No description provided.

coveralls · 2021-12-21T10:46:46Z

Pull Request Test Coverage Report for Build 1767741200

7 of 31 (22.58%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.6%) to 78.666%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
charon/config.py	0	1	0.0%
charon/pkgs/pkg_utils.py	1	3	33.33%
charon/pkgs/maven.py	0	4	0.0%
charon/cmd/command.py	1	6	16.67%
charon/utils/logs.py	4	16	25.0%

Files with Coverage Reduction	New Missed Lines	%
charon/pkgs/pkg_utils.py	1	68.0%

Totals
Change from base Build 1767714858:	-0.6%
Covered Lines:	1309
Relevant Lines:	1664

💛 - Coveralls

ligangty

Generally looks good to me except two changes. And as this is not urgent, we can hold it later for John's review to see if this is what he want.

ligangty · 2021-12-22T03:20:05Z

charon/config.py

 CONFIG_FILE = "charon.yaml"

 logger = logging.getLogger(__name__)
+add_file_handler(logger)


Maybe we should extract these two lines together in logs.py as a new method like get_charon_logger(name: str)? So if we want to change something in future, we don't need to change it everywhere.

@ligangty done, wrap it in the getLogger

ligangty · 2021-12-22T03:23:10Z

charon/utils/logs.py

+
+
+def add_file_handler(logger: logging):
+    handler = logging.FileHandler('errors.log')


Can we make this "errors.log" configurable for different location? I think we can configure this in CharonConfig, and give it a default location like this "./errors.log".

@ligangty If we introduce the CharonConfig in logs.py, it will cause the error of partially initialized, it's likely a circular import issue, otherwise, we could use another config(or log properties) rather than CharonConfig instead.

Or maybe we can let the config.py not use this getLogger method, as the error in CharonConfig is not our target to record in the errors.log.

ligangty · 2022-01-28T01:48:25Z

@yma88 Seems this pr needs to do rebase on new code base. Can you help do it?

ligangty · 2022-01-28T02:43:55Z

charon/utils/logs.py

+    formatter = logging.Formatter('%(asctime)s - %(levelname)s - %(message)s')
+    handler.setFormatter(formatter)
+    handler.setLevel(logging.ERROR)
+    handler.setLevel(logging.WARN)


Why setLevel twice here?

@ligangty Removed the duplicated unuseful level setting.

ligangty · 2022-01-28T05:53:28Z

@yma88 I just double checked the code. Seems we still need some changes.
We will not use this error logger for all logs except warning+error ones, but seems your change covered all, including the debug+info ones.
And I'm thinking if we could use "${prod+versin}.error.log" as the error log files, because we need to differentiate error logs between different product tarballs.

yma955 · 2022-01-29T03:44:21Z

@ligangty here are the updates:

ERROR_LOG_LOCATION is the env to set errors.log folder
I use a private static(thread-safe) dict to store the product&version, which only be set when the upload/delete operations actually start to execute.
For the log file appender, I don't put it into set_logging(init), as the name of file should be set during running.
Nothing about inject, just set the appender in proper places, such as the class init, public methods in py files, or necessary private methods. Not so much and have been verified all error/warning could be covered.

ligangty

Little curious. Can we just do all handler stuff once in command level? As the set_logging also did handler related stuff, we can add a new optional param like set_logging(name="charon", level=logging.DEBUG, handler=None, error_log_file=None), and then pass the error log file in during the command init. In this way we don't need any changes in other files(or just little change), and don't need that ProductVersion class. Are there any runtime problems to block all changes in this single method?

yma955 · 2022-01-29T10:55:55Z

Little curious. Can we just do all handler stuff once in command level? As the set_logging also did handler related stuff, we can add a new optional param like set_logging(name="charon", level=logging.DEBUG, handler=None, error_log_file=None), and then pass the error log file in during the command init. In this way we don't need any changes in other files(or just little change), and don't need that ProductVersion class. Are there any runtime problems to block all changes in this single method?

@ligangty I'm afraid we can't pass the error_log stuff through set_logging, it's triggered during init but we need to have the product&version when the command executes. For the ProductVersion, it's using the thread-safe static, it's necessary, and I've thought about it should be the harmless and safe way to do so, if we wanna have this info widely used within a single process.

ligangty · 2022-01-30T01:04:29Z

@yma88 For the init triggering, do yo mean set_logging() called in init.py? If so, maybe we can consider to move it out to command.py too.

ligangty · 2022-01-30T01:46:40Z

@yma88 I've done a pr for this set_logging moving here: #129

yma955 · 2022-01-30T04:52:52Z

@ligangty thanks, I've merged #129, and forced to commit the fixing based on it.

ligangty

LGTM

yma955 force-pushed the appender branch from 3a17c17 to 45c8c47 Compare December 21, 2021 10:47

yma955 requested a review from ligangty December 21, 2021 11:01

ligangty reviewed Dec 22, 2021

View reviewed changes

ligangty requested a review from jdcasey December 22, 2021 03:25

yma955 force-pushed the appender branch from 7d18a35 to 4dbb484 Compare December 22, 2021 05:30

ligangty reviewed Jan 28, 2022

View reviewed changes

yma955 force-pushed the appender branch 2 times, most recently from c3778b0 to 94742db Compare January 28, 2022 05:15

yma955 force-pushed the appender branch from d650e40 to a06c01c Compare January 29, 2022 04:33

ligangty reviewed Jan 29, 2022

View reviewed changes

yma955 added 3 commits January 30, 2022 12:37

Write failed files (errors) to an errors.log and summarize to console

bf83eb2

Rewrap the logger

2675740

Remove duplicated log level setting for handler

4fbe73e

yma955 force-pushed the appender branch from a06c01c to 0b5b573 Compare January 30, 2022 04:45

Define errors log location and name it with product & version

9fd6522

yma955 force-pushed the appender branch from 0b5b573 to 9fd6522 Compare January 30, 2022 04:49

ligangty approved these changes Jan 30, 2022

View reviewed changes

yma955 merged commit ba3276b into Commonjava:main Jan 30, 2022



		def add_file_handler(logger: logging):
		handler = logging.FileHandler('errors.log')

Conversation

yma955 commented Dec 21, 2021

Uh oh!

coveralls commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1767741200

💛 - Coveralls

Uh oh!

ligangty left a comment

Choose a reason for hiding this comment

Uh oh!

ligangty Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

yma955 Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

ligangty Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

yma955 Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

ligangty Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

ligangty commented Jan 28, 2022

Uh oh!

ligangty Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

yma955 Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

ligangty commented Jan 28, 2022

Uh oh!

yma955 commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ligangty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yma955 commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ligangty commented Jan 30, 2022

Uh oh!

ligangty commented Jan 30, 2022

Uh oh!

yma955 commented Jan 30, 2022

Uh oh!

ligangty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Dec 21, 2021 •

edited

Loading

yma955 commented Jan 29, 2022 •

edited

Loading

ligangty left a comment •

edited

Loading

yma955 commented Jan 29, 2022 •

edited

Loading