New sql server queries (on-prem) - refactoring and formatting

[Trovalo]

Required for all PRs:

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

This PR affects only the SQL Server on-prem queries (database_type = "SQLServer"), a different PR can be opened for the Azure queries (if this is approved).
The PR does not contain any breaking change, the output is the same as before (with minor fixes)

This PR contains:

• SQL code indentation and formatting
• SQL code refactoring (nothing breaking)
  • Removed @EngineEdition variables (not used anymore)
  • minor edits/fixes to the "sqlServerRequests" query
    • Removed DB_NAME() as [database_name]
    • added COALESCE for [session_db_name] and [open_transaction] (priority to dm_exec_requests, then dm_exec_sessions)

An additional not breaking fix can be made in the PerfCounters query. on line 436.

	INNER JOIN sys.dm_resource_governor_resource_pools AS rgrp /*No fields from this table. remove?*/
		ON rgwg.[pool_id] = rgrp.[pool_id]

this table is joined, but no data is fetched from it. @denzilribeiro, to me this join can be removed, can you confirm? do you have ideas about why it was there?

[denzilribeiro]

this table is joined, but no data is fetched from it. @denzilribeiro, to me this join can be removed, can you confirm? do you have ideas about why it was there?

That predates my time :)... I am not a big fan of the whole joining with workload groups/pools section as should have been just native perf counters ( not manufactured perf counters). Yes seems like join can be eliminated, in all honesty there are counters for workload groups so perhaps is there only for older versions - I left it there for back compat as didn't have context on why it was added.

[Trovalo]

That predates my time :)... I am not a big fan of the whole joining with workload groups/pools section as should have been just native perf counters ( not manufactured perf counters). Yes seems like join can be eliminated, in all honesty there are counters for workload groups so perhaps is there only for older versions - I left it there for back compat as didn't have context on why it was added.

You are right, there are already counters about that, even in SQL 2008. (some sample data below)

object_name	counter_name	instance_name
MSSQL$SQL2008:Workload Group Stats	Active parallel threads	internal
MSSQL$SQL2008:Workload Group Stats	Active requests	internal
MSSQL$SQL2008:Workload Group Stats	Blocked tasks	internal
MSSQL$SQL2008:Workload Group Stats	CPU usage %	internal
MSSQL$SQL2008:Workload Group Stats	CPU usage % base	internal
MSSQL$SQL2008:Workload Group Stats	Max request cpu time (ms)	internal
MSSQL$SQL2008:Workload Group Stats	Max request memory grant (KB)	internal
MSSQL$SQL2008:Workload Group Stats	Query optimizations/sec	internal
MSSQL$SQL2008:Workload Group Stats	Queued requests	internal
MSSQL$SQL2008:Workload Group Stats	Reduced memory grants/sec	internal
MSSQL$SQL2008:Workload Group Stats	Requests completed/sec	internal
MSSQL$SQL2008:Workload Group Stats	Suboptimal plans/sec	internal

I agree on having only native perf counter so I'll make a PR to remove that section for the new query.

To me that's it, at least for this PR. I'll make some more PRs to remove some sections (the one cited above) or add a few fields.

@denzilribeiro I'd like you to have a quick look at it and if everything's looks fine just approve the changes.
(if you like/agree on the formatting style I'll make a PR to format also the Azure queries)

As usual I've tested this queries on SQL 2008 to 2019 and everything looks fine.

[ssoroka]

fine by me. We can merge whenever you're ready.

[Trovalo]

Just one more info from @denzilribeiro before merging

I've run some additional tests with database_type = "AzureSQLDB", which fails now.
The reason is that the Azure queries are referencing an on-prem query (Script: sqlServerSchedulers)

queries["AzureSQLDBSchedulers"] = Query{ScriptName: "AzureSQLDBSchedulers", Script: sqlServerSchedulers, ResultByRow: false}

The same is valid also for Managed Instance
queries["AzureSQLMISchedulers"] = Query{ScriptName: "AzureSQLMISchedulers", Script: sqlServerSchedulers, ResultByRow: false}

Now that there is the check based on engine edition, when the schedulers query is called from any Azure edition an error is returned.

--THis is the check
IF SERVERPROPERTY('EngineEdition') NOT IN (2,3,4) BEGIN /*NOT IN Standard,Enterpris,Express*/
	DECLARE @ErrorMessage AS nvarchar(500) = 'Telegraf - the instance "'+ @@SERVERNAME +'" is not a SQL Server Standard,Enterprise or Express. Check the database_type parameter in the telegraf configuration.';
	RAISERROR (@ErrorMessage,11,1)
	RETURN
END

How do you want to address this kind of issue? the query itself is fully compatible but I'm not sure it's a good idea to reference the on-prem query from the azure section.
We can:

• make the clone of the query to be used by the Azure section
• create a "shared" query area
• extend the check on the query (even if "marked" as on-prem) to also include the Azure editions

[denzilribeiro]

• make the clone of the query to be used by the Azure section

@Trovalo thanks for catching it, I would make a clone follow the same pattern rather than mixing/matching.. . Was a huge PR and I missed it :(, updated one part but not the other.
Let me know when all done and will test/take a look

[Trovalo]

I'm done. @denzilribeiro you are free to check whenever you want

[denzilribeiro]

In this I would modify the throw of error message to include Database name and server name as logical server on Azure DB can have a ton of databases so won't be able to identify which connection string is incorrectly classified

DECLARE @ErrorMessage AS nvarchar(500) = 'Telegraf - Connection string Server:"'+ @@SERVERNAME + ',Database:' + DB_NAME() +'" is not a SQL Server Standard,Enterprise or Express. Check the database_type parameter in the telegraf configuration.';

[Trovalo]

I've edited the error message to include the Database name.

I've also removed some DEADLOCK_PRIORITY (where only dmv were referenced), it is now used only in DatabaseIO and PerfCounters queries

[Trovalo]

@ssoroka can you merge this?

[ssoroka]

Looks like there's conflicts with the other merge.

[Trovalo]

I was aware of a possible confilict, I'll manage it later or tomorrow.

…

On Wed, 7 Oct 2020, 17:56 Steven Soroka, ***@***.***> wrote: Looks like there's conflicts with the other merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8172 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5J5KWOEZ4DQIB3FVGLSZTSJSFULANCNFSM4RW74YWQ> .

[Trovalo]

@ssoroka I've fixed the conflict

[Trovalo]

@ssoroka can you merge this? I'd like it to be present in the next release of Telegraf.