Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No reason to issue a warning in the log for any clock-related shifts. #2038

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

moreaki
Copy link

@moreaki moreaki commented Feb 7, 2023

For years, people's logs have been filled up with warnings about thread starvation and retrograde clock change detections. This change sets the logger level to info for the following reasons:

  • There is really nothing the end user can do with this warning, the code solves the issue by itself.
  • It creates many questions and searches for solutions on the Internet because people see a WARN, where clearly an INFO would be sufficient
  • None of the code paths which follows (hot or cold) has any WARN statements, therefore the granularity of the logger is misleading

@thiagosperandio
Copy link

thiagosperandio commented May 16, 2023

That happened to me too.

I agree that it should be a WARN, because we always need to be aware of problems that may occur in a production environment.

So, it's a warning about hibernation, either of the machine or our service thread.

Maybe can increasing some additional information, like "checks if the machine was suspended".

@moreaki
Copy link
Author

moreaki commented May 22, 2023

That happened to me too.

I agree that it should be a WARN, because we always need to be aware of problems that may occur in a production environment.

So, it's a warning about hibernation, either of the machine or our service thread.

Maybe can increasing some additional information, like "checks if the machine was suspended".

Confused by your reply. With whom do you agree? Certainly not with me (which is fine, but your explanation makes little sense). I clearly made a patch to set the level to INFO from WARN, so we don't get pointless information. Did you read my explanation? There is nothing that you're being warned for which you can influence at all and all cold and warm code paths following that statement are on level INFO.

In short: a WARN level statement in the logs at this point in the code, as argued in my original statement, makes absolutely NO sense.

Let me know if I misunderstood something from your reply. Also, I would be extremely happy to hear from you on how you intend to address the issue outside of the already existing code which amends the reported issue further down in the code path. Any insights that a human intervention can do to alleviate this wrongly reported issue as a WARN would be most welcome, and probably close dozens of stackoverflow questions with people equally wondering why their logs fill up with statements of a potential issue over which they don't seem to have an influence.

@benedictdube
Copy link

I clearly made a patch to set the level to INFO from WARN, so we don't get pointless information. Did you read my explanation? There is nothing that you're being warned for which you can influence at all and all cold and warm code paths following that statement are on level INFO.

In short: a WARN level statement in the logs at this point in the code, as argued in my original statement, makes absolutely NO sense.

I'm in the early stages of my career in software eng. and would like to confirm, is it not appropriate to log at WARN level when there is something in the program that might (or can) lead to an unwanted state?
For example; If I encounter this frequently, it could guide my decisions in the scheduled jobs I have on my droplet/EC2 instance if I encounter this frequently? Saving me from waiting for a higher logging level which might cause unavailability for the service. This, I doubt I would pick up if it were logged at INFO level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants