-
Notifications
You must be signed in to change notification settings - Fork 24
Converted GETUTCDATETIME() to SYSDATETIME() #245
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
Converted GETUTCDATETIME() to SYSDATETIME() #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the description of the PR with details of why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the PR description.
Integration tests running against SQL Server instances in different time zones will validate that recent slow queries and execution plans are now properly collected within the specified interval window. No additional tests are needed IMO.
If Integration tests are working now. Shouldn't they have failed earlier because we were using UTC time to retrieve recent executed queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTm
@@ -19,7 +19,7 @@ var Queries = []models.QueryDetailsDto{ | |||
sys.dm_exec_query_stats qs | |||
WHERE | |||
qs.execution_count > 0 | |||
AND qs.last_execution_time >= DATEADD(SECOND, -@IntervalSeconds, GETUTCDATE()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://learn.microsoft.com/en-us/sql/t-sql/functions/getutcdate-transact-sql?view=sql-server-ver17
I don't think this will work directly for all the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered this in the comment with
The absolute timezone doesn't affect the relative time calculations, and both SYSDATETIME() and last_execution_time always operate in the same timezone context
as per this doc, the time zone is always set to UTC so I am not sure if this fix will work for all the cases. Is it tested to be working for all the environment types? |
SQL Server (On-Host): SYSDATETIME() returns the system datetime based on the server's local timezone Azure SQL Managed Instance: SYSDATETIME() returns datetime in UTC timezone by default The sys.dm_exec_query_stats.last_execution_time column stores timestamps in the same timezone as the server. So for managed instances: The absolute timezone doesn't affect the relative time calculations, and both SYSDATETIME() and last_execution_time always operate in the same timezone context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR corrects a critical time zone issue where MSSQLTopSlowQueries and ExecutionPlanQuery were using GETUTCDATE() to filter against qs.last_execution_time, which is stored in server local time. The mismatch caused queries to return no recent data in environments where local time is ahead of UTC.
Replace GETUTCDATE() with SYSDATETIME() in both query time filters to align with the sys.dm_exec_query_stats DMV's local time storage
Integration tests running against SQL Server instances in different time zones will validate that recent slow queries and execution plans are now properly collected within the specified interval window. No additional tests are needed IMO.