SQL Server HA/DR Availability Group queries #8379

avinash-nigam · 2020-11-09T20:50:46Z

Required for all PRs:

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

Includes changes for

Queries for HA/DR Availability Group for database type - SQL Server (IaaS/VM), these are optional queries by default
Fixed a bug in SQLServerProperties query

Merging original master to forked master

avinash-nigam · 2020-11-09T20:51:44Z

@denzilribeiro could you please review this?

denzilribeiro

Ideally please split the PR as it has HADR changes, a config change and Perfmon counter fixes.. Limit this one to HADR related, and then create a new PR for other stuff so is easily trackable.

plugins/inputs/sqlserver/azuresqlqueries.go

plugins/inputs/sqlserver/sqlserverqueries.go

plugins/inputs/sqlserver/sqlserver.go

avinash-nigam · 2020-11-12T08:05:55Z

Ideally please split the PR as it has HADR changes, a config change and Perfmon counter fixes.. Limit this one to HADR related, and then create a new PR for other stuff so is easily trackable.

That makes sense, pulled out the perfmon counters' commit from this PR. Now this only has HADR related changes.

denzilribeiro

Looks good, other than 2 checks requested

plugins/inputs/sqlserver/sqlserverqueries.go

plugins/inputs/sqlserver/sqlserver.go

…abled

denzilribeiro

Avinash I am super sorry about this was my mistake. I had asked to add this check
IF SERVERPROPERTY('EngineEdition') NOT IN (2,3,4) OR SERVERPROPERTY('IsHadrEnabled')
But now am thinking if someone doesn't have HADR enabled, the log will be flooded with messages.
I think would be better to revert this way. My apologies for having you make the changes that I didn't think out well.

IF SERVERPROPERTY('EngineEdition') NOT IN (2,3,4)
... Error message

IF SERVERPROPERTY('IsHadrEnabled') ==1
.. Execute query

mikedavem · 2020-11-18T20:17:17Z

plugins/inputs/sqlserver/sqlserverqueries.go

+SELECT
+	'sqlserver_hadr_avreplica_states' AS [measurement],
+	REPLACE(@@SERVERNAME,'\',':') AS [sql_instance],
+	convert(nvarchar(36), hars.replica_id) as replica_id,
+	ar.replica_server_name,
+	convert(nvarchar(36), hars.group_id) as group_id,
+	ag.name AS group_name,
+	ar.replica_metadata_id,
+	availability_mode,
+	availability_mode_desc,
+	failover_mode,
+	failover_mode_desc,
+	session_timeout,
+	primary_role_allow_connections,
+	primary_role_allow_connections_desc,
+	secondary_role_allow_connections,
+	secondary_role_allow_connections_desc,
+	seeding_mode,
+	seeding_mode_desc,
+	basic_features,
+	is_distributed,
+	is_local,
+	role,
+	role_desc,
+	operational_state,
+	operational_state_desc,
+	connected_state,
+	connected_state_desc,
+	recovery_health,
+	recovery_health_desc,
+	synchronization_health,
+	synchronization_health_desc,
+	last_connect_error_number,
+	last_connect_error_description,
+	last_connect_error_timestamp
+from sys.dm_hadr_availability_replica_states AS hars
+inner join sys.availability_replicas AS ar on hars.replica_id = ar.replica_id
+inner join sys.availability_groups AS ag on ar.group_id = ag.group_id


Hi, very nice idea :) May I suggest to add info about AG state in this query (or maybe in a different query to keep consistent) ? :

SELECT 'sqlserver_hadr_avreplica_states' AS [measurement], REPLACE(@@SERVERNAME,'\',':') AS [sql_instance], convert(nvarchar(36), hars.replica_id) as replica_id, ar.replica_server_name, convert(nvarchar(36), hars.group_id) as group_id, ag.name AS group_name, ag.basic_features, ag.is_distributed, hags.synchronization_health_desc AS ag_synchronization_health_desc, ar.replica_metadata_id, ar.availability_mode , ar.availability_mode_desc, ar.failover_mode, ar.failover_mode_desc, ar.session_timeout, ar.primary_role_allow_connections, ar.primary_role_allow_connections_desc, ar.secondary_role_allow_connections, ar.secondary_role_allow_connections_desc, ar.seeding_mode, ar.seeding_mode_desc, hars.is_local, hars.role, hars.role_desc, hars.operational_state, hars.operational_state_desc, hars.connected_state, hars.connected_state_desc, hars.recovery_health, hars.recovery_health_desc, hars.synchronization_health AS replica_synchronization_health, hars.synchronization_health_desc AS replica_synchronization_health_desc, hars.last_connect_error_number, hars.last_connect_error_description, hars.last_connect_error_timestamp from sys.dm_hadr_availability_replica_states AS hars inner join sys.availability_replicas AS ar on hars.replica_id = ar.replica_id inner join sys.availability_groups AS ag on ar.group_id = ag.group_id INNER JOIN sys.dm_hadr_availability_group_states AS hags ON hags.group_id = ag.group_id

I am not for having a separate query, which means yet another one to run in an iteration. @avinash-nigam if adding group state you may have to name the columns appropriately for state :)

One more thing @avinash-nigam - can we rename the measurement to sqlserver_hadr_replica_states ( no need for the av part?)

Thanks much!

@mikedavem included AG state as per your and Denzil's suggestions
@denzilribeiro removed the "av" part from the measurement

…te columns

avinash-nigam · 2020-11-19T00:15:11Z

Avinash I am super sorry about this was my mistake. I had asked to add this check
IF SERVERPROPERTY('EngineEdition') NOT IN (2,3,4) OR SERVERPROPERTY('IsHadrEnabled')
But now am thinking if someone doesn't have HADR enabled, the log will be flooded with messages.
I think would be better to revert this way. My apologies for having you make the changes that I didn't think out well.

IF SERVERPROPERTY('EngineEdition') NOT IN (2,3,4)
... Error message

IF SERVERPROPERTY('IsHadrEnabled') ==1
.. Execute query

@denzilribeiro made changes as per your suggestion

denzilribeiro

Looks good @Trovalo - can you also take a look at this?

avinash-nigam · 2020-11-30T22:10:46Z

Looks good @Trovalo - can you also take a look at this?

@Trovalo - any thoughts on the PR? or, shall I go ahead and request for a merge?

Trovalo

Sorry I got overwhelmed and forgot about it last week. Looks fine to me.

avinash-nigam · 2020-12-01T19:53:31Z

Thank you @denzilribeiro and @Trovalo for the review and approvals.
@ssoroka and @reimda - could you please help merge the changes?

ssoroka · 2020-12-01T20:12:22Z

plugins/inputs/sqlserver/sqlserver.go

+	DatabaseType        string   `toml:"database_type"`
+	IncludeQuery        []string `toml:"include_query"`
+	ExcludeQuery        []string `toml:"exclude_query"`
+	IgnoreDefaultServer bool     `toml:"ignore_default_server"`


I don't understand why we need an "ignore default server" flag.

Thank you @ssoroka for taking a look at this. The "ignore default server" flag was added for the case when the server list is empty and no server should be monitored. Without the flag, an empty server list would cause the localhost to be monitored, which would throw errors when using the agent for remote monitoring. Also, the localhost mechanism is valid only for SQL Server database type, but not for Azure SQL Database or Azure SQL Managed Instance. By adding the flag, the previous experience would work as is, and whoever does not want to monitor localhost can use this flag.

I didn't spot that. tbh, I don't get why someone should leave the list empty even for localhost, I know the plugin will look for the default instance (port 1433) on localhost but I've never used that anyway as I want all to be explicitly set.

I'm not against the change, I just find it odd, if you don't want to use the plugin you can simply comment it out, or if you use multiple .conf file and the --config-directory you can simply change the file extension to something different than .conf and it will be ignored (I personally use this pattern input_sqlserver.conf.ignore.

In any case, this looks out of scope for this PR, as it should just add the new queries, can you propose this in a new PR?

This isn't how we normally do defaults anyway. the default should go in the init() function, and then you don't need conditional checks like

if len(s.Servers) == 0 { s.Servers = append(s.Servers, defaultServer) }

that assume empty lists are unset. Let's remove this boolean and move the default to the init() function, and then explicitly empty lists will work properly.

Thank you @ssoroka and @Trovalo for the feedback. Let me pull this change out and create a separate PR for it, so that HA/DR queries related changes can go ahead and get merged.

@ssoroka and @Trovalo - I've reverted the default flag related changes and will create a separate PR for it. This PR now contains only changes for HA/DR queries. Please let me know if you have further feedback on this. If not, it'd be great to get this merged. Thank you.

@ssoroka - Could you please help merge this PR if there are no concerns?

ssoroka · 2020-12-07T22:08:13Z

plugins/inputs/sqlserver/sqlserver.go

+	if len(s.Servers) == 0 {
 		s.Servers = append(s.Servers, defaultServer)
 	}


This needs to move to the init() function to be override-able. You won't need the conditional there, so just s.Servers = []string{defaultServer}

@ssoroka - That's an existing code, I can address it when I try to address the default server aspect in a separate PR. This PR is only for the HA/DR queries now.

Alright. Please do make another PR for it, as it's currently busted, as you've noticed.

Sure, will do that. Thank you!

avinash-nigam and others added 2 commits October 29, 2020 03:19

Merge pull request #1 from influxdata/master

8d61ca4

Merging original master to forked master

added queries for hadr availability group and few other modifications

878ce05

fixed availability group query bug in sql server plugin

742d58a

denzilribeiro suggested changes Nov 11, 2020

View reviewed changes

avinash-nigam force-pushed the sql-hadr-avgroup-queries branch from e7b0eef to 742d58a Compare November 12, 2020 08:01

denzilribeiro mentioned this pull request Nov 16, 2020

SQL Server plugin: add availabity group feature #6854

Closed

3 tasks

denzilribeiro suggested changes Nov 16, 2020

View reviewed changes

plugins/inputs/sqlserver/sqlserverqueries.go Show resolved Hide resolved

plugins/inputs/sqlserver/sqlserverqueries.go Show resolved Hide resolved

plugins/inputs/sqlserver/sqlserver.go Outdated Show resolved Hide resolved

added condition to execute hadr sql server queries to execute when en…

38fe95f

…abled

denzilribeiro suggested changes Nov 18, 2020

View reviewed changes

mikedavem reviewed Nov 18, 2020

View reviewed changes

updated condition to execute hadr sql server queries and added ag sta…

57f741d

…te columns

denzilribeiro approved these changes Nov 23, 2020

View reviewed changes

sjwang90 added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 23, 2020

Trovalo approved these changes Dec 1, 2020

View reviewed changes

ssoroka reviewed Dec 1, 2020

View reviewed changes

reverted changes related to ignoring default server in sql server plugin

56e3baa

denzilribeiro approved these changes Dec 7, 2020

View reviewed changes

ssoroka reviewed Dec 7, 2020

View reviewed changes

ssoroka merged commit e687cd7 into influxdata:master Dec 8, 2020

avinash-nigam mentioned this pull request Jan 5, 2021

SqlServer - Modified how default server list is set #8655

Merged

3 tasks

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

SQL Server HA/DR Availability Group queries (influxdata#8379)

20bb577

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL Server HA/DR Availability Group queries #8379

SQL Server HA/DR Availability Group queries #8379

avinash-nigam commented Nov 9, 2020 •

edited

Loading

avinash-nigam commented Nov 9, 2020

denzilribeiro left a comment

avinash-nigam commented Nov 12, 2020 •

edited

Loading

denzilribeiro left a comment

denzilribeiro left a comment

mikedavem Nov 18, 2020

denzilribeiro Nov 18, 2020

avinash-nigam Nov 19, 2020

avinash-nigam commented Nov 19, 2020

denzilribeiro left a comment

avinash-nigam commented Nov 30, 2020

Trovalo left a comment •

edited

Loading

avinash-nigam commented Dec 1, 2020

ssoroka Dec 1, 2020

avinash-nigam Dec 2, 2020

Trovalo Dec 2, 2020

ssoroka Dec 2, 2020

avinash-nigam Dec 2, 2020

avinash-nigam Dec 2, 2020

avinash-nigam Dec 7, 2020

ssoroka Dec 7, 2020

avinash-nigam Dec 7, 2020

ssoroka Dec 8, 2020

avinash-nigam Dec 8, 2020

SQL Server HA/DR Availability Group queries #8379

SQL Server HA/DR Availability Group queries #8379

Conversation

avinash-nigam commented Nov 9, 2020 • edited Loading

Required for all PRs:

avinash-nigam commented Nov 9, 2020

denzilribeiro left a comment

Choose a reason for hiding this comment

avinash-nigam commented Nov 12, 2020 • edited Loading

denzilribeiro left a comment

Choose a reason for hiding this comment

denzilribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinash-nigam commented Nov 19, 2020

denzilribeiro left a comment

Choose a reason for hiding this comment

avinash-nigam commented Nov 30, 2020

Trovalo left a comment • edited Loading

Choose a reason for hiding this comment

avinash-nigam commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinash-nigam commented Nov 9, 2020 •

edited

Loading

avinash-nigam commented Nov 12, 2020 •

edited

Loading

Trovalo left a comment •

edited

Loading