Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect both DBM active sessions and blocking sessions which are sleeping #14054

Merged

Conversation

justiniso
Copy link
Contributor

@justiniso justiniso commented Feb 28, 2023

What does this PR do?

This query update handles cases where the root blocker is a sleeping process (and thus is not collected) resulting in missing data from the UI.

image

Performance of the current query:
image

Performance of the new query:
TK

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@justiniso justiniso changed the title Collect both active sessions and blocking sessions which are sleeping Collect both DBM active sessions and blocking sessions which are sleeping Feb 28, 2023
@github-actions
Copy link

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

@@ -68,7 +73,8 @@
INNER JOIN sys.dm_exec_requests req
ON c.connection_id = req.connection_id
CROSS APPLY sys.dm_exec_sql_text(req.sql_handle) qt
WHERE sess.session_id != @@spid AND sess.status != 'sleeping'
WHERE sess.session_id != @@spid
AND (sess.status != 'sleeping' OR sess.session_id IN (SELECT blocking_session_id FROM cteBlocking) OR isnull(req.blocking_session_id, 0) <> 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we looked in the active transactions table this might be a tad bit faster (would be a smaller DMV table)

if a thing is blocking, in a sleeping state I believe it should always show up there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But would it be possible to have a blocking session not in a transaction? Not sure those would be functionally equivalent.

But I do believe in the idea that we should collect all processes with an active transaction; that seems like a better improvement overall.

Copy link
Contributor

@nenadnoveljic nenadnoveljic Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I do believe in the idea that we should collect all processes with an active transaction; that seems like a better improvement overall.

It would make sense to collect long lasting transactions, especially for inactive sessions - that's usually suspicious, regardless of whether the session is currently blocking anyone. We could implement it as a separate metric.

But would it be possible to have a blocking session not in a transaction?

Yes, in the isolation level where the reader is blocking writers. That's the default setting in on-prem SQL Server.

The new query wit cte looks suspiciously fat, especially if we decide to increase the sampling frequency one day. I addressed the issue of idle blocker for Oracle (https://docs.google.com/document/d/1NjAwb_Mui0FGmyxf5k54AZO-_TVoH6Sm4Vef9SIHvhg/edit?pli=1) and proposed an alternative approach - to extract the information from previous samples, if we captured it once it was active. Haven't checked yet if that's feasible.

Copy link
Contributor

@nenadnoveljic nenadnoveljic Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A subquery to capture idle blockers could slow down the query and might be an impediment for increasing the sampling frequency. If we assume that in most samples we won't find any blockers a better approach could be: while scanning the result set of only active sessions build a set containing blockers spids. In the end, execute a second query to collect only the most basic information on idle blockers - as they aren't executing anything we could probably omit all the joins. On the downside, this would generate an additional round trip to the database in the case some blockers are found, but I think this is a good trade off, as we're optimizing for the most frequent scenario where there aren't any idle blockers.

@justiniso justiniso marked this pull request as ready for review March 3, 2023 15:38
@justiniso justiniso requested review from a team as code owners March 3, 2023 15:38
@justiniso justiniso force-pushed the justindd/sqlserver-collect-all-blocking-query-activity branch from 23aebdd to 2c49b63 Compare March 3, 2023 15:40
@github-actions
Copy link

github-actions bot commented May 4, 2023

Test Results

     13 files       13 suites   1h 25m 41s ⏱️
   220 tests    209 ✔️   11 💤 0
2 000 runs  1 810 ✔️ 190 💤 0

Results for commit f1345cc.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #14054 (f1345cc) into master (6b25085) will increase coverage by 0.01%.
Report is 24 commits behind head on master.
The diff coverage is 100.00%.

Flag Coverage Δ
sqlserver 87.47% <100.00%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

nenadnoveljic
nenadnoveljic previously approved these changes Jun 5, 2023
@justiniso justiniso force-pushed the justindd/sqlserver-collect-all-blocking-query-activity branch from 4d25f64 to ba9a8b3 Compare June 28, 2023 15:05
Copy link
Contributor Author

@justiniso justiniso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me some time to review. I think this revised approach presents some maintainability problems I'd prefer to avoid if possible. I suggested an alternative, let me know what you think!

sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
@lu-zhengda lu-zhengda requested a review from a team August 9, 2023 21:29
sqlserver/tests/test_activity.py Outdated Show resolved Hide resolved
sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
@lu-zhengda lu-zhengda requested a review from a team as a code owner August 10, 2023 19:27
@ghost ghost added the documentation label Aug 10, 2023
drichards-87
drichards-87 previously approved these changes Aug 10, 2023
Copy link
Contributor Author

@justiniso justiniso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! (I can't approve because I'm the one who originally opened the PR) ✅

sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
@lu-zhengda
Copy link
Contributor

Example of idle blocking session
image
image

sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
sqlserver/tests/test_activity.py Outdated Show resolved Hide resolved
sqlserver/datadog_checks/sqlserver/activity.py Outdated Show resolved Hide resolved
@lu-zhengda lu-zhengda merged commit 803ce1d into master Aug 15, 2023
28 checks passed
@lu-zhengda lu-zhengda deleted the justindd/sqlserver-collect-all-blocking-query-activity branch August 15, 2023 13:16
jmeunier28 added a commit that referenced this pull request Oct 5, 2023
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.

6 participants