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

Filter data out from system databases for Azure SQL DB only #8849

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

masree
Copy link
Contributor

@masree masree commented Feb 12, 2021

Azure SQL DB has few system databases like msdb, model .... which emits data and are not useful for customers for monitoring. However, we're collecting data from telegraf queries and seems unnecessary as this data does not make sense for single DB customers. Excluding these system databases from performance DMVs to lower the amount of data emitted.

Please enter the commit message for your changes. Lines starting

Required for all PRs:

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

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!

Copy link
Contributor

@denzilribeiro denzilribeiro left a comment

Choose a reason for hiding this comment

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

Makes sense to remove model/replicated databases as this is SOLELY for the single Azure DB collector, leave master and tempdb though

plugins/inputs/sqlserver/azuresqlqueries.go Outdated Show resolved Hide resolved
@masree masree force-pushed the lowerDataVolume branch 2 times, most recently from a92bc7f to 5fd2726 Compare February 13, 2021 01:16
@masree
Copy link
Contributor Author

masree commented Feb 23, 2021

@ssoroka / @sjwang90 / @srebhan - Can you help review and merge if no concerns?

@srebhan
Copy link
Contributor

srebhan commented Feb 23, 2021

Hmmm as the discussion shows finding a consent on what ppl want is difficult. How about make this exclude list a config option that is, by default, set to the list you want to exclude? People really wanting to monitor that DBs for whatever reason are then free to do so.

@srebhan srebhan self-assigned this Feb 23, 2021
@masree
Copy link
Contributor Author

masree commented Feb 24, 2021

Hmmm as the discussion shows finding a consent on what ppl want is difficult. How about make this exclude list a config option that is, by default, set to the list you want to exclude? People really wanting to monitor that DBs for whatever reason are then free to do so.

Thank you for the suggestion. In Azure SQL database offering, Azure SQL manages system databases(modelDB, msdb ..) and customer will likely not find any useful information for monitoring in these system databases that are currently filtered out. We think the filtered list of system databases are standard and not useful for monitoring. Even if we make it as an option in config for customers to exclude, most/all customers might always keep the default config as including system databases just dumps more data without actionable results. I agree it gives flexibility, but I think the config option might not be used to include other system databases as the data does not make sense for Azure SQL offering. We wanted to keep it simple by excluding them in the query, but if you think it is a concern, we can follow your suggestion. Let us know what you think. Thanks!

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Ok I now understand the use-case and I'm fine with the change. However, please keep in mind that if you ever want to touch this again to make it a config option! ;-)

@srebhan srebhan added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor labels Mar 1, 2021
@ssoroka
Copy link
Contributor

ssoroka commented Mar 2, 2021

looks like there's conflicts to resolve. Also is it at all possible that this data is useful to some customers, and we should instead parameterize it?

@denzilribeiro
Copy link
Contributor

looks like there's conflicts to resolve. Also is it at all possible that this data is useful to some customers, and we should instead parameterize it?

I don't think there is conflict - the change is only for "single database" offering of Azure , not for SQL Managed instance ( multiple DBs') or on-prem SQL Server ( multiple-DB's). I think you risk over parameterizing things too, there is no one that will get any benefit looking at these couners for master/model/MSDB databases. If you make it a config and then make the default as what backward compat was, then you remove usefulness of it, if we parameterize then would want default for param to be to exclude system Dbs not the other way around. I personally think no need of parameterization

@srebhan
Copy link
Contributor

srebhan commented Mar 3, 2021

@denzilribeiro I think @ssoroka is referring to the merge conflict in plugins/inputs/sqlserver/azuresqlqueries.go that needs to be resolved...

@masree
Copy link
Contributor Author

masree commented Mar 8, 2021

@denzilribeiro I think @ssoroka is referring to the merge conflict in plugins/inputs/sqlserver/azuresqlqueries.go that needs to be resolved...

@ssoroka I'll work on resolving the merge conflict and push a new iteration. Thanks everyone for the review!

relevant for monitoring in Azure SQL

Please enter the commit message for your changes. Lines starting
@masree
Copy link
Contributor Author

masree commented Mar 9, 2021

Fetched the latest from upstream and resolved the conflict.

@masree
Copy link
Contributor Author

masree commented Mar 9, 2021

@denzilribeiro I think @ssoroka is referring to the merge conflict in plugins/inputs/sqlserver/azuresqlqueries.go that needs to be resolved...

@ssoroka I'll work on resolving the merge conflict and push a new iteration. Thanks everyone for the review!
I've resolved the conflict!

@sspaink sspaink merged commit 35b75e9 into influxdata:master Mar 11, 2021
ssoroka pushed a commit that referenced this pull request Mar 17, 2021
* Excluding data from system databases like msdb,model which are not
relevant for monitoring in Azure SQL

Please enter the commit message for your changes. Lines starting

* Addressing review comments to handle null scenarios

(cherry picked from commit 35b75e9)
jblesener pushed a commit to jblesener/telegraf that referenced this pull request Apr 18, 2021
…ta#8849)

* Excluding data from system databases like msdb,model which are not
relevant for monitoring in Azure SQL

Please enter the commit message for your changes. Lines starting

* Addressing review comments to handle null scenarios
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
…ta#8849)

* Excluding data from system databases like msdb,model which are not
relevant for monitoring in Azure SQL

Please enter the commit message for your changes. Lines starting

* Addressing review comments to handle null scenarios

(cherry picked from commit 35b75e9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants