Use `COBalD` log channels for logging in `TARDIS` #146

giffels · 2020-04-28T13:09:44Z

This pull request introduces logging channels as suggested in COBalDs documentation and #137 correspondingly. This will improve the user's ability to filter log messages according their needs.

eileen-kuehn · 2020-05-14T06:58:56Z

Oh that is great! I am really looking forward on how practical this is going to be. If it is as good as expected, we should also go for it in other projects! :)

eileen-kuehn

Many log levels did change during this PR. Some also from debug to warning. It might be helpful to introduce a helping information for maintainers on what actually the reasoning for selecting proper levels is.

eileen-kuehn · 2020-05-14T07:03:44Z

tardis/adapters/batchsystems/htcondor.py

+                logger.info(f"Draining failed with: {str(cef)}")
+                logger.info(


Why is the log level changed from debug to info?

eileen-kuehn · 2020-05-14T07:07:14Z

tardis/adapters/sites/cloudstack.py

-                logging.info("Quota exceeded")
-                logging.debug(str(ce))
+                logger.warning("Quota exceeded")
+                logger.warning(str(ce))


Here are also interesting changes of log level, info -> warning and debug -> warning. Especially the jump from debug to warning is interesting. As this is actually not two warnings, we might think about joining the strings?

giffels · 2020-05-15T09:16:48Z

Many log levels did change during this PR. Some also from debug to warning. It might be helpful to introduce a helping information for maintainers on what actually the reasoning for selecting proper levels is.

Yes, many log levels did change since there was no convention on this. :-/

@maxfischer2781 has proposed something in #137.

Log Levels: We have the standard Python log levels. IMO these should be:

CRITICAL: COBalD or TARDIS can't go on anymore and shuts down

ERROR: Basically unused.

WARNING: Recoverable errors. Dropped connections, timeouts, the like. Default for monitoring health when you know things generally work.

INFO: High-level description of what goes on. Default for monitoring behaviour when you want to understand why things happen.

DEBUG: Low-level/Implementation details. If things go wrong and you want to report them.

I would suggest to add this to the COBalDdocumentation. @maxfischer2781, would do you think about this?

maxfischer2781 · 2020-05-15T12:12:10Z

Yes, adding the log level convention as well would be helpful.

giffels · 2020-05-15T13:29:42Z

Yes, adding the log level convention as well would be helpful.

Thanks for opening an issue.

giffels · 2020-05-22T07:35:30Z

Hi @rcaspart, @eileen-kuehn, @maxfischer2781,

this pull request needs two reviews. ;-)

Thanks,
Manuel

maxfischer2781

Log channels and levels look okay to me. Thanks for cleaning this up.

rcaspart

thanks a lot for the introduction of the log channels and cleaning up of the log levels.
I have a few minor suggestions related to consistent usage of assertLogs in the unittests. Can you please have a look at them?

tests/utilities_t/test_asynccachemap.py

tests/adapters_t/batchsystems_t/test_htcondor.py

Co-authored-by: Rene Caspart <rene.caspart@cern.ch>

codecov-commenter · 2020-05-25T07:56:07Z

Codecov Report

Merging #146 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   99.07%   99.08%           
=======================================
  Files          40       40           
  Lines        1520     1524    +4     
=======================================
+ Hits         1506     1510    +4     
  Misses         14       14

Impacted Files	Coverage Δ
tardis/adapters/batchsystems/htcondor.py	`100.00% <100.00%> (ø)`
tardis/adapters/sites/cloudstack.py	`100.00% <100.00%> (ø)`
tardis/adapters/sites/htcondor.py	`100.00% <100.00%> (ø)`
tardis/adapters/sites/moab.py	`100.00% <100.00%> (ø)`
tardis/adapters/sites/openstack.py	`100.00% <100.00%> (ø)`
tardis/adapters/sites/slurm.py	`100.00% <100.00%> (ø)`
tardis/plugins/prometheusmonitoring.py	`100.00% <100.00%> (ø)`
tardis/plugins/sqliteregistry.py	`100.00% <100.00%> (ø)`
tardis/plugins/telegrafmonitoring.py	`100.00% <100.00%> (ø)`
tardis/resources/drone.py	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad7a8e5...4e6cd21. Read the comment docs.

giffels · 2020-05-25T07:56:45Z

thanks a lot for the introduction of the log channels and cleaning up of the log levels.
I have a few minor suggestions related to consistent usage of assertLogs in the unittests. Can you please have a look at them?

Thanks for your suggestions, I have applied them to the code.

rcaspart

Thanks, looks good to go 👍

maxfischer2781

Whoops, sorry for the delay. Go for it!

eileen-kuehn

Go for it! 👍

giffels force-pushed the add-log-channels branch 2 times, most recently from fc81414 to 59b157a Compare May 13, 2020 14:29

giffels marked this pull request as ready for review May 13, 2020 14:29

giffels changed the title ~~WIP: Use COBalD log channels for logging in TARDIS~~ Use COBalD log channels for logging in TARDIS May 13, 2020

giffels added the Improvement Code Improvements label May 13, 2020

giffels requested review from a team, rcaspart, eileen-kuehn and maxfischer2781 and removed request for a team May 13, 2020 14:41

eileen-kuehn reviewed May 14, 2020

View reviewed changes

giffels mentioned this pull request May 14, 2020

Monitoring plugin for ElasticSearch #143

Merged

giffels added 4 commits May 14, 2020 13:29

Add log channels to plugins

2ad136c

Add log channels to resources

59d88f5

Add log channels to adapters

f3931c6

Add log channels to utilities

ec41685

giffels force-pushed the add-log-channels branch from 59b157a to ec41685 Compare May 14, 2020 11:30

maxfischer2781 mentioned this pull request May 15, 2020

Document log level convention MatterMiners/cobald#79

Open

Adjust log levels according to convention

49f1235

giffels linked an issue May 15, 2020 that may be closed by this pull request

[RFC] Log levels in COBalD / TARDIS #137

Closed

Add changelog

0425a55

giffels added this to the 0.4.0 - Release milestone May 15, 2020

giffels requested a review from eileen-kuehn May 15, 2020 14:42

giffels mentioned this pull request May 15, 2020

Slurm batchsystem adapter #129

Merged

maxfischer2781 previously approved these changes May 22, 2020

View reviewed changes

rcaspart requested changes May 25, 2020

View reviewed changes

Unify syntax for assertLogs

62e285a

Co-authored-by: Rene Caspart <rene.caspart@cern.ch>

giffels dismissed maxfischer2781’s stale review via 62e285a May 25, 2020 07:56

Merge branch 'master' into add-log-channels

7499b35

giffels requested review from rcaspart and maxfischer2781 May 25, 2020 07:59

Add missing logging import

4e6cd21

rcaspart approved these changes May 25, 2020

View reviewed changes

maxfischer2781 approved these changes Jun 2, 2020

View reviewed changes

eileen-kuehn approved these changes Jun 2, 2020

View reviewed changes

giffels merged commit 068ad68 into MatterMiners:master Jun 3, 2020

giffels deleted the add-log-channels branch June 3, 2020 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `COBalD` log channels for logging in `TARDIS` #146

Use `COBalD` log channels for logging in `TARDIS` #146

giffels commented Apr 28, 2020 •

edited

Loading

eileen-kuehn commented May 14, 2020

eileen-kuehn left a comment

eileen-kuehn May 14, 2020

eileen-kuehn May 14, 2020

giffels commented May 15, 2020

maxfischer2781 commented May 15, 2020

giffels commented May 15, 2020

giffels commented May 22, 2020

maxfischer2781 left a comment

rcaspart left a comment

codecov-commenter commented May 25, 2020 •

edited

Loading

giffels commented May 25, 2020

rcaspart left a comment

maxfischer2781 left a comment

eileen-kuehn left a comment

		logger.info(f"Draining failed with: {str(cef)}")
		logger.info(

Use COBalD log channels for logging in TARDIS #146

Use COBalD log channels for logging in TARDIS #146

Conversation

giffels commented Apr 28, 2020 • edited Loading

eileen-kuehn commented May 14, 2020

eileen-kuehn left a comment

Choose a reason for hiding this comment

eileen-kuehn May 14, 2020

Choose a reason for hiding this comment

eileen-kuehn May 14, 2020

Choose a reason for hiding this comment

giffels commented May 15, 2020

maxfischer2781 commented May 15, 2020

giffels commented May 15, 2020

giffels commented May 22, 2020

maxfischer2781 left a comment

Choose a reason for hiding this comment

rcaspart left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 25, 2020 • edited Loading

Codecov Report

giffels commented May 25, 2020

rcaspart left a comment

Choose a reason for hiding this comment

maxfischer2781 left a comment

Choose a reason for hiding this comment

eileen-kuehn left a comment

Choose a reason for hiding this comment

Use `COBalD` log channels for logging in `TARDIS` #146

Use `COBalD` log channels for logging in `TARDIS` #146

giffels commented Apr 28, 2020 •

edited

Loading

codecov-commenter commented May 25, 2020 •

edited

Loading