Split up sql script and config options for SQL Server plugin on database_type

[denzilribeiro]

Feature Request

a. Split up collector queries base don whether they are for Cloud versions ( Azure SQL DB / Managed instance) or on-prem
b. Enable a new configuration option called database_type or database_engine_type to simplify the queries and make maintainability of the plugin and test matrix much easier.

Opening a feature request kicks off a discussion.

Proposal:

Introduce a configuration parameter which could have one of 4 values
database_type = "azure_sql_db" / "azure_sql_managed_instance" / "sqlserver"

Based on the choice, then a set of queries will be enabled for this collector allowing the normal inclusion/exclusion within that set. This will allow us to separate queries out and make them smaller rather than each collector having to support all 3 engine types. In future I could add "azure_sql_elastic_pools" support which is a different concept totally.

If the user does NOT specify this parameter, it will default to the older mechanisms based on V1/V2 choice of queries. If they do, then will follow this path

In code then could have a set of queries enabled by each type:
a. If database_type="sql_server", then Queries allowed = Perfstats, WaitStats, DatabaseIO ,AvailabilityGroups ...
b, If database_type = "azure_sql_db" then Queries allowed = PerfStats, AzureDBWaitstats, WaitStats,
, DatabaseIO, AzureDBResourceStats

This would also enable Config to be simpler for example could have a section each with servers for each type of SQL edition
[[inputs.sqlserver]]
servers = ["Server=192.168.1.10;Port=1433;UserId=;Password=;app name=telegraf;log=1;"]
database_type = "azure_sql_db"

[[inputs.sqlserver]]
servers = ["Server=201.168.1.20;Port=1433;UserId=;Password=;app name=telegraf;log=1;"]
database_type = "sql_server"

Current behavior:

Currently from a configuration option you have 3 possible alternatives
a. You choose V1 queries - which are obsolete today and won't really cover this as not many people using this today.
b. You choose V2 queries - This covers all the queries relevant for on-premise version of SQL server. These queries are also elligible for all the cloud versions which means each of these queries when changed have to be tested for all versions SQL 2008 - SQL 2019, Azure SQL Managed instance Azure SQL Database
c. You choose azuredb = true/false - this enables few additional queries for cloud versions ( Azure SQL Managed instance/ Azure SQL DB.

Irrespective if a query is "relevant" to cloud versions or not, they are run unless excluded, each query today now has to check Engine Edition and branch out which means in all cases a connection is still made to SQL server, and the check is within the SQL query.

Desired behavior:

It would be much easier for a user to have 3 config sections and split out their connection strings based on database type.
This would enable "less" queries to run based on engine type

Use case:

Today there are fundamentally 2 problems
a. Most collectors have to special case things across cloud versions and on-premise making maintenance more and more difficult.
b. Changing any core collectors has a big testing matrix today and results in bugs.
c. Configuration for cloud is only and on or off switch and all it does is now runs these cloud specific collectors on every server in the config file but by default still runs all the collectors for on-premise.

Some examples:
a. On azure SQL DB today I may want to collect sys.dm_os_wait_stats and sys.dm_db_wait_stats, today they are clubbed together.
b. On-premise any of the resource governance collection is meaningless

The challenge is a bit of backward compabitibility. Would be much easier of we could deprecate V1 queries support in a shorter window to reduce that code which is expensive to run and not comprehensive.

[denzilribeiro]

@Trovalo , @danielnelson - if get a chance any thoughts?

[danielnelson]

Sounds like a good plan to me.

On the v1 queries, we should add logging prompting the user to update to version 2. We should split these queries off into their own directory so they will be easier to remove and be out of the way.

I think we may have discussed this over email though I don't remember our decision, how about we also log a warning message if database_type is unset, and then run a new query to detect the type of the database. The check would run each interval, which won't be ideal for performance but the fix is just to add the database_type option.

[Trovalo]

I like the idea as it will make development easier.

The only part that concerns me is the output structure of the different queries, especially those that are not exclusive to one "db engine", which (to me) should be kept as uniform as possible. (which is the current situation)

As an example, let's say that I want to have a query that gets the CPU data. I can build 3 versions of it, one for each "db engine" but it will be also important that those queries share more or less the same output format, so I can store them in the same measurement and easily access the data of any kind of "db engine".

Of course, this is valid only for generic queries like the majority that now are in the plugin, any "db engine" specific query can have his own measurement.

[danielnelson]

That is a really good point, if we make this change it will be easier for the queries to drift apart in ways that are not required. I think one way we can combat this is by improving the documentation around each query in the style used by other plugins and making sure to note any results which differ.

[denzilribeiro]

Some of the guidance should be
a. Measurement is the same if the source DMV is the same. If not makes sense to have separate measurement
b. We can cross review PRs as well
c. Documentation :) would be more important perhaps with each new addition aka -- what is collector, what is measurement, what is DMV it pulls from

Benefits still outweigh it considerably IMO.