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

SQL Server - PerformanceCounters - removed synthetic counters #8325

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

Trovalo
Copy link
Collaborator

@Trovalo Trovalo commented Oct 27, 2020

Required for all PRs:

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

This PR removes some synthetic performance counters from the sqlserver_performance_counters measurement.

We believe that those counters were added to create unavailable counters on older editions of SQL Server, as of now, some of those counters already exist even in SQL 2008. In any case, we don't really want to fetch performance counters that do not exist.

  • affects only the latest version of the queries
  • affect only the on-prem query, for Azure those synthetic counter never existed
  • The query has been simplified

Comment on lines 295 to 296
,@Columns AS nvarchar(MAX) = '
,@PivotColumns AS nvarchar(MAX) = '
Copy link
Contributor

Choose a reason for hiding this comment

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

looks unintentional

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I replaced some single quotes and that introduced that error, as it should be 2 single-quotes.
Anyway turns out those variables are no longer needed and can be removed, as there were only used in the shythetic counters section.

Thanks for spotting it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already made a commit to remove the variables

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.

Can I ask why the synthetic counters were removed? I don't really have any context on it, but someone might find the answer useful in the future.

@Trovalo
Copy link
Collaborator Author

Trovalo commented Oct 27, 2020

When we had a look at the code (me and @denzilribeiro), we had no idea about what those counters were doing there, so we concluded they were there in the first version of the plugin, to compensate for something (maybe specific) that was missing.

But since they just don't belong to SQL Server performance counters, It's probably better to remove, they might cause confusion as you can't find them in the SQL server dmv (sys.dm_os_performance_counters), also the "Workload Group" related counters are already there, in the default counters

here is part of the conversation
#8172 (comment)

@ssoroka ssoroka merged commit 1313f23 into influxdata:master Oct 27, 2020
@Trovalo Trovalo deleted the remove-synthetic-perf-counters branch October 28, 2020 08:55
ssoroka pushed a commit that referenced this pull request Oct 28, 2020
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.

2 participants