-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
SqlServer - Fixed Query mapping #8200
SqlServer - Fixed Query mapping #8200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just readme could have been changed correct yep was a Readme typo
Yes, but I was not sure about it, so I've applied the "standard" naming in a strict way (Prefix + QueryName) |
Changing both doc and code to AzureSQLMIRequests may be more standard, but if anyone was using AzureSQLMISqlRequests in their config before in include_query or exclude_query, this change will break their config and require them to figure out what the new value is. After a quick read of the filter code, I think if it's in include_query it won't be included as before and if it's in exclude_query it won't be excluded as before, and there won't be any warning to the user that the behavior is opposite. Why not just change the doc to AzureSQLMISqlRequests and preserve backward compatibility? |
That's because this new set of features (query splitted by version) has not reached any released version of Telegraf yet, if someone is using it, then they are using custom builds or nightly versions. About backward compatibility, the old queries are still there and will keep working unedited. That "old" version is still the default configuration proposed by the telegraf executable, see:
|
What do we want to do in the end? @denzilribeiro @reimda To me the PR is ok as it is but let me know |
This is a fairly new change so am ok with it, if it was later on that would be different |
@ssoroka can you merge this? |
Required for all PRs:
For reasons I got to test the Managed instance section of the queries, and I've found a mismatch between the available queries listed in the README.md and what was actually mapped in the code.
This means that those queries will never be called if the name used is the one specified in the readme
A mismatch was also discovered for Azure SQL DB.
Those are the queries that have been fixed
@denzilribeiro