Skip to content

Isolate SQL Server plan lookup failures - #25091

Draft
jasonmp85 wants to merge 3 commits into
masterfrom
jmp/sqlserver-plan-lookup-isolation
Draft

Isolate SQL Server plan lookup failures#25091
jasonmp85 wants to merge 3 commits into
masterfrom
jmp/sqlserver-plan-lookup-isolation

Conversation

@jasonmp85

Copy link
Copy Markdown
Contributor

What does this PR do?

Isolates each SQL Server execution-plan lookup so a timeout or other lookup error only skips that row. Customers now see a debug log and a dd.sqlserver.statements.error count tagged with the exception class instead of a query-metrics job-loop crash; collection continues with the remaining plan rows.

The plan rate limiter is acquired only after a successful lookup, so a failed plan is eligible for collection again on the next pass instead of being suppressed for the cache TTL. Cancellation is rechecked after lookup errors and still aborts collection.

The query-metrics job now uses the established SQL Server expected database exception set: SQLConnectionError, pyodbc.Error, and adodbapi.DatabaseError. Database errors that escape elsewhere are therefore reported as expected database warnings rather than crashes.

Validation: ddev test --lint sqlserver passed, and all unit tests in tests/test_statements.py passed in every compatible configured environment. The requested unfiltered ddev --no-interactive test sqlserver command was also attempted, but integration tests require SQL Server services that are not running in this worktree environment.

Motivation

On busy SQL Server instances, PLAN_LOOKUP_QUERY can time out. A single timeout currently escapes plan collection, terminates the query-metrics job loop until the next check run, and marks the failed plan as seen even though no plan event was collected. Per-row isolation preserves job liveness and makes the failed plan retryable.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add qa/required if this PR needs QA validation, or qa/skip-qa if it does not. Exactly one of the two is required.
  • If you need to backport this PR to another branch, you can add the backport/ label to the PR and it will automatically open a backport PR once this one is merged

@jasonmp85 jasonmp85 added the qa/required QA is required for this PR and will generate a QA card label Sep 3, 2026
@dd-octo-sts dd-octo-sts Bot added integration/sqlserver and removed qa/required QA is required for this PR and will generate a QA card labels Sep 3, 2026
@jasonmp85 jasonmp85 added the qa/required QA is required for this PR and will generate a QA card label Sep 3, 2026
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Sep 3, 2026

Copy link
Copy Markdown

evalya-impact-summary

evalya impact analysis
Impact analysis: 0 selected, 0 skipped (of 0 test tasks)
Publish tasks:   2 (always emitted)
Diff (8 files):
  datadog_checks_base/changelog.d/25091.added
  datadog_checks_base/datadog_checks/base/utils/db/utils.py
  datadog_checks_base/tests/base/utils/db/test_util.py
  sqlserver/changelog.d/25091.fixed
  sqlserver/datadog_checks/sqlserver/connection_errors.py
  sqlserver/datadog_checks/sqlserver/data_observability.py
  sqlserver/datadog_checks/sqlserver/statements.py
  sqlserver/tests/test_statements.py

Debug a specific task: evalya plan impact --path <path> --task <task>

Learn more about CI impact filtering

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Tests  Code Coverage

All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 90.77%
Overall Coverage: 88.99% (+0.10%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 529fdaa | Docs | View more details | Give us feedback!

Two review points from the first pass.

The plan-lookup guard reimplemented RateLimitingTTLCache.acquire's admission
rules at the call site, reaching into `maxsize` and `__contains__`, so the
policy now lived in two places. Give the cache a `would_acquire` method that
answers the question without storing the key, and have acquire share it.

The expected-database-exception set was defined identically in statements.py
and data_observability.py. Move it next to SQLConnectionError in
connection_errors.py, which already imports both drivers, and have both jobs
use it.
@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Validation Report

All 21 validations passed.

Show details
Validation Description Status
agent-reqs Verify check versions match the Agent requirements file
ci Validate CI configuration and code coverage settings
codeowners Validate every integration has a CODEOWNERS entry
config Validate default configuration files against spec.yaml
dep Verify dependency pins are consistent and Agent-compatible
http Validate integrations use the HTTP wrapper correctly
imports Validate check imports do not use deprecated modules
integration-style Validate check code style conventions
jmx-metrics Validate JMX metrics definition files and config
labeler Validate PR labeler config matches integration directories
legacy-signature Validate no integration uses the legacy Agent check signature
license-headers Validate Python files have proper license headers
licenses Validate third-party license attribution list
metadata Validate metadata.csv metric definitions
models Validate configuration data models match spec.yaml
openmetrics Validate OpenMetrics integrations disable the metric limit
package Validate Python package metadata and naming
qa-label Validate the pull request declares whether it needs QA for the next Agent release
readmes Validate README files have required sections
saved-views Validate saved view JSON file structure and fields
version Validate version consistency between package and changelog

View full run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant