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

SqlServer - Fixed Query mapping #8200

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

Trovalo
Copy link
Collaborator

@Trovalo Trovalo commented Sep 30, 2020

Required for all PRs:

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

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

  • AzureSQLDBOsWaitstats
  • AzureSQLMIRequests

@denzilribeiro

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.

Just readme could have been changed correct yep was a Readme typo

@Trovalo
Copy link
Collaborator Author

Trovalo commented Sep 30, 2020

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)

@reimda
Copy link
Contributor

reimda commented Sep 30, 2020

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?

@Trovalo
Copy link
Collaborator Author

Trovalo commented Oct 1, 2020

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.
As of now, I see no problems in fixing and editing this kind of behavior.

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:

const sampleConfig = `

@Trovalo
Copy link
Collaborator Author

Trovalo commented Oct 7, 2020

What do we want to do in the end? @denzilribeiro @reimda

To me the PR is ok as it is but let me know

@denzilribeiro
Copy link
Contributor

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

@Trovalo
Copy link
Collaborator Author

Trovalo commented Oct 7, 2020

@ssoroka can you merge this?

@reimda reimda merged commit 88698a6 into influxdata:master Oct 7, 2020
@Trovalo Trovalo deleted the SQL-Server---Query-mapping-fix branch October 8, 2020 08:52
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants