[sqlserver] Share one performance counter query and index rows by instance - #24908
Open
eric-weaver wants to merge 5 commits into
Open
[sqlserver] Share one performance counter query and index rows by instance#24908eric-weaver wants to merge 5 commits into
eric-weaver wants to merge 5 commits into
Conversation
…tance The three metric classes that read sys.dm_os_performance_counters each issued their own query, and scanning that table costs about the same whether it returns two rows or thousands: at 1000 autodiscovered databases the two fraction queries spent 110ms between them to return four rows. They now share a single fetch over the union of their counter names. Grouping rows by counter name left the dispatch quadratic, because every per-database metric still scanned the rows of its own counter looking for its instance. Indexing by instance name as well turns that scan into a lookup, which takes the dispatch from 234ms to 3ms at 1000 databases. Together these cut the steady-state check from 2938ms to 2423ms at 1000 databases, measured on the same instance, and the emitted series and tag sets are unchanged. A fraction metric whose base counter is missing for its instance is now skipped rather than raising, which previously abandoned the remaining metrics for that run. Co-authored-by: Cursor <cursoragent@cursor.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 8b800c9 | Docs | View more details | Give us feedback! |
Co-authored-by: Cursor <cursoragent@cursor.com>
evalya-impact-summaryevalya impact analysis |
The tag depends only on the instance name, so constructing it in the row loop put it a level below where it belongs. The rows of one counter and instance are almost always a single row, so this is for the reader rather than for speed. Co-authored-by: Cursor <cursoragent@cursor.com>
…sertions The three tracked operations became one, so the integration tests asserting a dd.sqlserver.operation.time series per operation name were looking for tags no longer emitted. Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f5fac9d1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Names the nested index the dispatch reads from, so the shape is stated once rather than inferred from the loops that walk it. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
The three metric classes that read
sys.dm_os_performance_countersnow share one query and one snapshot of the table, and the rows are indexed by instance name as well as counter name so each metric looks its rows up instead of scanning for them.A fraction metric whose base counter is missing for its instance is now skipped rather than raising, which previously abandoned the remaining metrics for that run.
Motivation
Follow-up to #24876. Scanning that table costs about the same whether it returns two rows or thousands, so a query per metric class is overhead: at 1000 autodiscovered databases the two fraction queries spent 110ms between them to return four rows each. Grouping by counter name alone also left the dispatch quadratic, because every per-database metric still walked its counter's rows looking for its own instance; indexing by instance takes that from 234ms to 3ms.
At 1000 autodiscovered databases with DBM enabled, the steady-state run drops from 2938ms to 2423ms (medians of six runs on the same instance).
Emitted data is unchanged: at 100 databases, with an
instance_name: ALLcustom metric configured, anagent check --jsoncapture emits an identical set of 5797 (metric, type, tag set) series.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged