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

feat: different monitor timezones from user timezone (#5201) #5212

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marcellodomenis
Copy link

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Backend changes:

  • Add a timezone column to the monitor table and ensure it's passed to the frontend.
  • The custom timezone is stored in the database to persist across multiple sessions.

Frontend changes:

  • A new Monitor Timezone setting is now available under the General section in each monitor setting.
  • By default, the monitor uses the timezone configured in settings/general.
  • If the timezone changes from the default value, the PingChart will reflect the custom timezone.

Fixes #5201

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Screen.Recording.2024-10-16.at.19.51.06.mov

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I think I forgot to tell more specifically than @GogoFC how I want changes done in this case.
Sorry about that

To know what time server went down and up in the timezone where the servers is located at

Adding more options to the monitor (especially if they are as specific as this one) are not something I want. The UI is already way too complicated and specific for this.
Implementing having 3 timezones for monitors is also too complicated/error prawn. Having two is likely pushing it already..

What I would agree with is how we display timezones that might have a smaller, non bold text below it like (server: 2024-.. 8:43 CEST) when hovering over it in the UI.

@louislam
Copy link
Owner

louislam commented Oct 17, 2024

I also think timezone by monitor is too complicated and it might be confusing over time.

Also by a lot of modern application designs, they are using single timezone. To be honest, currently there are two timezones in the settings page, that is too complicated already.

@CommanderStorm CommanderStorm added area:monitor Everything related to monitors area:settings Related to Settings page and application configration type:enhance-existing feature wants to enhance existing monitor pr:please address review comments this PR needs a bit more work to be mergable labels Oct 17, 2024
@CommanderStorm CommanderStorm marked this pull request as draft October 17, 2024 14:19
@GogoFC
Copy link

GogoFC commented Oct 17, 2024

I also think timezone by monitor is too complicated and it might be confusing over time.

Also by a lot of modern application designs, they are using single timezone. To be honest, currently there are two timezones in the settings page, that is too complicated already.

From a dev point of view I can't comment on complications of timezones, that's been complicated and overcomplicated from the start.

From a user point of view. If I go to check a log in a server in California that went down 3 days ago at 5:33 AM I have to convert that to minus 9 hours which is... I literally have to look at a non digital clock now. So that's 8:33 PM the day before and now I can check the logs.

Same way if I see a UI notification saying server is down 5 hours ago, well that just means 14 hours ago. In Sydney if a server went down 5 hours ago that just means it went down 14 hours into the future which didn't happen yet in EU. So if the server in Sydney went down 5 hours ago that would mean it went down 8 PM which is 11AM here, but it doesn't really matter what time it was in EU when that happened.

I personally am not a fan of 'server went down 5 hours ago' anyway since then I have to do the math. I don't do the math because I can click and see the time stamp and that's what I find useful.

So a simple offset would do the trick if there wasn't Daylight Saving Time.

When I'm logged in to a remote server I like to look at the logs at the localtime and I think in terms of local time of where the server is at and not convert the time. Similarly if a European moves to US he shouldn't be converting meters to feet all the time, he really should think in miles. And when here think in Celsius etc.

Also I don't undertstand the purpose of two timezones in the UI. I don't know what the 'server' timezone is. I couldn't get that to change the way UI displays time. That might be something to do with HA if there are multiple servers?

What I'm talking about is not having three time zones for the UI. I'm talking about UI having one timezone and each Monitor having either the default timezone which is UI or having it's own timezone.

I like this pull request because it shows the timestamp of the Monitor and that's useful. What is less useful, but still useful, is showing the UI timezone in Monitors.

The attached video is actually clean. It shows the correct time of each Monitor and not the incorrect time.

@marcellodomenis
Copy link
Author

marcellodomenis commented Oct 17, 2024

If I can add some inputs:

During implementation, I considered scenarios like international businesses serving customers across different regions (e.g., Amazon). If you're serving customers in both Germany and the U.S., knowing when a service disruption occurs in their local time is important. A disruption at 4 a.m. would likely be less critical than one at 10 a.m. Additionally, for debugging, knowing a service went down at 10 a.m. local time could help identify issues like peak traffic, whereas using only one timezone across the board might lose such insights.

The proposed solution wouldn’t add much complexity to the UI, as this feature would be 'hidden' within the settings of each monitor. It could even be further moved under the Advanced section for even more simplicity. By default, users wouldn’t need to do anything extra, since new monitors would adhere to the User Timezone set in Settings -> General. However, it would provide the flexibility for users who manage services across multiple regions to customize it.

While working on the feature, I didn’t fully grasp the purpose of the Server Timezone setting, as changing the value didn’t seem to affect the UI. I assume Server Timezone is relevant in scenarios like hosting an Uptime Kuma instance on a server in Frankfurt, but using it from New York. In such cases, differentiating between time zones could be useful for logging or debugging, particularly if the instance experiences downtime.

I also agree with @GogoFC that the best approach would be to have one single default timezone while allowing each monitor to override it with a custom one aligned with the regions they serve.

@CommanderStorm
Copy link
Collaborator

You both ignored the concerns that I and Louis had.

  • having 3 sources for the timezone is quite an error prown solution and likely very confusing for users. 2 is likely already a bit too confusing.
    Given our test coverage, that is just trouble waiting to happen.

  • wouldn’t add much complexity to the UI, as this feature would be 'hidden' within the settings of each monitor

    See my previous comment: I think the settings UI for monitors is already too overwhelming.

The real solution is to either not have cross timezones (monitor them in UTC, manually convert when that is important) or run an instance per timezone.

The time of the day in local time when something went down likely should not play that much of a role in your workflow. If you are global, the oncall team will be on call for every incident, no matter the time of day where the incident is happening.
If some time of the day does not matter to you, consider using maintenances to block it at these times.

@marcellodomenis
Copy link
Author

having 3 sources for the timezone is quite an error prown solution and likely very confusing for users. 2 is likely already a bit too confusing.
Given our test coverage, that is just trouble waiting to happen.

Okay, understandable.

Also upon checking similar monitoring tools, none have a custom monitor timezone feature.

What I would agree with is how we display timezones that might have a smaller, non bold text below it like (server: 2024-.. 8:43 CEST) when hovering over it in the UI.

Do you mean when hovering over single points datapoints in the PingChart? So you would have a tooltip with:

2024-10-17 12:45:32
server: 2024-.. 8:43 CEST
211 ms

with server timezone based on the value in Settings -> General -> Server Timezone.
and when timezones are the same, the tooltip does not display the server row.

Are there any other parts in the UI that should reflect this change?

If it's still worth working on, I can make changes, otherwise, I'll close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors area:settings Related to Settings page and application configration pr:please address review comments this PR needs a bit more work to be mergable type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify timezones between UI and server
4 participants