Skip to content

SQL Server - PerformanceCounters - removed synthetic counters#8325

Merged
ssoroka merged 2 commits intoinfluxdata:masterfrom
Trovalo:remove-synthetic-perf-counters
Oct 27, 2020
Merged

SQL Server - PerformanceCounters - removed synthetic counters#8325
ssoroka merged 2 commits intoinfluxdata:masterfrom
Trovalo:remove-synthetic-perf-counters

Conversation

@Trovalo
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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