-
Notifications
You must be signed in to change notification settings - Fork 322
[php-wasm-logger] Filter logs by severity in Logger and assign severity based on verbosity argument in CLIs #2436
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
base: trunk
Are you sure you want to change the base?
Changes from 8 commits
071ffc2
400999f
805dc23
d5c3140
436e46d
f48d972
a8a6421
bcf694c
61908a9
1cdcec7
00d7e5c
39c5d90
ace24c4
33dcebb
7350c02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,22 +17,48 @@ export type Log = { | |
raw?: boolean; | ||
}; | ||
|
||
/** | ||
* Log verbosity levels | ||
*/ | ||
export const LogVerbosity = { | ||
Normal: 'normal', | ||
Quiet: 'quiet', | ||
Debug: 'debug', | ||
} as const; | ||
|
||
export type LogVerbosity = (typeof LogVerbosity)[keyof typeof LogVerbosity]; | ||
|
||
/** | ||
* Log severity levels. | ||
*/ | ||
export type LogSeverity = 'Debug' | 'Info' | 'Warn' | 'Error' | 'Fatal'; | ||
export const LogSeverity = { | ||
Debug: 'debug', | ||
Info: 'info', | ||
Warn: 'warn', | ||
Error: 'error', | ||
Fatal: 'fatal', | ||
} as const; | ||
|
||
export type LogSeverity = (typeof LogSeverity)[keyof typeof LogSeverity]; | ||
|
||
/** | ||
* Log prefix. | ||
*/ | ||
export type LogPrefix = 'WASM Crash' | 'PHP' | 'JavaScript'; | ||
export const LogPrefix = { | ||
WASM: 'Wasm Crash', | ||
PHP: 'PHP', | ||
JS: 'JavaScript', | ||
} as const; | ||
|
||
export type LogPrefix = (typeof LogPrefix)[keyof typeof LogPrefix]; | ||
|
||
/** | ||
* A logger for Playground. | ||
*/ | ||
export class Logger extends EventTarget { | ||
public readonly fatalErrorEvent = 'playground-fatal-error'; | ||
private readonly handlers: LogHandler[]; | ||
private handlers: LogHandler[]; | ||
private filters: LogSeverity[]; | ||
|
||
// constructor | ||
constructor( | ||
|
@@ -41,6 +67,7 @@ export class Logger extends EventTarget { | |
handlers: LogHandler[] = [] | ||
) { | ||
super(); | ||
this.filters = Object.values(LogSeverity); | ||
this.handlers = handlers; | ||
} | ||
|
||
|
@@ -62,14 +89,39 @@ export class Logger extends EventTarget { | |
/** | ||
* Log message with severity. | ||
* | ||
* @param message any | ||
* @param severity LogSeverity | ||
* @param raw boolean | ||
* @param log Log | ||
* @param args any | ||
*/ | ||
public logMessage(log: Log, ...args: any[]): void { | ||
for (const handler of this.handlers) { | ||
handler(log, ...args); | ||
const isConsole = handler.name === 'logToConsole'; | ||
const isSeverityAllowed = log.severity | ||
? this.filters.includes(log.severity) | ||
: !!this.filters.length; | ||
|
||
if (!isConsole || isSeverityAllowed) { | ||
handler(log, ...args); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused – what's the reasoning here? Do we need special casing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're asking for the If you're asking for the I would suggest to replace the undefined severity by a - log.severity ?? LogSeverity.Info,
+ log.severity == LogSeverity.Log ? LogSeverity.Info : log.severity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, good question! Let's target all handlers. If we want to restrict the console output while still collecting all the logs somewhere, we can do that at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
} | ||
|
||
/** | ||
* Filter message based on verbosiy | ||
* @param verbosity LogVerbosity | ||
*/ | ||
public filterByVerbosity(verbosity: LogVerbosity): void { | ||
if (verbosity === LogVerbosity.Quiet) { | ||
adamziel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.filters = []; | ||
} | ||
|
||
if (verbosity === LogVerbosity.Normal) { | ||
this.filters = Object.values(LogSeverity).filter( | ||
(severity) => severity !== LogSeverity.Debug | ||
); | ||
} | ||
|
||
if (verbosity === LogVerbosity.Debug) { | ||
this.filters = Object.values(LogSeverity); | ||
} | ||
} | ||
|
||
|
@@ -84,7 +136,7 @@ export class Logger extends EventTarget { | |
{ | ||
message, | ||
severity: undefined, | ||
prefix: 'JavaScript', | ||
prefix: LogPrefix.JS, | ||
raw: false, | ||
}, | ||
...args | ||
|
@@ -101,8 +153,8 @@ export class Logger extends EventTarget { | |
this.logMessage( | ||
{ | ||
message, | ||
severity: 'Debug', | ||
prefix: 'JavaScript', | ||
severity: LogSeverity.Debug, | ||
prefix: LogPrefix.JS, | ||
raw: false, | ||
}, | ||
...args | ||
|
@@ -119,8 +171,8 @@ export class Logger extends EventTarget { | |
this.logMessage( | ||
{ | ||
message, | ||
severity: 'Info', | ||
prefix: 'JavaScript', | ||
severity: LogSeverity.Info, | ||
prefix: LogPrefix.JS, | ||
raw: false, | ||
}, | ||
...args | ||
|
@@ -137,8 +189,8 @@ export class Logger extends EventTarget { | |
this.logMessage( | ||
{ | ||
message, | ||
severity: 'Warn', | ||
prefix: 'JavaScript', | ||
severity: LogSeverity.Warn, | ||
prefix: LogPrefix.JS, | ||
raw: false, | ||
}, | ||
...args | ||
|
@@ -155,8 +207,8 @@ export class Logger extends EventTarget { | |
this.logMessage( | ||
{ | ||
message, | ||
severity: 'Error', | ||
prefix: 'JavaScript', | ||
severity: LogSeverity.Error, | ||
prefix: LogPrefix.JS, | ||
raw: false, | ||
}, | ||
...args | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,77 @@ | ||
import { logger } from '../lib/logger'; | ||
import { clearMemoryLogs } from '../lib/log-handlers'; | ||
import { type Log, logger, LogVerbosity } from '../lib/logger'; | ||
import { clearMemoryLogs, type LogHandler } from '../lib/log-handlers'; | ||
|
||
describe('Logger', () => { | ||
beforeEach(async () => { | ||
let output: string[]; | ||
let handlers: LogHandler[]; | ||
|
||
function logToConsole(log: Log, arg?: string) { | ||
output.push(`${log.message}${arg ? arg : ''}`); | ||
} | ||
|
||
beforeAll(() => { | ||
// @ts-ignore | ||
handlers = logger.handlers; | ||
}); | ||
|
||
beforeEach(() => { | ||
output = []; | ||
// @ts-ignore | ||
logger.handlers = [...handlers, logToConsole]; | ||
|
||
clearMemoryLogs(); | ||
}); | ||
|
||
it('Log message should be added', () => { | ||
it('adds message in logs', () => { | ||
logger.warn('test'); | ||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(1); | ||
expect(logs[0]).toMatch( | ||
/\[\d{2}-[A-Za-z]{3,4}-\d{4} \d{2}:\d{2}:\d{2} UTC\] JavaScript Warn: test/ | ||
/\[\d{2}-[A-Za-z]{3,4}-\d{4} \d{2}:\d{2}:\d{2} UTC\] JavaScript warn: test/ | ||
); | ||
}); | ||
|
||
it('Log event should be dispatched', () => { | ||
it('dispatches log event', () => { | ||
const eventListener = vitest.fn(); | ||
logger.addEventListener('playground-log', eventListener); | ||
logger.warn('test'); | ||
expect(eventListener).toHaveBeenCalled(); | ||
}); | ||
|
||
it('outputs all logs by default', () => { | ||
logger.log('log'); | ||
logger.info('info'); | ||
logger.warn('warn'); | ||
logger.error('error'); | ||
logger.debug('debug'); | ||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(5); | ||
expect(output).toEqual(['log', 'info', 'warn', 'error', 'debug']); | ||
}); | ||
|
||
it('outputs main logs when verbosity is set to normal', () => { | ||
logger.filterByVerbosity(LogVerbosity.Normal); | ||
logger.log('log'); | ||
logger.debug('debug'); | ||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(2); | ||
expect(output).toEqual(['log']); | ||
}); | ||
|
||
it('outputs main and debug logs when verbosity is set to debug', () => { | ||
logger.filterByVerbosity(LogVerbosity.Debug); | ||
logger.log('log'); | ||
logger.debug('debug'); | ||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(2); | ||
expect(output).toEqual(['log', 'debug']); | ||
}); | ||
|
||
it('does not output logs when verbosity is set to quiet', () => { | ||
logger.filterByVerbosity(LogVerbosity.Quiet); | ||
logger.log('log'); | ||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(1); | ||
expect(output).toEqual([]); | ||
}); | ||
}); |
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.
Let's not distinguish between verbosity and severity at the logger level. One concept is enough. We can decide what each verbosity means in the CLI args parser when we let the user decide. All internal methods could use severities for filtering.
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.
done.