SqlServer - Modified how default server list is set

[avinash-nigam]

Required for all PRs:

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

For SqlServer plugin, modified how the default server list is set. This allows the following three scenarios -

1. Server list is not mentioned in the config - default server (localhost) would be used
2. Server list is defined as empty list - no servers are processed
3. Server list is defined as non-empty list - servers from the list are processed

[avinash-nigam]

@ssoroka - As per conversation here, I've moved the default server list setting to the init method. Could you please help review the same?

@Trovalo, @denzilribeiro - FYI

[srebhan]

Hey @avinash-nigam, could you please outline why you are introducing the NewSQLServer() function? It's not that the change cannot be accepted, but why not doing the minimum necessary change?

[avinash-nigam]

Hey @avinash-nigam, could you please outline why you are introducing the NewSQLServer() function? It's not that the change cannot be accepted, but why not doing the minimum necessary change?

@srebhan - This was introduced with the thought that if we need extra processing steps before creating the initializing object, then those can be done in this function. I see such usage in other plug-ins too. Having said that, I see no harm in directly initializing in the init method instead of creating a separate method.

[srebhan]

@avinash-nigam please see my comment above and let me know on how to proceed.

[avinash-nigam]

@avinash-nigam please see my comment above and let me know on how to proceed.

@srebhan - I've updated code as per above conversation.

[srebhan]

@avinash-nigam the code looks good but let me ask one question: Now if the user enters an empty list, i.e. he/she uncomments the server line and specifies servers=[] you will not output any metric nor output any error. Is this by intention? Before the default server was used in these cases...
What is the reasoning behind changing this behavior?

[avinash-nigam]

@avinash-nigam the code looks good but let me ask one question: Now if the user enters an empty list, i.e. he/she uncomments the server line and specifies servers=[] you will not output any metric nor output any error. Is this by intention? Before the default server was used in these cases...
What is the reasoning behind changing this behavior?

Yes @srebhan, the intention was to not throw an error (and obviously no metrics either) when the server list is defined as empty, considering it as an indication from the user to not process anything.

This change's background context is here. Below is a summary of the points that were in consideration -

1. Existing code was not the correct pattern to set default values (as Steven @ssoroka pointed out in the above mentioned conversation). The defaults should be set in the init() method.
2. The default server as localhost works only for when the database type is SQL Server, but won't work (throw errors) for other database types (Azure SQL Database or Azure SQL Managed Instance). In order to address this, we wanted to provide a non-erroneous option and support the following scenarios -

• Server list is not mentioned in the config - default server (localhost) would be used
• Server list is defined as empty list - no servers are processed
• Server list is defined as non-empty list - servers from the list are processed

3. Use case - When a config file is defined with multiple [[inputs.sqlserver]] configurations (one for each database type) and the server list populated based on the database type being monitored, then, keeping the server list empty (since that database type is not monitored) results in errors, even though the intention was not to monitor anything. Although a less likely scenario, but we did encounter this while setting up a monitoring automation on our end.

I'd happy to hear your thoughts on this.

++ @ssoroka

[srebhan]

@avinash-nigam I understand your comment, but I don't get why the user specifies a plugin and does not want to process anything!? I guess the more real-world use-case is that the user just forgot to specify the server list or did expect something wrong. This would only make sense for some automatically generated configs where the server-list might be empty, but then just fix the generator... To be honest I would rather error out in this case as most likely the user did something that is very unlikely to be wanted (forgot list or assumes some default behavior erroneously).

[srebhan]

But yeah, let @ssoroka decide. ;-)

[ssoroka]

LGTM. I think you need to ask it from the user's perspective: "Ok so I've set the servers list to empty, and I start Telegraf. What do I expect it to do?". There's two cases: 1. I meant to set it to [] to temporarily disable collection, or 2. It's an accident and I meant to write a value there. In case 2, I'd love to see a warning that it's starting with no servers in the list. In case 1, I don't really want it to crash just because my configuration management wrote out zero urls.

based on these I think you have your ideal answer: don't crash or error, but write out a warning in the (big i) Init() error function if the user starts with no servers.

[srebhan]

I agree with the warning, but a different aspect is that a lot of other plugins use some sensible default on empty lists (like include everything etc.). From this perspective it might be unexpected that nothing is collected. But if there is a warning the user will likely be able to discover the problem if there is any.

[avinash-nigam]

LGTM. I think you need to ask it from the user's perspective: "Ok so I've set the servers list to empty, and I start Telegraf. What do I expect it to do?". There's two cases: 1. I meant to set it to [] to temporarily disable collection, or 2. It's an accident and I meant to write a value there. In case 2, I'd love to see a warning that it's starting with no servers in the list. In case 1, I don't really want it to crash just because my configuration management wrote out zero urls.

based on these I think you have your ideal answer: don't crash or error, but write out a warning in the (big i) Init() error function if the user starts with no servers.

@ssoroka - As per your comment, added a warning when the server list is empty.
@srebhan - FYI

[srebhan]

Looks good to me.

[avinash-nigam]

Thank you @ssoroka and @srebhan for your help on this.