Sql server old version compatibility #7495

Trovalo · 2020-05-11T15:00:08Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

This PR adds compatibility for all the queries from SQL Server 2008 SP4 and later version, I made a very similar PR some weeks ago, the difference is that this one contains only the minimum to obtain compatibility.

Main changes:

The switch logic used to compose the queries is now based on variables when appropriate (MajorVersion, MinorVersion, EngineEdition) instead of being fetched every time, this also solved some existing (but not reported) issues, see bottom of the PR
Several CASE statement have been added to manage SQL 2008 and 2008 R2

It has been tested on:

SQL 2008 SP4
SQL 2008 R2
SQL 2012
SQL 2014
SQL 2016 SP2
SQL 2017
SQL 2019
Azure SQL DB

Additional Fix:
In the current version of telegraf, in the "performance counters" query, the column [total_cpu_usage_preemptive_ms] is never considered, it is added as follows:

CASE WHEN SERVERPROPERTY('ProductMajorVersion') > 10 THEN 'rgwg.total_cpu_usage_preemptive_ms

-- Sadly that switch does not work unless you also cast SERVERPROPERTY to int, run this to check
SELECT IIF( SERVERPROPERTY('ProductMajorVersion') > 10 ,'ok','not ok')

Now that the switches are based on variables (with proper data type) the metric will be loaded for SQL Server 2016 and later, in previous versions it does not exists

ssoroka

Wow. Excellent effort here; thank you.

I do wonder if this plugin is getting a little too complicated for its own good. Open to any thoughts you might have here.
~~What versions have you tested against?~~ [edit: never mind, I see you specified that; thanks!].
Any thoughts on how we can keep future updates from breaking backwards compatibility?
A downside of these giant SQL scripts is we probably won't find out until it doesn't work in production for someone.

Trovalo · 2020-05-14T14:30:20Z

TLDR:

complexity - the queries can be improved and standardized to be more readable and easier to maintain, but won't be less complex
avoid compatibility breaks - who writes the code must check from which version the referenced object is available and possibly test its query on previous and later versions of SQL Server

About complexity
I'm sure that the code can be improved to be more readable and easier to maintain, I'm reading Erland Sommarskog The Curse and Blessings of Dynamic SQL to achieve that. Defining a sort of standard about how to write SQL code for the plugin should help, especially for the dynamic SQL parts.
I will update this PR or make a new one when I come up with something better (and I will)

In any case, I doubt that the queries will be less complex, in my opinion, the dynamic SQL is the best option to manage additional/different columns/objects by checking edition and version of SQL Server.

About avoiding compatibility breaks
there are few things to take into consideration

Before adding/editing something in a query is to have a look at msdocs or other for info about the column/view, in order to add it only in the appropriate edition and version. (easy for recent version, not always easy for old versions)
Test the query on previous and later version of SQL server to ensure it works correctly (SQL server express is free for any version, since SQL 2014 you can use the developer edition for free)

other things not strictly related to telegraf are:

go-mssqldb (the package used to connect) - it supports the connection to SQL 2008 SP3 and SQL 2008R2 SP2 and later versions. source.
There are lots of SQL 2008/2008R2 still around (sadly), therefore I doubt they will remove compatibility for those versions (I see no reason to do so anyway)
SQL Server itself - Sometimes a CU adds a new useful view, but as of now, (almost) all the views used in the plugin have been there since SQL 2008 and have not been marked as "deprecated". Even if something will be marked as "deprecated" there will be plenty of time to develop a workaround, in fact the deprecated object is not usually removed before at least 2 versions of SQL server (if it is removed at all, in SQL 2019 you can still find objects deprecated since SQL 2005)

danielnelson · 2020-05-14T20:44:45Z

@denzilribeiro Can you take a look when you have time?

denzilribeiro

Note I haven't tested these against all versions, semantics have by and large not changed, these ae more standardization. There is no need to change sqlServerPropertiesV2 to dynamic SQL it does not achieve a whole lot I would not make this change - what we should do for this is split into on-prem v/s cloud.

Formulating the approach you have for dynamic SQL is fine IF you need dynamic SQL, I wouldn't convert those queries that don't need dynamic SQL to dynamic SQL.

Maybe a bit of over complication in variable section don't need a derived table - this is in multiple places - example below - you could even have that assigned as part of declare statement itself.

SELECT
@EngineEdition = CAST(SERVERPROPERTY('EngineEdition') AS int)
,@majorversion = CAST(PARSENAME(CAST(SERVERPROPERTY('ProductVersion') as nvarchar),4) AS int)

Secondly not sure of the reason for this change

Session_db_name comes form the session DMV not the request DMV?
DB_NAME(r.database_id) as session_db_name

denzilribeiro · 2020-05-14T21:38:08Z

My thought is you can support older versions forever and you pay for them in some way :) both with maintaining and a huge text matrix. End of life support for 2008/R2 has been reached last year I don't even have any 2008 lying around to test.

I don't agree with changing everything to dynamic SQL - debuggability of dynamic SQL is painful as well - makes sense when you are already using dynamic SQL to adhere to a standard.
Ideally each connection string first detects it version once and then picks a relevant set of simplified not over complicated scripts - would make it easier to maintain. Right now single set of scripts actually. I will start some discussion around feature request next week over long weekend to start splitting some of this to make it more manageable, this has grown quick and # versions increased drastically :) and not yet added elastic pool support .

Trovalo · 2020-05-15T15:32:17Z

Note I haven't tested these against all versions, semantics have by and large not changed, these ae more standardization. There is no need to change sqlServerPropertiesV2 to dynamic SQL it does not achieve a whole lot I would not make this change - what we should do for this is split into on-prem v/s cloud.

Formulating the approach you have for dynamic SQL is fine IF you need dynamic SQL, I wouldn't convert those queries that don't need dynamic SQL to dynamic SQL.

Maybe a bit of over complication in variable section don't need a derived table - this is in multiple places - example below - you could even have that assigned as part of declare statement itself.

SELECT
@EngineEdition = CAST(SERVERPROPERTY('EngineEdition') AS int)
,@majorversion = CAST(PARSENAME(CAST(SERVERPROPERTY('ProductVersion') as nvarchar),4) AS int)

Secondly not sure of the reason for this change

Session_db_name comes form the session DMV not the request DMV?

DB_NAME(r.database_id) as session_db_name

You are right about the dynamic SQL for sqlServerPropertiesV2, it's not needed in every query but only in the on-prem editions one. (the column virtual_machine_type_desc is missing in SQL 2008)
You are also right about the variable's value assignment, I can set it in the declare statement
about the database_id column, that's for compatibility, in fact, the field is available in sys.dm_exec_sessions since SQL 2012, but is available in sys.dm_exec_requests for any version (and since it was already in the query I took it from there)

Trovalo · 2020-05-15T16:07:56Z

My thought is you can support older versions forever and you pay for them in some way :) both with maintaining and a huge text matrix. End of life support for 2008/R2 has been reached last year I don't even have any 2008 lying around to test.

I don't agree with changing everything to dynamic SQL - debuggability of dynamic SQL is painful as well - makes sense when you are already using dynamic SQL to adhere to a standard.
Ideally each connection string first detects it version once and then picks a relevant set of simplified not over complicated scripts - would make it easier to maintain. Right now single set of scripts actually. I will start some discussion around feature request next week over long weekend to start splitting some of this to make it more manageable, this has grown quick and # versions increased drastically :) and not yet added elastic pool support .

About supporting older versions (even out of extended support), I think it's worth it since those versions are still around (I have an instance of SQL 2000...) and as a dba being able to monitor them is really helpful. There are already several switches to manage different versions and adding at worst two more (for SQL 2008 and 2008R2) won't make a difference

Personally, if I have to choose between maintaining a set of scripts (in the worst case one for each version) and a more complex dynamic SQL script I would choose the latest.

Currently, my idea to make it more readable is to create the "standard" script with some placeholders/variables (ie: for the columns), at the beginning the placeholder variable will be populated with the columns appropriate for the version and then injected in the SQL string, this should improve the query readability by a lot since it moves all the CASE statements to a different area leaving the query cleaner.

Trovalo · 2020-05-27T13:32:44Z

In the last version of this branch:

I've improved/edited the queries as suggested by @denzilribeiro
the dynamic SQL has been rewritten to be way more readable and manageable
I've already managed the merge with the latest master branch (there were only 2 PRs on this plugin, Fix for perfmon instance_name for #7325 #7526 and Spelling - sqlserver #7524)

Tests:

I've run the tests on SQL Server from 2008 to 2019 and also on Azure SQL DB
A long-running test is now in progress (on SQL 2017 but with a real workload)

Trovalo · 2020-05-29T16:03:41Z

My long running test is raising several warnings

W! [agent] ["outputs.influxdb"] did not complete within its flush interval

The same error sometimes occurs for the sqlserver input plugin, but the same error occurs also on the official version of telegraf even if with less frequency.
I have 2 telegraf instances (the official and the test one) monitoring the same database and writing on different retention policies
I'm not sure about the cause of the error though, @danielnelson have you got any idea about how can I get more info about it?

ssoroka · 2020-06-02T13:59:38Z

@Trovalo That warning is likely unrelated to this plugin. It happens for lossy output connections, and can happen with the old v1 InfluxDB output when gzip is not turned on and there is a lot of data to send. By itself it just means that the write to InfluxDB took longer than whatever you have set for "flush_interval" in your settings, and is not necessarily indicative of an immediate problem.

Trovalo · 2020-06-08T13:34:53Z

@denzilribeiro, @danielnelson any update on this?

danielnelson · 2020-06-08T17:21:15Z