Skip to content

Add logging support to Playground - #1035

Merged
bgrgicak merged 37 commits into
trunkfrom
add/wp-logs
Feb 29, 2024
Merged

Add logging support to Playground#1035
bgrgicak merged 37 commits into
trunkfrom
add/wp-logs

Conversation

@bgrgicak

@bgrgicak bgrgicak commented Feb 13, 2024

Copy link
Copy Markdown
Collaborator

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
Comment thread packages/php-wasm/universal/src/lib/base-php.ts Outdated
Comment thread packages/php-wasm/universal/src/lib/base-php.ts Outdated
Comment thread packages/php-wasm/universal/src/lib/logger.ts Outdated
Comment thread packages/php-wasm/universal/src/lib/logger.ts Outdated
Comment thread packages/playground/remote/src/lib/playground-mu-plugin/0-playground.php Outdated
Comment thread packages/playground/remote/src/lib/worker-thread.ts Outdated
@bgrgicak

Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread packages/php-wasm/logger/src/index.ts Outdated
Comment thread packages/php-wasm/logger/src/logger.ts Outdated
/**
* Collect errors from JavaScript window events like error and log them.
*/
private collectJavaScriptErrors() {

@adamziel adamziel Feb 26, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

@adamziel adamziel Feb 27, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

Comment thread packages/playground/blueprints/src/lib/compile.ts Outdated
Comment thread packages/playground/remote/src/lib/playground-mu-plugin/0-playground.php Outdated
Comment thread packages/playground/remote/src/lib/worker-thread.ts
@bgrgicak
bgrgicak requested a review from adamziel February 27, 2024 05:43
Comment thread packages/playground/blueprints/src/lib/compile.ts Outdated
Comment thread packages/playground/client/src/index.ts Outdated
}),
progressTracker
);
getLogger().addPlaygroundRequestEndListener(playground);

Copy link
Copy Markdown
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);

@adamziel adamziel Feb 27, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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)

Comment thread packages/playground/website/src/components/playground-viewport/index.tsx Outdated

@adamziel adamziel left a comment

Copy link
Copy Markdown
Collaborator

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();

@adamziel adamziel Feb 28, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator Author

I moved the log collection to main.

@bgrgicak
bgrgicak merged commit 050a2dc into trunk Feb 29, 2024
@bgrgicak
bgrgicak deleted the add/wp-logs branch February 29, 2024 07:39
>({});

useEffect(() => {
collectWindowErrors(logger);

@adamziel adamziel Feb 29, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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

2 participants