DM-54429: Update logging and unit tests#18
Conversation
Reduce logging to debug and add total messages stored log line
ebellm
left a comment
ebellm
left a comment
There was a problem hiding this comment.
Some questions about the daily reset behavior, maybe I'm missing something.
| state_tracker["daily_stored"], | ||
| ) | ||
| state_tracker["daily_stored"] = 0 | ||
| state_tracker["day_start_time"] += one_day |
There was a problem hiding this comment.
this is going to get weird with things like leap seconds. Is there a reason not to use datetime or similar here?
There was a problem hiding this comment.
No, you are right datetime is the way to go.
| """ | ||
| one_day = 86400 | ||
| while current_time - state_tracker["day_start_time"] >= one_day: | ||
| logger.info( |
There was a problem hiding this comment.
don't you need to reset prefix_counts in here somewhere?
| idle_seconds = current_time - prefix_last_write[prefix] | ||
| if idle_seconds >= self.prefix_idle_timeout: | ||
| logger.info( | ||
| "Alert prefix %s: %d alert(s) stored to S3 " |
There was a problem hiding this comment.
Because I don't see the logic that resets prefix_counts daily, it seems like this will emit more and more logs over time?
There was a problem hiding this comment.
So you are right and prefix_counts don't reset daily, as they are meant to be a per prefix measure anyways since each prefix should only exist for a day. I do want to keep the logs over a certain period of time, but you are right and the current behavior means they will exist forever and the logging already occurs in logged_prefixes. I've changed it now so prefix_countsandprefix_last_writeis removed oncelogged_prefixes` happens.
bsmartradio
force-pushed
the
tickets/DM-54429
branch
from
6b1a454 to
a78ce3c
Compare
2 participants
No description provided.