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

Generic SQL input #8735

Merged
merged 24 commits into from
Jun 15, 2021
Merged

Generic SQL input #8735

merged 24 commits into from
Jun 15, 2021

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Jan 22, 2021

Required for all PRs:

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

This PR adds a generic SQL input plugin that can be used to query SQL servers and convert the returned data into metrics. In a first attempt only a handful of drivers are supported (see SQL_DRIVERS_INPUT.md) but there are more on the list.

Currently a test-suite is missing, therefore I'm marking this as DRAFT. Reviews, comments, suggestions and tests are welcome!

Takeover of PR #2785
Fixes #352

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@srebhan srebhan force-pushed the sql branch 4 times, most recently from 5e21e55 to e411f69 Compare January 22, 2021 19:23
@srebhan
Copy link
Member Author

srebhan commented Jan 26, 2021

!signed-cla

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A work of art. well done

plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
@srebhan
Copy link
Member Author

srebhan commented Jan 26, 2021

!signed-cla

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@sspaink
Copy link
Contributor

sspaink commented Jan 26, 2021

!signed-cla

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@srebhan
Copy link
Member Author

srebhan commented Jan 26, 2021

For running the tests, please create a testdata directory in the sql-plugin dir. Then download and unzip the MariaDB sample data and put the nation.sql file in the testdata directory. I skipped this as I cannot find out what the License of this dataset is.

If those preparations are successful, run go test -args -spinup in the sql-plugin dir. Keep fingers crossed! ;-P

@srebhan srebhan mentioned this pull request Jan 27, 2021
3 tasks
@srebhan
Copy link
Member Author

srebhan commented Jan 27, 2021

!signed-cla

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 ✅ CLA has been signed. Thank you!

@helenosheaa helenosheaa added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 4, 2021
@sspaink
Copy link
Contributor

sspaink commented Feb 17, 2021

!retry-checks

@aslgithub
Copy link

@srebhan Hi Sven, any chance you could get this pull completed so SQL queries can be run as a Telegraf input please ?
Got excited when you took this PR over in January that someone would be able to complete this functionality..
Thanks !

@srebhan srebhan marked this pull request as ready for review May 11, 2021 13:19
@srebhan
Copy link
Member Author

srebhan commented May 11, 2021

@aslgithub your wish is my command. ;-P
Testing this is very much appreciated! You can use the artifacts built by CI by expanding telegraf-tiger's comment. :-)

@aslgithub
Copy link

@srebhan nice one, thank you, much appreciated :-)
Will test this against Microsoft SQL Server.

@srebhan srebhan requested a review from ssoroka May 17, 2021 15:19
@aslgithub
Copy link

@aslgithub your wish is my command. ;-P
Testing this is very much appreciated! You can use the artifacts built by CI by expanding telegraf-tiger's comment. :-)

@srebhan
Hi Sven,
I've started testing against Microsoft SQL.

It appears to work as expected in that I am able to connect to the SQL server and complete a basic query that does a select count from a table to return an integer.

Specifying a database in the dsn string within inputs.sql doesn't seem to have any impact?
However I believe that might be as intended given you can have multiple inputs.sql.query each targeting their own databases and tables on the same server connection ?

Would it be possible to support the tags feature at the query level please?
It works at the top level :
[inputs.sql.tags]
environment="dev"

however it doesn't work at the query level :
[inputs.sql.query.tags]
environment="dev"

This would be really helpful as I can have each query tagging its data as appropriate under the same connection.

Thanks,
Paul

@srebhan
Copy link
Member Author

srebhan commented May 20, 2021

@aslgithub nice to hear it's at least not failing completely! :-)
TBH we do not filter the DSN in this PR. Can you please double check your DSN with the driver documentation.
Regarding the tags, I would like to add this in another PR (you are welcome to beat me at it :-)) once this is in. Complicating the code will make it only harder for the reviewers. Hope that's ok for you!?

@aslgithub
Copy link

@srebhan
Hi Sven,
Certainly not failing completely, this build has now been running for 12 days and has collected 227,249 data points from a Microsoft SQL Database using 2 inputs.sql.query sections.

Sorry, I'm going to admit doing a PR to add the tag would be beyond me appreciate your support in doing this.

Thanks,
Paul

docs/SQL_DRIVERS_INPUT.md Outdated Show resolved Hide resolved
@reimda reimda merged commit 908ad2f into influxdata:master Jun 15, 2021
@srebhan
Copy link
Member Author

srebhan commented Jun 15, 2021

@aslgithub give master a test if you can... Thanks to @reimda this plugin will (hopefully) make it into 1.19

@srebhan srebhan mentioned this pull request Jun 16, 2021
3 tasks
@aslgithub
Copy link

@srebhan I've downloaded the nightly build and it seems to be okay giving the same result as the previous artifact that I have been running on test. Thanks for your efforts on this.
@reimda Thank you so much for merging this :-)

reimda pushed a commit that referenced this pull request Jun 17, 2021
(cherry picked from commit 908ad2f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin 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.

Generic DB Query Plugin (DBI plugin)
7 participants