Feature/Debug Enhancement #506
Conversation
|
Curious why we have some stuff being removed like a filter doc, but also the 3rd parameter for |
|
@tw2113 I'm going to push up some fixes to this PR, I initially added a patch but that might have messed things up because it was a couple versions behind. Will make this to draft for now until I get things fixed. |
…ith public getter added for reindex batch size
…ment for common hosts
|
Will be interesting to see if/how this could get separated out per-site with Pro's network-wide indexing. Visually it feels like it's just looking at one specific site at a time. I could be wrong on that part though. |
@tw2113 Yup, you’re right—right now it’s scoped per-site for the free plugin. If we want full network-wide control and visibility, that’s something we can dig into for a future iteration. I’m pretty sure it’s doable, though it’ll take some bigger changes to the current debug page. That page is already accessible to network admins, but for now it only shows data for the main site—indices, settings, and logs. The foundation’s there though—we’d just need to tap into multisite-specific functions to take it further. |
| $this->reindexing = false; | ||
|
|
||
| // Add a delay between batches to avoid rate limiting. | ||
| sleep( 5 ); // Gentle: 5 second delay. |
There was a problem hiding this comment.
Not completely sure about this one as well.
We got bit in the butt from similar changes in version 2.6.0:
* Added: Wait for delete operations to complete before moving to updates.
This changed the indexing from asynchronous to synchronous, and we had reports of slowed down performance for people who didn't need it.
There was a problem hiding this comment.
@tw2113 I actually don’t remember that issue in 2.6.0, but good to know—thanks for the heads up.
The sleep(5) here is just a lightweight way to slow things down a bit between batches so we don’t slam into Algolia’s rate limits, like we did on CT. It’s not switching the flow to fully synchronous or anything like that—just a gentle pause between chunks.
That said, I’m open to other approaches if this feels too risky. Also open to making this configurable or rethinking it if it ends up causing more harm than good.
There was a problem hiding this comment.
Is there a lower sleep amount that can still help with avoiding rate limiting. If I'm mathing correctly, if a site has say 400 batches to process, with a new 5sec delay between starting the next, that'd equal out to 30min longer to process everything.
There was a problem hiding this comment.
I’d like to keep the default at 5 seconds for now, mainly as a conservative safeguard against Algolia’s rate limits—especially for users on shared hosting or with lower API quotas. While it does add up on very large sites, for most users (with smaller or medium-sized sites), the extra time per batch is negligible compared to the risk of hitting rate limits and having indexing fail or stall.
The 5-second pause is a “gentle” default that errs on the side of reliability and supportability, which is especially important for less technical users who may not know how to adjust filters or troubleshoot API errors. If we get feedback that this is too slow for many users, or if we see it’s a pain point in practice, we can always revisit and lower it, or make it more easily configurable.
There was a problem hiding this comment.
at bare minimum, if we could get a filter for this, that'd be best, though I'm still anxious about this amount of gap between moving to the next, because we do have a more than a handful amount of users with decent sized websites.
| $this->slug, | ||
| [ $this, 'display_page' ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
This one has me going "hmm" because from a plugin standpoint, should free/"base" plugins be intentionally aware of addons that extend it? or should they be "dumb" but built in ways that allow for extending unbeknownst to it's own code?
There was a problem hiding this comment.
@tw2113 The current setup is intentionally “dumb” in the best-practice sense—it doesn’t have any logic that’s aware of addons. It just registers its own admin pages and handles its own debug view.
Long-term, the idea is to keep the base plugin clean and maintainable, while making it easy to extend via hooks or filters. So yeah, it’s not trying to be smart—just flexible.
There was a problem hiding this comment.
I have to assume it'd just silently fail, but the reference to wpswa_pro_network is the spot where we're "smartening it up" with one line of code.
| <tr> | ||
| <th><?php esc_html_e( 'Search API Key', 'wp-search-with-algolia' ); ?></th> | ||
| <td><?php echo esc_html( get_option( 'algolia_search_api_key' ) ? get_option( 'algolia_search_api_key' ) : 'N/A' ); ?></td> | ||
| </tr> |
There was a problem hiding this comment.
for the options, we'd also want to keep in mind that things could be saved as available PHP constants and also on the topic of Pro, they could be network-based constants OR network options.
There was a problem hiding this comment.
@tw2113 Yep, exactly. This part is just echoing out whatever’s already been saved. It’s not setting or overriding anything, and the plugin already respects constants and network options where defined. This debug view is purely for visibility, showing whatever get_option() returns, regardless of where the value originated.
There was a problem hiding this comment.
For a proverbial "version one" this would cover one possible source, but ideally once more "matured", it'd be checking for all potential spots.
|
Overall, I'm trying to mentally draw the line between what parts of this are needed specifically for CT, vs what could be useful for the rest of our users. Yes, I do want to get more information surfaced for when issues occur, so that I can help with our support requests and get everyone's content indexed, accurately, as quick as possible. But I also want to have that done in the best possible way at scale.
Reading this has me wondering if we should initially treat as an addon or hold off on merging into anything until we're ready for it to work in all the possible user scenarios. For example in the free plugin, we could have it coded in such a way where the debug page shows details for whichever site is being seen, with multisite, or if just a single site, that one is obvious. Then for Pro's network-wide indexing, in some way extend a base "debugging" class from Free, and have it get wired up for that feature. That extending code could reside in Pro, leaving Free "dumb" to it, like I mention in one of my inline comments. |
|
@khleomix Curious about your thoughts on making this a separate plugin vs rolled into the free version? |
|
Thanks both! @tw2113 Thanks for the detailed feedback and for thinking through the broader use cases. I agree it makes sense to keep the free plugin’s debug functionality scoped per site for now. That covers the majority of use cases and keeps things simple and maintainable. The current behavior, showing debug info based on the active site context (main site or subsite), should feel intuitive for most users. For network-wide indexing and visibility, as needed in Pro, I’m fully on board with extending a base Debugging class from the free plugin. That keeps the base plugin dumb and focused, while giving Pro the flexibility to layer in more advanced multisite-aware features without adding complexity to the core. This gives us room to iterate on those network-wide features separately and only bring them into the free plugin later if it makes sense for the broader user base. Right now, the base plugin is doing exactly what it should—staying lean, focused, and extensible. @ramiwds As for whether this should live in the free plugin or be a separate addon, I’d say it depends on scope. If the debug functionality is lightweight and broadly useful, it makes sense to include it in the free version. But if we’re adding more advanced features like network-wide or multisite support that are specific to Pro use cases, it’s probably better to keep those in an addon or in Pro itself. That way, the free plugin stays lean and focused while still giving us room to grow. So with all that in mind, I think this is ready to merge as-is. It keeps the base plugin focused, useful, and extensible, and leaves us plenty of room to grow. |
|
Thoughts on gating this behind say |
|
Before we do any merging at all, I think we need to break this up a bit, as we're doing a small handful of things.
Regarding the debugging page, I believe it actually will be pulling from whatever site it's being viewed in. It's not isolated to the main site. Technically less work in the future, but something to keep in mind. I think anything around the debugging should be its own isolated PR. The changes around indexing process, should be perhaps part of https://github.com/WebDevStudios/wp-search-with-algolia/milestone/25 The PHPCS changes can be part of https://github.com/WebDevStudios/wp-search-with-algolia/milestone/28 |
|
@tw2113 Totally makes sense to split this up so things are clearer and easier to track. That said, I think the PHPCS fixes are worth pushing through sooner rather than later. They’re pretty straightforward and help clean up the noise in local dev and CI. Doesn’t feel like they need to wait on the other pieces. Happy to move those into a separate PR if that works better. |
|
Agreed. I've plucked AJAX responses already at https://github.com/WebDevStudios/wp-search-with-algolia/pull/510/files and we can definitely do so with the PHPCS items and an associated PR to #496 |
* various PHPCS fixes fetched from #506 * allow for overriding of early return for FSE based themes. (#505) * allow for overriding of early return for FSE based themes. * new compatibility method for block based themes * rename filter to read block based * fill in intended release number, add early return if already true * update changelog file * update readme changelog * update versions * last array item * alignment * tabs * missed method param doc * array alignment
Closes
N/A
DESCRIPTION
algolia_indexing_batch_sizefilter as usage and performance are monitored.SCREENSHOTS
OTHER
STEPS TO VERIFY
wp-config.php:define('WP_DEBUG', true);