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

Follow-up to #939: TZ best practices for DBs #972

Closed
alexdelprete opened this issue Dec 8, 2023 · 9 comments
Closed

Follow-up to #939: TZ best practices for DBs #972

alexdelprete opened this issue Dec 8, 2023 · 9 comments
Assignees
Labels
❓ question Further information is requested

Comments

@alexdelprete
Copy link
Contributor

alexdelprete commented Dec 8, 2023

@alexjustesen sorry Alex, but I had to open another issue to follow-up your post in #939 after you locked the discussion.

You wrote:

TZ environment variable SHOULD NOT be passed to the database container or the application container.

and

will be the database should run in UTC and not your local time zone. Broadly speaking we don't change the time zone of the database in any of the other applications, websites and tools I've developed so I'm not deviating from that standard here either.

This is NOT the best practice for DBs, you can ask any professional DB/System administrator. What you usually do in your professional projects is simply wrong (you can read any DB manual) and you should adhere to industry best practices instead of forcing users to adopt BAD practices. The fact that this bug created so many issues and that you will keep leaving things as they are while knowing what the problem is really amazes me.

I highlighted what the bug was and how to solve it (easy fix), but that post and the fact you didn't want to even discuss your decision really deludes me.

Many users, including me, rely on a central DB that is configured perfectly (with TZ properly configured) and does not create ANY issue to other apps, because they follow best practices, unlike ST.

So if you don't revisit your decision I'm sorry to say I will move from this project, even though I was here since the beginning.

Please let me know so I can stick the nail on ST's coffin.

Thanks.

@hoorna
Copy link

hoorna commented Dec 8, 2023

Thanks @alexdelprete for opening this issue to follow up #939. I was also planning to do so but you beat me to it.

@alexjustesen, it supprised me that you closed issue #939 without offering any further solution. In one of your last posts in #939 you give a perfect analysis of the cause of the timezone issue (namely that in some situations the offset is calculated twice). It took you a long time to figure this out. But now that you have finally discovered the cause, you leave the solution behind while it is so close. You were even given several solutions, one including the necessary code. What else do you want?

My advice would be to keep going, try out one or more solutions and then choose the best one. You have already put so much time into it, so finish it now that the solution is in sight.

As I said before, if you need help, let us know.

@alexdelprete
Copy link
Contributor Author

BTW: if you need to write UTC timestamps, don't use TIMESTAMPS, but DATETIME records, so no conversion will be done by the DB.

image

Very simple and basic stuff for a DB client app.

Repository owner locked and limited conversation to collaborators Dec 8, 2023
@alexjustesen
Copy link
Owner

alexjustesen commented Dec 8, 2023

Locking for now, will revisit at a later date.

Additional research notes

Intercept db connection timezone

Potentially can intercept the timezone established as part of the connection and inject the timezone set in the config:


1st edit: adding references to MySQL and Postgres timezone in connection

@alexjustesen alexjustesen added the ❓ question Further information is requested label Dec 10, 2023
@alexjustesen alexjustesen self-assigned this Dec 10, 2023
Repository owner unlocked this conversation Dec 13, 2023
@alexjustesen alexjustesen added this to the v0.14.0 (timezones) milestone Dec 13, 2023
@alexjustesen
Copy link
Owner

@alexdelprete @hoorna v0.14.0-beta8 is currently building and will be available shortly. This introduces a new db_has_timezone setting which is detailed in #984 along with test cases.

This also represents the last of my effort on this topic for a long time, please let me know if this solution works for you so the outcome can be documented.

Please let me know so I can stick the nail on ST's coffin.

Use it or don't man, not here to make money or provide coverage for everyone's nuanced home lab. You won't offend me either way.

@alexjustesen
Copy link
Owner

Also docs have been updated to reflect this new setting, the recommendation remains a database should store and result timestamps in UTC time for the application to transform at runtime.

https://docs.speedtest-tracker.dev/faqs#my-display-timestamps-or-scheduled-tests-arent-correct.

@hoorna
Copy link

hoorna commented Dec 13, 2023

@alexjustesen, I installed v0.14.0-beta8 with the container TZ environment variabele removed. On the General page I defined my local time zone under "Time Zone Settings" and also enabled "Database has time zone". My database container has the local timezone set with the container TZ environment variable.

All is working now! The results page displays the correct times and the cron time set in "Speedtest schedule" is working correct.

You have opted for a somewhat idiosyncratic solution, but it works perfectly. Thanks for that.

Also docs have been updated to reflect this new setting, the recommendation remains a database should store and result timestamps in UTC time for the application to transform at runtime.

https://docs.speedtest-tracker.dev/faqs#my-display-timestamps-or-scheduled-tests-arent-correct.

There is one small issue that I would like to report to you. The help description about the new "Database has time zone" setting is not entirely clear. It may be due to my limited English language skills, but I don't understand the sentence there.

@alexdelprete
Copy link
Contributor Author

alexdelprete commented Dec 13, 2023

Use it or don't man, not here to make money or provide coverage for everyone's nuanced home lab. You won't offend me either way.

Homelabs database containers are configured with proper timezone settings, because that's the way they need to be configured. Actually it's you forcing ALL users to adhere to the app's bad practice, that's the only nuance here.

I don't offend anybody, I simply report the documented facts, and the unfortunate reality is pretty clear: ST is forcing the DB to use a wrong configuration just because you don't want to adhere to best practices.

I documented for you several options to fix it properly:

  1. set user configured TZ in the connection function (as explained in the other issue)
  2. don't use Timestamps record type but Datetime, so no TZ conversion is done on the db side

All the other solutions are logically absurd workarounds. For example, your workaround is a flag asking users if the DB is properly configured with a TZ: but you don't ask what that TZ is, because you assume it is the same: but that is not the case because I can have a DB in the cloud in a different TZ.

Another flawed approach, when you have proper solutions available.

Cheers.

@alexjustesen
Copy link
Owner

Also docs have been updated to reflect this new setting, the recommendation remains a database should store and result timestamps in UTC time for the application to transform at runtime.

https://docs.speedtest-tracker.dev/faqs#my-display-timestamps-or-scheduled-tests-arent-correct.

There is one small issue that I would like to report to you. The help description about the new "Database has time zone" setting is not entirely clear. It may be due to my limited English language skills, but I don't understand the sentence there.

You're not wrong, it needs to be written better and I've got an overhaul of the documentation planned for the Christmas break.

@alexjustesen
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❓ question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants