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

Log datetime #2191

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

geneticdrift
Copy link
Contributor

No description provided.

@@ -129,7 +129,7 @@ handle_status(Client &client, [[maybe_unused]] Request args, Response &r)
COMMAND_STATUS_PLAYLIST ": {}\n"
COMMAND_STATUS_PLAYLIST_LENGTH ": {}\n"
COMMAND_STATUS_MIXRAMPDB ": {}\n"
COMMAND_STATUS_STATE ": {}\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which commit introduced this bug? This should be mentioned in the commit message so we know whether a backport is necessary.

@MaxKellermann
Copy link
Member

Your commit message doesn't contain an explanation for why you believe this is necessary.
I don't like thread_local. Your commit message doesn't explain why this is necessary.

Time was only in minutes before.
Seconds is more useful in analyzing the log for example
with issues of timeouts, and reponse times.
When running with stdout output to debug the server
or misbehaving clients, it is useful to have the timestamp
to detect timing issues and response times.
Especially when opening and playing online sources that
block the connection thread sometime for a significantly
long time that makes the client-server unresponsive
and cause timeouts in clients.
…in log_date()

This function may be called by multiple threads concurrently.
But it uses a static buffer for the datetime string which
can be written into simultanously by multiple thread.
If it is not thread local, for it to corrent it must be
assumed that the implementation writing to the buffer
never conflicts with itself in a way that may result
in a bogus timestamp output.
Having milliseconds resolution in the log gives more
information than just seconds when debugging or analyzing
response time issues or effects of changes in server or
client code.
@geneticdrift
Copy link
Contributor Author

Thanks for the review.

I've added more explanations in the commit messages as well as the commit number.

The logging timestamp format can be made optional by configuration if you prefer.
Personally I find the higher resolution timestamp more helpful in catching issues even when not looking for one specifically. And in cases of user postmortem logs when it's hard to reproduce an issue, so enabling higher resolution after the fact is not always helpful.

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.

2 participants