Refactoring reset_logger to reset every 12 hours #1666
Conversation
Codecov Report
@@ Coverage Diff @@
## release-2.2.43 #1666 +/- ##
=================================================
Coverage ? 66.61%
=================================================
Files ? 80
Lines ? 11207
Branches ? 1573
=================================================
Hits ? 7465
Misses ? 3412
Partials ? 330
Continue to review full report at Codecov.
|
|
Important: Currently, the hash map is shared across levels and won't generate a new entry into the hashmap for the same messages with different log levels. This could suppress a newer log entry with a higher-level coming in later. |
azurelinuxagent/ga/monitor.py
Outdated
|
|
||
| if time_now >= (self.last_reset_loggers_time + | ||
| MonitorHandler.RESET_LOGGERS_PERIOD): | ||
| logger.reset_periodic() |
There was a problem hiding this comment.
I know this is the pattern we use today for all these periodic actions; we should consider if instead we should follow this:
try:
logger.reset_periodic()
finally:
self.last_reset_loggers_time = time_now
so that if there is an exception we don't keep calling the reset code continuously (to do after release)
| # Resetting the logger time states. | ||
| monitor_handler = get_monitor_handler() | ||
| monitor_handler.last_reset_loggers_time = datetime.datetime.utcnow() - timedelta(hours=1) | ||
| MonitorHandler.RESET_LOGGERS_PERIOD = timedelta(milliseconds=100) |
There was a problem hiding this comment.
+1, and this part of the test should be done directly on the logger, rather than on the logger via the monitor (plus move test to the logger tests)
There was a problem hiding this comment.
(could be done separately, in the interest of moving forward with the release)
There was a problem hiding this comment.
@pgombar
It is needed. We are not using the RESET_LOGGERS_PERIOD for any sleep, but for the calculation to reset the logger. I am just moving the last time sent to 1 hour, but also need to force the calculation of time_now >= now(-1hr) + 100ms to be true, to hit that code.
if time_now >= (self.last_reset_loggers_time + MonitorHandler.RESET_LOGGERS_PERIOD):
logger.reset_periodic()
@narrieta: This part of the test is testing the monitor pieces of the code and not the logger pieces of the code. We already test the periodic_* in the test_loggers (eg: test_periodic_does_not_emit_if_previously_sent). Yes, there needs to be a test specifically for only logger.reset_periodic() in the test_logger, and that can be added later (will send a new PR later for that, against develop).
There was a problem hiding this comment.
yup, my bad... good as it is now. thanks!
Description
Refactoring the reset loggers to bring it to the MonitorHandler level. The logger now resets every 12 hours.
Issue #
PR information
Quality of Code and Contribution Guidelines
This change is