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

Add the timezone in clock meter #1459

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eworm-de
Copy link
Contributor

I connect to servers with non-local timezones every now and then... Let's add the timezone in clock meter to minimize confusion there.

@fasterit
Copy link
Member

I would at least make this optional (config option) as (... how do I say that politely ...) it is not a sign of following best practices to run distributed servers with clocks in local time zones.
/DLange

@Explorer09
Copy link
Contributor

I would at least make this optional (config option) as (... how do I say that politely ...) it is not a sign of following best practices to run distributed servers with clocks in local time zones. /DLange

Nah. It's about the display of the system time in UTC or in local time. Note the colck meter uses localtime_r already and not gmtime_r. But without the UTC keyword it can cause the user confusion.

The switch between local time and UTC can be done as a separate feature.

@eworm-de
Copy link
Contributor Author

Whether or not the timezone being local on a server makes sense... Depends.

But that's the point: My workstation has local timezone CEST, and the server has UTC. Looking at the clock from server's htop shocked me for a little moment. 🙃

Will have a look to make it configurable.

@fasterit
Copy link
Member

I am confused how sysadmins would be confused about the TZ they set on their own user / their root user.
And why in ClockMeter.c but not in DateTimeMeter.c ?

@fasterit
Copy link
Member

But that's the point: My workstation has local timezone CEST, and the server has UTC. Looking at the clock from server's htop shocked me for a little moment. 🙃

Bring your TZ to the server. TZ="Europe/Berlin" htop works.
(so does fixing your ssh env 😄)

@Explorer09
Copy link
Contributor

grep -r localtime
ClockMeter.c:   const struct tm* lt = localtime_r(&host->realtime.tv_sec, &result);
DateMeter.c:   const struct tm* lt = localtime_r(&host->realtime.tv_sec, &result);
DateTimeMeter.c:   const struct tm* lt = localtime_r(&host->realtime.tv_sec, &result);
Process.c:   (void) localtime_r(&this->starttime_ctime, &date);

That's all the code that would be affected by time zones. And yes, if you show the timezone offset in ClockMeter, it should also show in DateTimeMeter.

@eworm-de
Copy link
Contributor Author

I am confused how sysadmins would be confused about the TZ they set on their own user / their root user.

Well, my confusion was not about the settings of the server, but more about what terminal window I was looking at. 😜

And why in ClockMeter.c but not in DateTimeMeter.c ?

Because that's what I use. 😁 Added it to date/time meter in another commit, but still without any configuration option.

Bring your TZ to the server. TZ="Europe/Berlin" htop works. (so does fixing your ssh env 😄)

I would have to add that in user configuration of any server... Using environment variables via ssh is a nice idea, but does not work out: This requires the server to accept these variables (see AcceptEnv and PermitUserEnvironment in sshd_config man page), and the default is to not do so.

@BenBE BenBE added needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature labels Apr 18, 2024
@BenBE
Copy link
Member

BenBE commented Apr 18, 2024

TBH, all except TZ=UTC is just overrated …

image

I connect to servers with non-local timezones every now and then...
Let's add the timezone in clock meter to minimize confusion there.
... just as done in clock meter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants