Database_type config to Split up sql queries by engine type #7934
Conversation
|
@Trovalo , @danielnelson can you both review this please given a big PR - still may make a small change or 2 but took a stab at this. |
|
There may not be a conflict with #7842 , as that is one of the few queries I haven't changed |
| @@ -2,6 +2,8 @@ package sqlserver | |||
|
|
|||
There was a problem hiding this comment.
All the SQL Related queries have been moved in their own file, what about putting the "standard" edition SQL Queries in their own file too?
I think this will be even cleaner as we won't have queries in the main GO file
There was a problem hiding this comment.
Yeah I was thinking about doing that ran out of time, will do that in next commit, will split V2 in own file and clean up queries to just have SQLServer queries in own file .
There was a problem hiding this comment.
Split V2 in latest commit - Will do SQLServer as separate file later.
There was a problem hiding this comment.
- I've added a bunch of comments, mostly about documentation.
- I would just put even the v2 queries in their own file in order to have an even cleaner structure
- we could add a warning to prompt if they use the updated/deprecated parameters "azuredb" or "query_version", I saw this approach in the past
I'll soon run a full test of the plugin, but since nothing has been changed in the queries themselves everything should work fine.
About the possible Merge conflicts or order with other open PRs, I'll have a look at it later.
@danielnelson - thanks and best of luck for your new job
|
About open PRs which might need to be handled before or after this one, those are the one I've found The first 2 are mine, so I've no problems with merging them later, personally I think there won't be any problem even with the last one since the query is the same for all the versions |
plugins/inputs/sqlserver/README.md
Outdated
| @@ -82,7 +91,8 @@ GO | |||
| ## - AzureMIMemoryClerks | |||
| ## - AzureMIPerformanceCounters | |||
|
|
|||
| ## Version 2: | |||
| ## Version 2 by default collects the following queries | |||
| ## Version 1 is being deprecated, please consider using database_type. | |||
There was a problem hiding this comment.
just a small error
"Version 1 is being ..."→ "Version 2 is being ..."
plugins/inputs/sqlserver/README.md
Outdated
|
|
||
| ## Optional parameter, setting this to 2 will use a new version | ||
| ## of the collection queries that break compatibility with the original | ||
| ## dashboards. | ||
| ## of the collection queries that break compatibility with the original dashboards. | ||
| ## Version 2 - is compatible from SQL Server 2012 and later versions and also for SQL Azure DB |
There was a problem hiding this comment.
"from SQL Server 2012 and" → "from SQL Server 2008 SP3 and"
plugins/inputs/sqlserver/README.md
Outdated
| The `sqlserver` plugin provides metrics for your SQL Server instance. It | ||
| currently works with SQL Server 2008 SP3 and newer. Recorded metrics are | ||
| lightweight and use Dynamic Management Views supplied by SQL Server. | ||
|
|
||
| ### THe SQL Server plugin supports the following editions/versions of SQL Server |
There was a problem hiding this comment.
there is an extra capital H "THe" → "The"
The plugin as it stands today is increasing complexity of queries given different engine types Current Engine type ssupported Azure SQL DB, Managed instance, SQL Server versions 2008 - 2019 Config file now has a database_type which is SQLServer / AzureSQLDB / AzureSQLManagedInstance. This enables us to split up the queries so one change doesn't regress others and reduce testing surface. This will take only if users use database_type in config, if not it remains as before. Also split queries for V1 and queries for Azure SQL into separate files.
SQL Managed instance Database IO was broken with prior change Also all managed instance queries do not need DB_NAME in all measures, that was introduced for Azure Single DB This will make maintainability much easier and have to test only with the one plaform a query has changed.
Currently on-prem SQL Server queries have not been refactored They are effectively the same V2 queries but only the ones applicable to SQL Server (not Azure related DMVS) Ideally they should be refactored in another PR to simplify and get rid of cloud portions, separated out into own file.
Added few new queries to tests
Making readme changes per PR comments , added deprecation notice Also split out V2 queries into its own file.
Incorporated changes from - influxdata#7834
Incorporating changes from - influxdata#7808
denzilribeiro
force-pushed
the
sqlSplitAzure
branch
from
8bef67e to
9c9c085
Compare
|
Had to rebase, due to other commits, sorry about that. |
Fixing Variable
Split on-prem sql server queries for database_type="SQL Server" Updated readme, also minor naming changes for consistency of queries
Commit for queries for database_type = "SQLServer"
|
@ssoroka - any idea why the test was failing? --- FAIL: TestAlignedTickerJitter (0.01s) |
|
@denzilribeiro Sorry, was just a flakey test. |
|
@Trovalo, did you get a chance to run your tests on this branch? happy? |
|
I'm on vacation and I won't be able to run any test until this weekend. |
| , REPLACE(@@SERVERNAME,'\',':') AS [sql_instance] | ||
| , s.session_id | ||
| , ISNULL(r.request_id,0) as request_id | ||
| , DB_NAME(s.database_id) as session_db_name |
There was a problem hiding this comment.
For 2008 and 2008R2 compatibility get col from dm_exec_requests
, DB_NAME(r.database_id) as session_db_nameThere was a problem hiding this comment.
Will do, this was a change I picked up from 45a83b9 and can revert it.
| , s.program_name | ||
| , s.host_name | ||
| , s.nt_user_name | ||
| , s.open_transaction_count AS open_transaction |
There was a problem hiding this comment.
For 2008 and 2008R2 compatibility the same can be obtained with
,open_transaction =
CASE
WHEN r.session_id IS NOT NULL THEN r.open_transaction_count
ELSE (SELECT COUNT(*) FROM sys.dm_tran_session_transactions AS t WHERE t.session_id = s.session_id)
ENDThanks @spaghettidba
There was a problem hiding this comment.
This wasn't something I changed, haven't committed so far, can discuss under different PR. Having a count(*) from open transactions for every request isn't a cheap operation ( think of 5000 active requests). This results in a nested loop join executed for every request. SQL 2008/R2 is past extended support from a SQL Server perspective, at some point trying to get parity for everything new isn't going to be possible and will complicate existing queries.
There was a problem hiding this comment.
This has been changed in the 45a83b9, the priority has been given to the sessions table (s) before thew column was picked from the request table (r). (which to me still makes sense tbh, since the table is detailed "by request" and not by session.
Probably the best solution is to use COALESCE(r.open_transaction_count, s.open_transaction_count) AS [open_transaction_count], by doing so we get the data per-request, but if there is no request then it makes sense to fetch it from the session table.
We can revert it back to r.___ and get the data from the requests, or I can split the query in 2 versions using dynamic SQL in order to manage this better.
I'm aware of the fact that SQL 2008/R2 is past extended support, and I hope to don't see it around anymore.... but for now it's still around...
So I'd like to keep the full compatibility at least for the V2 of the queries, but I agree on the fact that a certain point the support for it should be stopped
There was a problem hiding this comment.
These are totally 2 different discussions
a. I get the Requests v/s Sessions argument for open_transactions, that is a trivial change and shouldn't be different or if ti is the session count should override anyways as tran is still open
b. what I was commenting on is the fact that open_transactions isn't there before 2012 and hence the select to get the count from dm_tran_session_transactions is more expensive.. Either way this wasn't a change
I am not sure where you are seeing it changed in 45a83b9 I just took a look a the line says
, s.open_transaction_count AS open_transaction
There was a problem hiding this comment.
In this case I would point to solution "a", so just get the column form requests instead of sessions (for compatibility reasons, otherwise the query explodes on SQL 2008).
Just abort the idea of the subquery on "dm_tran_session_transactions" (it was not a great idea), as you said it's heavier and it's not worth running it just to keep compatibility with SQL 2008 and R2
There was a problem hiding this comment.
I ended up changing to dynamic SQL to get appropriate column. Changing to r.open_transactions means that even for versions after 2008R2 you will not get the open tran count for an Idle session ( think of a session that has an open transaction and no request).
Reverting a change made in 45a83b9 that had incorporated in this commit. SQL 2008 / R2 doesn't have database_id in sys.dm_exec_sessions.
Except for the open point about the column |
sys.dm_exec_sessions does not have open_transactions column prior to SQL 2012 To accomodate 2008/R2 switched to dynamic SQL
Changes done with Dynamic SQL |
| , DB_NAME() as [database_name] | ||
| , s.session_id | ||
| , ISNULL(r.request_id,0) as request_id | ||
| , DB_NAME(r.database_id) as session_db_name | ||
| , DB_NAME(s.database_id) as session_db_name |
There was a problem hiding this comment.
s.database_id is not available in SQL 2008 and R2, it should be r.database_id
But since this is dynamic SQL now you can just pass it in the column parameter
IF @MajorMinorVersion >= 1200
SET @Columns = ',s.open_transaction_count as open_transaction
,DB_NAME(s.database_id) as session_db_name'
ELSE
SET @Columns = ',r.open_transaction_count as open_transaction
,DB_NAME(r.database_id) as session_db_name'after this the PR is approved to me
There was a problem hiding this comment.
Dang it sorry moving target should install a 2008/R2 instance at some point. should be done now.
Incorporated dynamic SQL for database_id
Required for all PRs:
This is a big PR that addresses the issues in #7713
For example I just realized that DatabaseIO was broken on Managed instances with the prior fix for on-prem stuff.
Fundamental principals
a. Split out queries by each database type - that simplifies queries, bug in a change will only affect a subset of input target platforms.
b. Maintain the same measures if the underlying core DMV is the same.
c. Reduces number of queries that are executed against each target or have ability to reduce them.
d. Ability to change the "default queries" per target.
Resolves #7713