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 more info to crash reports #1253

Merged
merged 19 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 72 additions & 4 deletions packages/php-wasm/logger/src/lib/logger.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/// <reference lib="WebWorker" />

import {
PHPRequestErrorEvent,
UniversalPHP,
Expand Down Expand Up @@ -33,6 +35,11 @@ export class Logger extends EventTarget {
*/
private lastPHPLogLength = 0;

/**
* Context data
*/
private context: Record<string, any> = {};

/**
* The path to the error log file.
*/
Expand Down Expand Up @@ -108,6 +115,21 @@ export class Logger extends EventTarget {
this.windowConnected = true;
}

/**
* Register a listener for service worker messages and log the data.
adamziel marked this conversation as resolved.
Show resolved Hide resolved
* The service worker will send the number of open Playground tabs.
*/
public addServiceWorkerMessageListener() {
navigator.serviceWorker.addEventListener('message', (event) => {
if (event.data?.numberOfOpenPlaygroundTabs !== undefined) {
this.addContext({
numberOfOpenPlaygroundTabs:
event.data.numberOfOpenPlaygroundTabs,
});
}
});
}

/**
* Register a listener for the request.end event and log the data.
* @param UniversalPHP playground instance
Expand Down Expand Up @@ -218,7 +240,21 @@ export class Logger extends EventTarget {
* @returns string[]
*/
public getLogs(): string[] {
return this.logs;
return [...this.logs];
}

/**
* Add context data.
*/
public addContext(data: Record<string, any>): void {
this.context = { ...this.context, ...data };
}

/**
* Get context data.
*/
public getContext(): Record<string, any> {
return { ...this.context };
}
}

Expand All @@ -233,6 +269,7 @@ export const logger: Logger = new Logger();
*/
export function collectWindowErrors(loggerInstance: Logger) {
loggerInstance.addWindowErrorListener();
loggerInstance.addServiceWorkerMessageListener();
}

/**
Expand All @@ -248,14 +285,45 @@ export function collectPhpLogs(
}

/**
* Add a listener for the fatal Playground errors.
* These errors include Playground errors like Asyncify errors. PHP errors won't trigger this event.
* **Call this inside a service worker.**
*
* Reports service worker metrics.
* Allows the logger to request metrics from the service worker by sending a message.
* The service worker will respond with the number of open Playground tabs.
*
* @param worker The service worker
*/
export function reportServiceWorkerMetrics(worker: ServiceWorkerGlobalScope) {
worker.addEventListener('activate', () => {
worker.clients.matchAll().then((clients) => {
const metrics = {
numberOfOpenPlaygroundTabs: clients.filter(
// Only count top-level frames to get the number of tabs.
(c) => c.frameType === 'top-level'
).length,
};
worker.clients
.matchAll({
includeUncontrolled: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why includeUncontrolled? Let's leave a comment in the code to explain. Otherwise LGTM 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bgrgicak yeah I meant more not the literal meaning of that option but rather why bother with uncontrolled clients.

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 update it in #1264 with something like this.

// Inform all Playground clients that there was a change in the number of active tabs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but why an uncontrolled client is considered a Playground client? It seems like that browser tab wouldn't be able to access Playground anyway, as being controlled is crucial for that, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, I tested this a bit and it's actually not needed, so I will remove this line in #1264

})
.then((clients) => {
for (const client of clients) {
client.postMessage(metrics);
}
});
});
});
}

/**
* Add a listener for the Playground crashes.
* These crashes include Playground errors like Asyncify errors.
* The callback function will receive an Event object with logs in the detail property.
*
* @param loggerInstance The logger instance
* @param callback The callback function
*/
export function addFatalErrorListener(
export function addCrashListener(
bgrgicak marked this conversation as resolved.
Show resolved Hide resolved
loggerInstance: Logger,
callback: EventListenerOrEventListenerObject
) {
Expand Down
4 changes: 2 additions & 2 deletions packages/php-wasm/logger/src/test/logger.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { NodePHP } from '@php-wasm/node';
import { LatestSupportedPHPVersion } from '@php-wasm/universal';
import { logger, addFatalErrorListener, collectPhpLogs } from '../lib/logger';
import { logger, addCrashListener, collectPhpLogs } from '../lib/logger';

describe('Logger', () => {
let php: NodePHP;
Expand All @@ -10,7 +10,7 @@ describe('Logger', () => {
it('Event listener should work', () => {
const listener = vi.fn();
collectPhpLogs(logger, php);
addFatalErrorListener(logger, listener);
addCrashListener(logger, listener);
php.dispatchEvent({
type: 'request.error',
error: new Error('test'),
Expand Down
3 changes: 3 additions & 0 deletions packages/playground/remote/service-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
broadcastMessageExpectReply,
} from '@php-wasm/web-service-worker';
import { wordPressRewriteRules } from '@wp-playground/wordpress';
import { reportServiceWorkerMetrics } from '@php-wasm/logger';

if (!(self as any).document) {
// Workaround: vite translates import.meta.url
Expand All @@ -22,6 +23,8 @@ if (!(self as any).document) {
self.document = {};
}

reportServiceWorkerMetrics(self);

initializeServiceWorker({
handleRequest(event) {
const fullUrl = new URL(event.request.url);
Expand Down
10 changes: 10 additions & 0 deletions packages/playground/website/public/logger.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ function response($ok, $error = null)
}
$text .= "\n\nUrl\n\n" . $_POST['url'];
}

if (isset($_POST['context']) && !empty($_POST['context'])) {
$text .= "\n\nContext\n\n" . $_POST['context'];
}

// Add blueprint
if (isset($_POST['blueprint']) && !empty($_POST['blueprint'])) {
$text .= "\n\nBlueprint\n\n" . $_POST['blueprint'];
}

$text = urlencode($text);

$ch = curl_init();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { useEffect, useState } from 'react';
import Modal from '../modal';
import { addFatalErrorListener, logger } from '@php-wasm/logger';
import { addCrashListener, logger } from '@php-wasm/logger';
import { Button, TextareaControl, TextControl } from '@wordpress/components';

import css from './style.module.css';

import { usePlaygroundContext } from '../../playground-context';
import { Blueprint } from '@wp-playground/blueprints';

export function ErrorReportModal() {
export function ErrorReportModal(props: { blueprint: Blueprint }) {
const { showErrorModal, setShowErrorModal } = usePlaygroundContext();
const [loading, setLoading] = useState(false);
const [text, setText] = useState('');
Expand All @@ -17,7 +18,7 @@ export function ErrorReportModal() {
const [submitError, setSubmitError] = useState('');

useEffect(() => {
addFatalErrorListener(logger, (e) => {
addCrashListener(logger, (e) => {
const error = e as CustomEvent;
if (error.detail?.source === 'php-wasm') {
setShowErrorModal(true);
Expand Down Expand Up @@ -50,6 +51,19 @@ export function ErrorReportModal() {
resetSubmission();
}

function getContext() {
bgrgicak marked this conversation as resolved.
Show resolved Hide resolved
return {
...props.blueprint.preferredVersions,
userAgent: navigator.userAgent,
...((window.performance as any)?.memory ?? {}),
...logger.getContext(),
window: {
width: window.innerWidth,
height: window.innerHeight,
},
};
}

async function onSubmit() {
setLoading(true);
const formdata = new FormData();
Expand All @@ -60,6 +74,8 @@ export function ErrorReportModal() {
if (url) {
formdata.append('url', url);
}
formdata.append('context', JSON.stringify(getContext()));
formdata.append('blueprint', JSON.stringify(props.blueprint));
try {
const response = await fetch(
'https://playground.wordpress.net/logger.php',
Expand Down
2 changes: 1 addition & 1 deletion packages/playground/website/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ function Main() {
<PlaygroundContext.Provider
value={{ storage, showErrorModal, setShowErrorModal }}
>
<ErrorReportModal />
<ErrorReportModal blueprint={blueprint} />
<PlaygroundViewport
storage={storage}
displayMode={displayMode}
Expand Down
Loading