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

sqlserver - Changed requests to include blocking sessions. #7834

Closed
wants to merge 2 commits into from
Closed

sqlserver - Changed requests to include blocking sessions. #7834

wants to merge 2 commits into from

Conversation

spaghettidba
Copy link
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Changed query to gather requests to also include blocking sessions. The original query started from sys.dm_exec_requests, which excludes all idle queries (which have a session but not a request).
Also fixed query text truncation of the last character.

@danielnelson
Copy link
Contributor

Thanks for the pull request, but could you update the pr to minimize any whitespace changes? This will make the changes to the query a bit easier to understand and improve the history of the file.

Copy link
Contributor

@denzilribeiro denzilribeiro left a comment

Choose a reason for hiding this comment

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

I totally agree with the change in that Sessions left outer join requests
What I don't agree is getting Session CPU time/reads/writes etc... I think it should be a coalesce
https://docs.microsoft.com/en-us/sql/t-sql/language-elements/coalesce-transact-sql?view=sql-server-ver15
If the reqest is not null, you want to get everything from the request ( CPU/IO/Transaction etc). if a session doesn't have a request then get it from the session

@spaghettidba
Copy link
Contributor Author

@denzilribeiro No problem, I'll change it to a COALESCE.

@danielnelson I certainly could reduce the number of whitespace changes, but the query is formatted in a way that makes it a bit hard to read. How do you suggest I can improve it? Should I open a second PR? Wouldn't that "clutter" the history of the file anyway? Should I avoid changing formatting at all?

@spaghettidba
Copy link
Contributor Author

Hi! Sorry to bother. Will this PR be merged now that it's approved? Do I need to do anything else?

@Trovalo
Copy link
Collaborator

Trovalo commented Aug 10, 2020

@ssoroka can you have a look at this?

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

It seems like coalescing the values from two different tables into a single column has the potential to provide really weird behavior. Would it be better to report these as separate columns instead?

@denzilribeiro
Copy link
Contributor

It seems like coalescing the values from two different tables into a single column has the potential to provide really weird behavior. Would it be better to report these as separate columns instead?

No that is how it should be actually. Don't merge this, I will take care of it under PR #7934 given the major change if that is ok with you and @Trovalo

@ssoroka
Copy link
Contributor

ssoroka commented Aug 10, 2020

Fine by me. I'll close this out, then.

@ssoroka ssoroka closed this Aug 10, 2020
denzilribeiro added a commit to denzilribeiro/telegraf that referenced this pull request Aug 10, 2020
denzilribeiro added a commit to denzilribeiro/telegraf that referenced this pull request Aug 10, 2020
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.

5 participants