fix: use error only notifier logging for frequent jobs [DHIS2-17998] #19949
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
| NotificationLevel level = | ||
| job.getJobType().isUsingNotifications() | ||
| job.getJobType().isUsingNotifications() && !nonVerboseLogging |
There was a problem hiding this comment.
Double negative here. Could !nonVerboseLogging just be verboseLogging? (If I'm understanding this correctly)
There was a problem hiding this comment.
I thought about that too but I didn't want to inverse the logic in the function and I did not find another why to phrase it so in the end I thought the double negation was the lesser problem.
netroms
approved these changes
Feb 17, 2025
david-mackessy
approved these changes
Feb 17, 2025
jbee
added a commit
that referenced
this pull request
Feb 17, 2025
…19949) * fix: use error only notifier logging for frequent jobs [DHIS2-17998] * fix: blank first and last notifier message if INFO is not logged [DHIS2-17998]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I noticed that with the changes made to the notifier where all jobs do forward at least the first and the last message (procress start/end) to the notifier jobs that do run frequently (like the housekeeping job) basically spam the log with 1 line per run through the notifier logging.
To prevent it the notifier log level is set to ERROR if a job should use non-verbose logging. This means for the first and last message where the notification is still always send the message is blanked which also prevents logging.
Automatic Testing
There is no good way of testing it and I don't think it is worth making a complicated setup to verify.
Manual Testing
DefaultNotifieronce every 30 sec