search

BrentOzarULTD / SQL-Server-First-Responder-Kit

sp_Blitz, sp_BlitzCache, sp_BlitzFirst, sp_BlitzIndex, and other SQL Server scripts for health checks and performance tuning.
http://FirstResponderKit.org
Other
3.38k stars 999 forks source link

sp_Blitz: Add more Query Store best practice checks. #3527

Closed ReeceGoding closed 5 months ago

ReeceGoding commented 6 months ago

Is your feature request related to a problem? Please describe. I'm always frustrated when sp_Blitz tells me that I have trace flag 3226 on and gives me a lovely description of it, but gives no lovely description for either of the trace flags that are considered best practice for Query Store (7745 and 7752). This got me thinking that we should do some more general checks for Query Store best practice.

Describe the solution you'd like sp_Blitz should warn in all of the following scenarios:

  1. Query Store enabled. but TF 7745 is off.
  2. Query Store enabled on a pre-2019 box, but TF 7752 is off.
  3. Query Store enabled on a 2019+ box, but TF 7752 is on. I might be mistaken, but I think that we can tell if an enabled-by-default trace flag has been deliberately turned on? If so, that means that a 2019+ box with TF 7752 enabled is worrying. It probably means that somebody did an upgrade and forgot to clear it from the list of startup parameters.
  4. TF 7745 or TF 7752 enabled on a box that doesn't support Query Store.
  5. Query Store enabled, but Wait Stats Capture Mode disabled despite the box supporting it.
  6. Query Capture Mode in an state other than AUTO. ALL is usually a stupid idea and, much like how we warn for RCSI being enabled, we should probably warn if the mode is CUSTOM because that probably means that somebody is doing some interesting non-default stuff.
  7. Query Store not in ReadWrite state.
  8. Maybe warn if Query Store is eating up a silly amount of RAM? I'm not sure what "silly" means. 1 GB?

In addition, the generic warning that TF 7745 or TF 7752 are on should be specific and say what they actually do. Maybe link to Erin?

Describe alternatives you've considered Leaving it alone comes to mind. dbatools has beaten us to it.

Are you ready to build the code for the feature? I sincerely can't decide.

BrentOzar commented 6 months ago

Everything you're describing here makes sense. If you go for it, also update the Documentation/sp_Blitz_Checks_by_Priority.md file to add your new check IDs there.

For URLs for warnings, you definitely don't wanna link to BrentOzar.com stuff because we just don't have any kind of good guidance on there. Feel free to link to whatever URL makes the most sense for each one of the warnings.

ReeceGoding commented 5 months ago

Items 1-2 addressed in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3539 Items 3-4 addressed in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3531 Item 5 addressed in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3535 Item 6 addressed in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3541 Item 7 addressed in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3536 Item 8 cancelled in https://github.com/BrentOzarULTD/SQL-Server-First-Responder-Kit/pull/3533

BrentOzar commented 5 months ago

I'm going to close this for now - going forward, let's open one issue and one corresponding pull request per change we want to make, keeping 'em self-contained so we can test & merge things easier. Thanks!