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

Adding/fixing support for Azure SQL Database (singleton database) #6111

Merged
merged 8 commits into from
Jul 12, 2019
Merged

Adding/fixing support for Azure SQL Database (singleton database) #6111

merged 8 commits into from
Jul 12, 2019

Conversation

denzilribeiro
Copy link
Contributor

Required for all PRs:

As is the SQL Plugin doesn't lend itself to Azure SQL Database.

  • ServerName is a logical construct on Azure SQL Database, added Database name to all collectors to be able to filter

  • Added Resource governance collection for Managed instance and Azure SQL Database collected if the AzureDB flag is enabled in telegraf.conf

  • Added waitstats collection against sys.dm_db_wait_stats if the it is Azure SQL DB instead of server level waits.

  • Will Publish grafana reports specific to Azure SQL DB separately.

  • Signed CLA.

  • [ X] Associated README.md updated.

  • [ X] Has appropriate unit tests.

@danielnelson danielnelson added this to the 1.12.0 milestone Jul 11, 2019
@danielnelson danielnelson added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jul 11, 2019
@danielnelson
Copy link
Contributor

Looks good, would you be able to run the plugin once with --test against an instance, this will make it a bit easier for me to inspect the output since I'm not very familiar with the query language:

telegraf --input-filter sqlserver --test

@denzilribeiro
Copy link
Contributor Author

@danielnelson Here you go - output requested
testtelegraf.txt

@danielnelson
Copy link
Contributor

I don't see any sqlserver_scheduler measurements in the output, is this expected?

On sqlserver_azuredb_waitstats vs sqlserver_waitstats, consider using the same measurement name especially if they are subsets of each other. This could make comparing a mixed infrastructure easier and ease migration into Azure since you could use some of the same alerts and dashboards. One argument against this would be if they have fields with conflicting meanings or data types.

Let's rename dm_user_db_resource_governance and dm_instance_resource_governance to start with sqlserver, I think this will help with organization when you have lots of measurements.

Also, optional and just something for you to consider: these new measurements don't necessarily need to be named like the source table, it might be limiting if you decide to pull in other tables in the future. Think about if there should be a higher/lower concept that they represent.

@denzilribeiro
Copy link
Contributor Author

So yeah the sqlserver_scheduler one :) I had it disabled in my conf file when ran the test. Sounds good on the waitstats, they have the same measurements. Will make the rest of changes shortly

@denzilribeiro
Copy link
Contributor Author

I decided to leave sqlserver_azuredb_waitstats as is - the reason is if you mistakenly use a dashboard from on-prem against Azure, will get incorrect results as on Azure the server is just a logical construct and so will be adding up waits from different DB's potentially in one dashboard . On-prem the servername is the starting point - hope am making sense. All other changes made

@danielnelson
Copy link
Contributor

Wouldn't having a dashboard that groups by both sql_instance and the new database_name tag solve this?

@denzilribeiro
Copy link
Contributor Author

No it won't - Waits on databases aren't additive - aka for an normal SQL Server (on-prem or managened instance) you look at waits at the server level, not the database level as they are server level only so these are semantically different, for Azure SQL DB (single) you only look at a single database waits . Alternatively would have to make a change that stores the type of Database/instance in the config itself or for every measure so based on that dashboard can reflect appropriate thing as just sql_instance or not or can expose a parameter to filter for Azure SQL v/s not. The combination of sql_instance and database_name doesn't tell you the type of DB. necessarily - so will be a bigger change. If we thing would be better to add the SQLEngineType kinda tag to measures to help in future as well, let me know.

Copy link
Contributor

@m82labs m82labs left a comment

Choose a reason for hiding this comment

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

Changes look good. I don't have any Azure SQL DB instances to test against myself.

@danielnelson
Copy link
Contributor

@denzilribeiro Thanks for the detailed explanation, I do prefer having them as separate measurements to avoid unwanted aggregation. I dislike laying out the data so that one generally needs to exclude certain tags, i.e.: select * from foo where engine_type != "azure". It's fine sometimes, but shouldn't almost always be a requirement to query. That said storing the engine type or database version as a tag could still be a good feature to add in the future.

@danielnelson danielnelson merged commit 149be55 into influxdata:master Jul 12, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants