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

Add logging support to Playground #1035

Merged
merged 37 commits into from
Feb 29, 2024
Merged

Add logging support to Playground #1035

merged 37 commits into from
Feb 29, 2024

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Feb 13, 2024

What is this PR doing?

This PR adds support for collecting PHP logs and WordPress debug logs in Playground.
After each request logs are printed with console.debug in the browser.

Support for Node, WP-now, and VSCode will be added in separate PRs.
We also plan to add more Playground logs in the future to help us improve the stability of WordPress Playground.

What problem is it solving?

When Playground crashes it's hard to know why it happened. By adding access to PHP logs and users will be able to debug their sites.
Playground developers will benefit from having Playground and PHP logs in a single place and a shared format.

How is the problem addressed?

On request.end we collect logs from the current PHP request and send them back with the event.
These logs are caught by the Logger and printed.
Additionally, the logger catches some of the Playground errors using regular window events.

Testing Instructions

add_action('init', function () {
    if (isset($_GET['post'])) {
        throw new Exception('This is a test exception');
    }
});
  • Refresh Playground
  • Confirm that it loads
  • Go to a post edit page using wp-admin
  • Confirm that the post edit page crashes and the debug log is printed

@bgrgicak bgrgicak self-assigned this Feb 13, 2024
@bgrgicak
Copy link
Collaborator Author

@adamziel this is still missing proper Playground log collecting, but it will help debugging WordPress as-is.

The logger is now loaded in Blueprints because this is the first place I found that exposes UniversalPHP which is a requirement for collecting PHP logs. If it's moved to Website, it won't collect logs when Playground fails to load.

I wanted to keep the logger tied to a single package for this PR, but it would be great if we could move it to a separate package and call it in different packages as needed.

For example, we can't detect the register_shutdown_function error now, and I assume that we will need to dispatch an event from register-service-worker to catch it with the logger.

@bgrgicak bgrgicak marked this pull request as ready for review February 19, 2024 08:56
@bgrgicak
Copy link
Collaborator Author

The logger is now loaded in Blueprints because this is the first place I found that exposes UniversalPHP which is a requirement for collecting PHP logs. If it's moved to Website, it won't collect logs when Playground fails to load.

I guess we could move it to Remote.

@bgrgicak bgrgicak requested a review from adamziel February 19, 2024 09:55
@bgrgicak bgrgicak requested a review from adamziel February 26, 2024 14:59
/**
* Collect errors from JavaScript window events like error and log them.
*/
private collectJavaScriptErrors() {
Copy link
Collaborator

@adamziel adamziel Feb 26, 2024

Choose a reason for hiding this comment

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

It would be nice to make this pluggable, maybe as a Constructor argument? This way different runtimes could ship their own global error collecting logic, e.g. the VS Code extension will need highly customized logic that I don't think belongs to this repo. Also, there might be environments where the window object exists, and yet it either doesn't provide any useful error information or only provides Playground-unrelated noise – I vaguely remember VS Code might be one of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it necessary? What would the constructor argument do?
The logger should be accessible by all environments, so a runtime can submit logs and when we add full support it will also be able to read logs.

Copy link
Collaborator

@adamziel adamziel Feb 27, 2024

Choose a reason for hiding this comment

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

Is it necessary?

We can merge without that. I'd just eventually move the responsibility for selecting the logging method to the consumer of this API instead of guessing what the environment is.

On the website we'd call something like collectWindowErrors(), in web worker that could be collectWorkerErrors(), and then there could be bindings for service workers, Node workers etc.

My point was mostly about exposing a loosely coupled API like collectWindowErrors(logger) or collectPlaygroundErrors(logger, playground); vs tightly coupled logger.collectWindowErrors() logger.addPlaygroundRequestEndListener(). This would make the Logger class lean and responsible for a single thing, while the "error collectors" could be plentiful, separate, and also provided by other modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored the code to match what you described in a3feb32.

Let me know what you think.

collectWindowErrors runs only in the Website package now.

@bgrgicak bgrgicak requested a review from adamziel February 27, 2024 05:43
@@ -94,6 +95,7 @@ export async function startPlaygroundWeb({
}),
progressTracker
);
getLogger().addPlaygroundRequestEndListener(playground);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick for iteration 2: We could import an instantiated logger instead of a getLogger() function.

import { logger } from '@php-wasm/logger';
logger.addPlaygroundRequestEndListener(playground);

Copy link
Collaborator

@adamziel adamziel Feb 27, 2024

Choose a reason for hiding this comment

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

Oh! This is actually blocking: the logger should only collect all the window errors on playground.wordpress.net in the context of the website app. However, the client package is a universally reusable artifact that other websites and app can reuse, e.g. https://translate.wordpress.org/projects/wp-plugins/friends/dev/de/default/playground/. do this:

import { startPlaygroundWeb, phpVars, setPluginProxyURL } from 'https://playground.wordpress.net/client/index.js';

Calling this.collectJavaScriptErrors(); in the logger constructor would make the logger collect all the Playground-unrelated global JS errors in all the apps that merely use the client.

This is why I wouldn't attach any global error collection logic (e.g. window-related) at the library level but defer it to the consumer app. This way, Playground worker-thread, remote/index.html and website could all decide to attach global error listeners without simultaneously imposing that decision on the API consumers.

That being said, binding the logger to the Playground instance here seems great 👍

@adamziel
Copy link
Collaborator

@bgrgicak I left a bunch of nitpicks that are fine to address in a subsequent PR, but I also found one blocker:

#1035 (comment)

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

This looks good, thank you for working through all the feedback @bgrgicak! I left one last note in https://github.com/WordPress/wordpress-playground/pull/1035/files#r1506127419, and once that's addressed I'm happy to get this in!

* @param loggerInstance The logger instance
*/
export function collectWindowErrors(loggerInstance: Logger) {
loggerInstance.addWindowErrorListener();
Copy link
Collaborator

@adamziel adamziel Feb 28, 2024

Choose a reason for hiding this comment

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

Perhaps eventually this call could be inlined, as in:

export function collectWindowErrors(loggerInstance: Logger) {
		window.addEventListener('error', e => logWindowError(loggerInstance, e));
		window.addEventListener(
			'unhandledrejection',
			e => logUnhandledRejection(loggerInstance, e.reason)
		);
		window.addEventListener(
			'rejectionhandled',
			e => logUnhandledRejection(loggerInstance, e.reason)
		);
}

But for now, let's leave it the way it is and see how adding support for other runtimes shapes the development of the API.

@bgrgicak
Copy link
Collaborator Author

I moved the log collection to main.

@bgrgicak bgrgicak merged commit 050a2dc into trunk Feb 29, 2024
5 checks passed
@bgrgicak bgrgicak deleted the add/wp-logs branch February 29, 2024 07:39
@@ -82,6 +83,10 @@ function Main() {
Partial<ExportFormValues>
>({});

useEffect(() => {
collectWindowErrors(logger);
Copy link
Collaborator

@adamziel adamziel Feb 29, 2024

Choose a reason for hiding this comment

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

This could even be a top-level call outside of the Main component , e.g. right after the imports – this way it would also collect all the errors that happened before React had a chance to render.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can move it in the next pr.

@adamziel
Copy link
Collaborator

Yay, I'm really glad to have this PR in!

@bgrgicak bgrgicak linked an issue Mar 1, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose PHP errors with JavaScript events
2 participants