Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sqlserver_extensible input plugin #3069

Closed
wants to merge 1 commit into from

Conversation

zensqlmonitor
Copy link
Contributor

This sqlserver plugin provides metrics for your SQL Server instance.
It has been designed to parse SQL statements defined in the plugin section of your telegraf.conf or imported from a file.

Required for all PRs:

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

@danielnelson
Copy link
Contributor

If we assume that #2785 is merged in the future, I'd need to have a clear understanding of when a user would want to use this specialized plugin vs the generic plugin. This way we can provide guidance on which plugin to use.

With that in mind, what are the main reasons to use this plugin over that generic sql plugin? Is there functionality that it lacks or cannot be done using only the generic api?

@zensqlmonitor
Copy link
Contributor Author

With the generic plugin:

  • we can define only one provider; it won't work in the case of remote dedicated Telegraf server to pull metrics for many database providers. That is currently possible with separate plugins.
  • server metrics are collected synchronously, meaning that if you have a blocking query for one server you won't get any data collected for the other servers.
  • Keep connection option: even with a small performance hit opening the connection (we have here a monitoring agent not a trading agent), I would recommend closing each connection as soon as you are done using it.. That's an option we are allowed to change, ok

With the sqlserver_exensible plugin, I added a minimum version check (as in postgres) allowing or not to execute queries for avoiding errors. This kind of specificity could be lost with a generic plugin.
Excepted this (and the points mentioned above), it does not bring more than the generic plugin.
Any thoughts?
BR/

@danielnelson
Copy link
Contributor

we can define only one provider; it won't work in the case of remote dedicated Telegraf server to pull metrics for many database providers. That is currently possible with separate plugins.

What do you mean by provider?

server metrics are collected synchronously, meaning that if you have a blocking query for one server you won't get any data collected for the other servers.

This will need to be fixed in the generic plugin.

Keep connection option: even with a small performance hit opening the connection (we have here a monitoring agent not a trading agent), I would recommend closing each connection as soon as you are done using it.. That's an option we are allowed to change, ok

This should probably be configurable, I know some work has gone into adding keep-alive support to the postgres extensible plugin.

I can see two possibilities for the version check feature in a database neutral way. It could be pushed into the query, and we could suggest how to do that for popular databases. Another option would be allowing a free form predicate query. I'm currently inclined to require pushing it into the query, it seems like this could be more flexible.

Overall goal though is a juggling act of the normal stuff:

  • Reduce code size and duplication
  • Consistent behavior between plugins
  • Easy to use and understand
  • Not make common tasks impossible

Hopefully we can find the goldilocks zone.

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
@Fank
Copy link

Fank commented Feb 1, 2018

Any status on this?

@danielnelson
Copy link
Contributor

@Fank We will likely merge #2785 for this functionality, unless we end up needing sqlserver specific functionality, it is on the todo list but keeps slipping behind other priorities.

@smummert
Copy link

When will the extensible be merged? We need to query SSRS usage and other items as well. Thanks

@jcajkovic
Copy link

+1

@TheRockStarDBA
Copy link

This will be super useful if implemented (merged). Microsoft uses telegraf and influxdb for sql server monitoring. See - https://github.com/denzilribeiro/sqlmimonitoring

If we have an ability of use custom queries to monitor sql server that will be super useful. E.g. from @zensqlmonitor repo - https://github.com/zensqlmonitor/influxdb-sqlserver/tree/master/sqlscripts , I can use the scripts to report on various metrics that I am interested in.

@zensqlmonitor
Copy link
Contributor Author

1 year and half later and still no progress...

I'm the initial contributor of the sql server plugin but coding the statements inside the code is not the best way. With the use of custom queries, this extensible plugin will provide more flexibility.

Any chance to see this plugin integrated in Telegraf soon?

@danielnelson
Copy link
Contributor

@zensqlmonitor I am still hoping that we can do a shared general purpose sql plugin. If I finish up a plugin that is a combination of the generic sql plugin in #2785 and this plugin could you help test on sql server? I think that the only thing that wouldn't fit from this design is the version check, perhaps it could be moved into the queries without too much impact?

@danielnelson danielnelson self-assigned this Jun 8, 2020
@danielnelson danielnelson removed their assignment Oct 21, 2020
@Trovalo
Copy link
Collaborator

Trovalo commented Nov 2, 2020

Sorry to bother you @ssoroka, but I'd like to know your opinion on this one.
The "generic SQL plugin" is still stuck after all this time, while this one is almost ready to go.

Since recently the SQL plugin has been "split" into 3 sections On-Prem | AzureDB | AzureManagedInstance we just need to mirror this kind of switch and it should be ready to go. (as of now its check makes sense only for On-prem)

Do you think the generic SQL plugin will see the light soon?
Will this plugin get accepted if the generic one is still stuck for X time?

Being able to run custom queries without having to hard code the statements and build the executable every time will be handy

@sjwang90 sjwang90 added area/sqlserver new plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 18, 2020
@sjwang90 sjwang90 added this to the Planned milestone Dec 9, 2020
@sjwang90 sjwang90 added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 26, 2021
@sjwang90 sjwang90 removed this from the Planned milestone Jan 29, 2021
@srebhan srebhan self-assigned this Jul 2, 2021
@srebhan
Copy link
Member

srebhan commented Jul 2, 2021

Hey @zensqlmonitor, @TheRockStarDBA and @Trovalo,

this PR is unfortunately sitting here for almost 4 years now, sorry for this situation! So time to tackle it. :-) With v1.19 we merged plugins/inputs/sql which superseeded #2785 and allows to perform SQL queries against different types of SQL server. I've checked and go-mssql is also supported.
So let me try to understand the relation of this plugin to the (now) existing one. As far as I can see from the conversation above the differences are

we can define only one provider; it won't work in the case of remote dedicated Telegraf server to pull metrics for many database providers. That is currently possible with separate plugins.

I'm not sure I understand this point. With the merged sql plugin, you can define one driver that queries one server. By replicating the plugin config you can support an arbitrary combination and number of server and types. Querying multiple type/host combinations in one plugin mostly adds complexity (concurrency handling, etc) at the small benefit of reducing the number of configuration entries...

server metrics are collected synchronously, meaning that if you have a blocking query for one server you won't get any data collected for the other servers.

See above. With the merged sql plugin you can configure multiple instances of the plugin running in parallel.

Keep connection option: even with a small performance hit opening the connection (we have here a monitoring agent not a trading agent), I would recommend closing each connection as soon as you are done using it.. That's an option we are allowed to change, ok

This is a valid point and we can add this to the merged sql plugin. PRs welcome.

Please don't get me wrong, if there is an aspect in this PR that cannot be folded into the generic plugin, I'm open for reviewing and merging this PR.

@sspaink
Copy link
Contributor

sspaink commented Sep 13, 2021

Closing this pr now that the generic sql plugin has been merged and released. Thanks to everyone who contributed to this pr.

@sspaink sspaink closed this Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants