@denzilribeiro @dimitri-furman could you please review?
End-to-end tests were done using telegraf standalone configuration and Azure SQL Insights solution.

However, can you please comment on Stevens question regarding a "feature-enable list" instead of using booleans to switch on/off features? I think it is much more extensible...

@ssoroka , @srebhan -- This is no different than turning queries on/off right aka feature-enable list or include_queries list is the same ? Unless the feature-enable groups a set of queries into a "feature" that you turn on/off. The difference with query store is that it is generally different in terms of granularity of collection and the query is a bit more heavy duty than say the other light weight queries and isn't really a live monitoring metric but lets you analyze queries and perf that contributed to resource usage in time.

The crux of the problem is we wanted to disable by default and have someone explicitly enable, so we wanted a disable by default, explicitly opt in behavior.

Take for example someone who had telegraf prior to this PR, doesn't change their config files to exclude this explicitly, they get this collector turned on by default and will have to explicitly add to exclude_query list -- if that is fine then we can just use the exclude_list . That isn't just for query store related collector only but should agree upon an approach for any of the more "advanced" collectors or bit more heavy weight collectors. If exclude_query is the way to go that is fine just we would introduce new things for existing customers that don't change their config file but update telegraf. Query store being a bit of a different type of collector thought of using explicit flag but I get the point that a flag for a feature in time can explode the conf section.

We do understand that the same could be achieved with this ( and just use exclude list) but it won't be disable by default, rather onus is on customers.
exclude_query = ["SQLServerAvailabilityReplicaStates", "SQLServerDatabaseReplicaStates","AzureSQLDBQueryStoreRuntimeStatistics","AzureSQLDBQueryStoreWaitStatistics"]

@denzilribeiro the difference is in the extensibility of both schemes. Imagine adding another "thing to collect".

With the boolean approach you will need to extend the config (sampleConfig, README, struct) with another option that is say disabled by default.

With the feature list approach, you just implement the feature and add the "feature enable string" to the sampleconfig and README documentation. No need to change the actual configuration. Furthermore, if you have more than a handful of those feature enable/disable toggles, a include list is much more readable than a bulky list of true/false options.
You can still keep the backward compatibility by carefully crafting the feature_include/feature_exclude lists.

So yes, both approaches are equivalent feature-wise, but the latter is much more extensible/readable in the long-run.

You can still keep the backward compatibility by carefully crafting the feature_include/feature_exclude lists.
So yes, both approaches are equivalent feature-wise, but the latter is much more extensible/readable in the long-run.

Right now feature_include/feature_exclude is the same as exclude_query /include_query which is already implemented. So if the decision is still to revert to that, then it is fine

At some point look at this a bit more holistically. The issue I have with include/exclude you don't have a notion of "default collectors" for those that don't specify the include/exclude lists and so that moves the onus to customers.

Say we have queries a,b,c,d in existing version and now we introduce a query e that we want enabled by default, and a query f that we want disabled by default
a. Customers that have an include list in existing conf file have to add query e
b. Customers with an exclude list in their conf file will have to add query f to exclude it
c. Customers that do not have an include/exclude list will get both e and f.

Sure we can change the default conf to reflect it, but a customer that already has a conf and doesn't update it won't get it, maybe I am missing something :)

Right now feature_include/feature_exclude is the same as exclude_query /include_query which is already implemented. So if the decision is still to revert to that, then it is fine

I'm fine with using the existing lists.

At some point look at this a bit more holistically. The issue I have with include/exclude you don't have a notion of "default collectors" for those that don't specify the include/exclude lists and so that moves the onus to customers.

I'm not sure I understand your comment correctly. You can still define a default include or exclude set in your plugin and put that into the sample config. If the user chooses to modify it, he/she will start with the default and adapt to the needs.

Say we have queries a,b,c,d in existing version and now we introduce a query e that we want enabled by default, and a query f that we want disabled by default

Then I would specify:

# query_include = ["a", "b", "c", "d", "e"]
# query_exclude = []

in your sample config and set these options when setting up the plugin in init() (lowercase i):

func init() {
	inputs.Add("sqlserver", func() telegraf.Input {
		return &SQLServer{
			Servers: []string{defaultServer},
			IncludeQuery: []string{"a", "b", "c", "d", "e"},
		}
	})
}

a. Customers that have an include list in existing conf file have to add query e

Right, but that is the semantics behind an include list. You explicitly whitelist a set of features. So you don't want to get new stuff automagically. :-)

b. Customers with an exclude list in their conf file will have to add query f to exclude it

No because you did not include it in the first place, so you don't need to exclude it. Note you are free to craft the include list such that only ever backward-compatible features are enabled. The exclude list will then superseed the include list.

c. Customers that do not have an include/exclude list will get both e and f.

See above. Just do not specify e and f in your default include list.

Sure we can change the default conf to reflect it, but a customer that already has a conf and doesn't update it won't get it, maybe I am missing something :)

Right, but again, if somebody sets an include list, he/she will get exactly the stuff he/she specified. That's the intention, no surprises. ;-)

Will change to include/exclude lists for now and revisit any other changes later.

Just to clarify concern wasn't ever of someone using include lists as they get exactly what they want, no more, no less. Concern was folks that weren't using include lists.. or using exclude lists in getting a potentially heavier collector since the default for all collectors as premise is by default all collectors enabled.

My experience has been in my limited experience with telegraf that folks don't necesarily update their conf files often yet update telegraf major versions but then I maybe wrong

[...] Concern was folks that weren't using include lists.. or using exclude lists in getting a potentially heavier collector since the default for all collectors as premise is by default all collectors enabled.

Well almost. Premise is be backward-compatible. That is, if you add a new feature, you now have to explicitly define the include list (instead of "use all") to exclude the new feature. If you had feature a to d in the first version of telegraf and default was "include all" and you now add feature e you have to define include=["a", "b", "c", "d"] as the new default. This way, people not using the include-list will still get what they got before. Everybody else will get what they defined.

My experience has been in my limited experience with telegraf that folks don't necesarily update their conf files often yet update telegraf major versions but then I maybe wrong

No you are right and that's exactly the point. Please craft the include list in a way that you get the same results with the old and new version of telegraf.

@srebhan, the flag was removed. Are we good to merge this pull request?

@masree any update here?

@srebhan unfortunately we will not be able to continue work on this PR in foreseeable future. My impression is that it is close to meeting the quality bar for merging once your comment is addressed. Perhaps this can be finished by other contributors at some point in the future. Apologies, and thanks for understanding.

CC: @masree

Closing this issue. If anyone is interested in picking this up please feel free to re-open.