Restrict SQL Server instance-level database metrics to autodiscovered databases - #24952
Restrict SQL Server instance-level database metrics to autodiscovered databases#24952jasonmp85 wants to merge 6 commits into
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b3d69c9 | Docs | View more details | Give us feedback! |
|
|
evalya-impact-summaryevalya impact analysis |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09461d1c41
ℹ️ 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".
|
OK, so there was this Unfortunately, queries which implicitly include all databases would not honor this at all (queries that aren't per-DB, but make one call and get all-DB information). So customers who set this before wouldn't be getting certain metrics for non-matching DBs, but would be getting others. With this fix, the A customer might be implicitly depending upon the broken behavior. But we're "fixing" the stated behavior. So is this a bug fix or a breaking change (in the CHANGELOG)? |
e301762 to
72ddef1
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ffa03e4 to
ceb5e70
Compare
Review from sethsamuel is dismissed. Related teams and files:
- database-monitoring-agent
- sqlserver/changelog.d/24952.fixed
- sqlserver/datadog_checks/sqlserver/database_metrics/base.py
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Restricts instance-level database metric collectors to the databases selected by autodiscovery. A shared
_database_filterhelper adds anIN (...)predicate to the queries behind file stats, database stats, and database backup metrics when a database list is configured, and leaves the query unchanged when it is not.Measured effect
Bench instance with 3,945 databases on the server and
autodiscovery_includenaming five of them. Per-collector attribution, same configuration on both sides:Total check time 918.8 ms → 132.3 ms (6.9x), and roughly 102,600 samples for unmonitored databases are no longer emitted. File stats alone accounted for 65% of the cost.
The sample counts on the right are what the configuration actually asked for: five monitored databases and their files.
Motivation
autodiscovery_includecurrently bounds the database-level collectors but not the instance-level ones, so naming a handful of databases does not stop the check from reporting on every database on the server. A user monitoring five databases on a shared instance pays for all 3,945 and receives metrics for databases they did not ask about.Note for reviewers: this changes what the check emits. Users who currently rely on instance-level metrics for databases outside their autodiscovery filter would stop receiving them. I have filed the changelog entry as
changedon that basis, but would appreciate a maintainer's read on whetherfixedis the better call, since the current behavior contradicts the documented meaning of the option.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