opserver / Opserver

Stack Exchange's Monitoring System
https://opserver.github.io/Opserver/
MIT License
4.51k stars 827 forks source link

Convert SQL Dates to UTC #225

Closed ChristopherHaws closed 7 years ago

ChristopherHaws commented 7 years ago

Hello,

Most of our servers are not in UTC which causes all of the dates and times to be incorrect throughout the app. I believe that I have patched all of the places where DateTimes are read from SQL.

SQL monitors that contained DateTimes that were patched:

The SQL Instance Property class now includes TimeZone which falls back to UTC if it cannot read the TimeZone from the SQL Server. This property is then used by all of the above monitors to convert the DateTime's to UTC when they are being read, so the times are all stored in UTC in the caches.

Unsolvable Issue:

I also hope you don't mind, I added a .editorconfig file. In most of my projects as well as my companies projects we use tabs instead of spaces, so I use the EditorConfig extention when working on open source projects to make sure that I stick to their standards. If you don't already know, EditorConfig is a file, that many IDE's have plugins for, that allows you to share settings such as spaces/tabs, line feed character, etc with other users.

PS: Thanks for this awesome product! :)

NickCraver commented 7 years ago

Thanks for the PR! Unfortunately, this is far from a simple issue and has come up before...I just haven't gotten to it in far too long (see #42). In this case, there are 2 major issues I see:

Some example approaches (which would need integrating directly into existing queries):

I just need to fire up an instance that's not on UTC time to test these a bit and have never found the time to prioritize it :(

ChristopherHaws commented 7 years ago

Thanks for the quick response!

I completely hear you on using an undocumented stored procedure, and I didn't want to use it. As far as I could see, there aren't too many good alternatives. As for the permission issue, I did take that into account. If you don't have permission to that stored procedure or it doesn't exist, the code will fall back and use UTC.

Here are a few approaches that I thought of, but ultimately decided not to go with:

What are your thoughts on adding a setting in the SQLSettings.json file instead of using ServerProperties until we can come up with a better approach? This approach would get rid of the async issue you pointed out as well as the issue of using an undocumented stored procedure.

The settings json file would look something like this, and if the timezone is not specified then the app can assume that the server is in UTC.

{
    "defaultConnectionString": "Data Source=$ServerName$;Initial Catalog=master;Integrated Security=SSPI;",
    "instances": [
        {
            "name": "SQLSERVERNAME",
            "timeZone": "Pacific Standard Time"
        }
    ]
}

I am more that happy to go in this direction instead. It wouldn't be that big of a change from what I have already.

Let me know what you think. :)

Thanks!

NickCraver commented 7 years ago

Closing this out for the reasons above - but I have some ideas about how to fix this, see #250 for tracking. TL;DR: I think we can get the server timezone offset once (cached) and apply this on the query side globally in Opserver in some nice ways. I appreciate the work here, I really do - just need to approach it in a more scalable/simple way.

danilocbraga commented 7 years ago

Probably it's not best approach, but the only thing that I did to fix it was changing all "DateTime.UtcNow" to "DateTime.Now" in the project. Everything that was not matching before works fine now. Thx guys!