[sqlserver] Group perf counter rows by counter name before dispatch - #24876
Draft
eric-weaver wants to merge 1 commit into
Draft
[sqlserver] Group perf counter rows by counter name before dispatch#24876eric-weaver wants to merge 1 commit into
eric-weaver wants to merge 1 commit into
Conversation
Every metric object scanned the whole sys.dm_os_performance_counters result set, and with autodiscovery both the metric count and the row count grow with the database count, so the dispatch cost grew with the square of the number of databases. Group the rows by counter name once per run instead. Also cache the counter type for counters that need no base counter, which was costing a round trip per counter per database on every metric list rebuild, and rebuild instance_per_type_metrics from scratch so counter names for metrics that are no longer collected stop being queried. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 092a017 | Docs | Datadog PR Page | Give us feedback! |
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
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?
Restructures how rows from
sys.dm_os_performance_countersare handed to metric objects, and fixes two related defects in the metric-list rebuild.SqlSimpleMetric.fetch_all_valuesnow strips the blank-padded name columns once and groups the rows into a dict keyed by counter name;fetch_metriclooks up its own counter's rows instead of scanning the entire result set. This matches the contractSqlFractionMetric.fetch_all_valuesalready uses in the same file. TheALL_INSTANCES,object_name, and first-matchbreaksemantics are unchanged, including the deliberate asymmetry where theALL_INSTANCESbranch does not apply theobject_namefilter.get_sql_counter_typenow caches the type of counters that need no base counter. The cache write previously sat inside the base-name branch, so the roughly 37 plainBULK_COUNT/LARGE_RAWCOUNTcounters were never cached. A base-requiring counter whose base lookup fails is still left uncached so a transient failure is retried rather than pinned for the process lifetime._make_metric_list_to_collectbuildsinstance_per_type_metricsinto a localdefaultdict(set)and assigns it at the end. It previously only ever added, so counter names from custom metrics that were removed stayed in the query'sIN (...)list forever. Building locally and swapping in also means a rebuild that raises midway cannot leave this mapping inconsistent withinstance_metrics.No metrics are added, removed, or renamed, and no tags change.
Motivation
With autodiscovery, both the number of metric objects (
40 + 8D) and the number of rows returned (one per database for the sevenSQLServer:Databasescounters) grow with the database countD, so dispatching every row past every metric object grew withDsquared.A microbenchmark over synthetic result sets padded to
nchar(128), added here assqlserver/tests/test_bench.pyand run on the same machine before and after:One caveat worth stating plainly: this is a large constant-factor win, not a change of complexity class. A per-database metric still walks its counter's rows until it finds its own database, so the residual cost is still quadratic in the database count, just with a roughly 40x smaller constant. Making it truly linear would mean keying the groups by instance name as well, which changes more of the matching logic and belongs in its own change.
The counter-type cache fix removes roughly
8D + 39round trips per metric-list rebuild (about 840 at 100 databases), each of which created and closed its own cursor.Note that
dd.sqlserver.operation.timewithoperation:simple_metricswill not show the dispatch win, and may tick slightly up:tracked_querywraps onlyfetch_all_values, and the grouping work moves into that region while the dispatch loop it replaces was never inside it. Judge this by total check run duration.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