-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add: Debug info to Site Health Info screen #517
Conversation
* | ||
* @return array | ||
*/ | ||
function get_formatted_internal_connnections() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have duplicated code here (and the next function) and in the pull-ui.php
. I'm considering splitting it and move the logic to utils.php
.
@dinhtungdu some feedback/question from my testing:
|
@jeffpaul I fixed 1 and 2, 4. |
@dinhtungdu yes, we'll want to log how users have those settings configured. |
@jeffpaul thanks for the head up, this PR is ready for review again |
@dinhtungdu 1-3 look good. For 4, can we call out the |
@jeffpaul, Thanks for the feedback, I updated both internal and external connections data, see https://prnt.sc/rkhnbq. Please note that you need to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few very minor comments, but otherwise code looks good and it's working well locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@dinhtungdu are we able to detect the Distributor version for the External Connections and report that for those in the respective section? |
@jeffpaul With current codebase, no, but we can with a custom public REST API endpoint. |
While it would be nice to be able to confirm all Distributor versions are current/equal, I'm not sure whether the value there is high enough to warrant a public API... @dkotter what's your take? |
@jeffpaul That's my concern, too. |
@jeffpaul @dinhtungdu This is something that was added as part of the Auth Setup Wizard PR (see https://github.com/10up/distributor/pull/368/files#diff-923616c19cf0f150ddc8b464ca53da19R257). So if that stays in (and we get that merged at some point) we will be able to utilize that here as well. |
@dinhtungdu let's wait on this PR until #368 gets merged and you can build on top of the API then. |
Added version to debug info. But the current solution may not scale if the site has many connections. I thought about caching or saving version info to What do you think @dkotter ? |
Unless we have alternatives that clearly scale better, I'd say we get this merged in to benefit anyone triaging Distributor setups and then work to also include the Distributor version as a column in the External Connections list table (as previously suggested by @dinhtungdu). |
@dinhtungdu @jeffpaul Yeah, not sure there's a great answer here. Ideally, this health info screen isn't accessed very often, so not sure how much of a concern this will be in reality. But for sites that have hundreds of external connections, this will add a significant overhead to that page. We could take the same approach we did for the Push UI, where we don't load anything on page load, only when the menu is hovered into. So instead of querying for the version information when the health info screen is loaded, we do it after page load, so it doesn't hurt performance. Not sure if that's worth the effort here or not but caching won't help a ton, as the data won't be cached until the page is visited (and again, hopefully this page isn't visited often) and as mentioned, saving to meta can become out-of-date. |
@jeffpaul After considering some options, I incline to merge this PR. I added a filter to disable the debug info as well. |
Description of the Change
Add debug information of Distributor to Site Health Info screen.
Alternate Designs
n/a
Benefits
Possible Drawbacks
n/a
Verification Process
Go to Tools > Site Health > Info and check the Distributor section.
Checklist:
Applicable Issues
Relates to #407
Changelog Entry
Added: Site Health debug information.