Porting PR#563 into 1.7 (adding queue logging to de_logger in 1.7 base branch)

HEPCloud / decisionengine

HEPCloud Decision Engine framework

Apache License 2.0

6 stars 26 forks source link

Porting PR#563 into 1.7 (adding queue logging to de_logger in 1.7 base branch) #572

Closed goodenou closed 2 years ago

goodenou commented 2 years ago

This PR consist of two commits, one that cherry-picks the code from PR #563 and one that cherry-picks the code from PR #515 .

PR#563: Steve was seeing race conditions in the decision engine structured log file when running multiple channels. This is because the standard logging library is not thread-safe. When multiple channels are running and logging data is being pushed through 'quickly', the channels compete with each other to have their logs written to the single decisionengine_structlog file. I have added a QueueHandler and QueueListener to handle the logging into the structured log file from the channel processes and the main process in a thread-made manner.

PR#515: Kyle added simplifications to some of the logger definitions in the modules, based on the fact that the modules inherit from the Module class, which defines the specifics of the logger.

pep8speaks commented 2 years ago

Hello @goodenou! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file src/decisionengine/framework/modules/tests/test_QueueLogger.py:

Line 1:1: F401 'gc' imported but unused

Comment last updated at 2021-12-09 21:40:17 UTC

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 4ebd527ac9cc751f03c8338f622646fc5421f6bb into a1453693235b829cf8e8e0c02b3b12773d263b4d - view on LGTM.com

new alerts:

1 for Module is imported more than once

mambelli commented 2 years ago

Hi Lisa, I'm merging this. The changes seem also to include some changes in the logging handlers. If there is anything beyond PR#563 and PR#563 add it to the description. Then keep an eye on the CI tests on 1.7 after I merge this. Somehow GH ran only LGTM on this PR, not unit tests, ... probably because it was created before the branch changes.

goodenou commented 2 years ago

PRs #515 and #563 are the only two PRs going into this one. The changes in the handlers that you noticed are changes in how the configuration is written and also the addition of the QueueHandler, which are both from #563. I checked the CI tests and the FLAKE8 test is failing due to an unnecessary 'import gc' in src/decisionengine/framework/modules/tests/test_QueueLogger.py.