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

misc(logger): 1.3.0, package types, timeDecorate, add tests #6740

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 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
2 changes: 2 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
collectCoverageFrom: [
'**/lighthouse-core/**/*.js',
'**/lighthouse-cli/**/*.js',
'**/lighthouse-logger/**/*.js',
'!**/test/',
'!**/scripts/',
],
Expand All @@ -19,5 +20,6 @@ module.exports = {
testMatch: [
'**/lighthouse-core/**/*-test.js',
'**/lighthouse-cli/**/*-test.js',
'**/lighthouse-logger/**/*-test.js',
],
};
9 changes: 9 additions & 0 deletions lighthouse-core/lib/log.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/** @type {typeof import('../../lighthouse-logger')} */
module.exports = require('lighthouse-logger');
Copy link
Member

Choose a reason for hiding this comment

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

is this file used anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be, but I should have removed it as part of breaking up the original PR (#6515). It'll be needed so that importing the logger has types, w/o needing to use the /** @type comment everywhere.

Will delete from this PR.

218 changes: 200 additions & 18 deletions lighthouse-logger/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,54 @@
'use strict';

const debug = require('debug');
// @ts-ignore
const marky = require('marky');
Copy link
Member

Choose a reason for hiding this comment

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

can you add a type file for marky in types/marky? It looks like it should be fairly minimal.

Copy link
Member

Choose a reason for hiding this comment

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

yah?
It looks like it's basically

declare module 'marky' {
  class Marky {
    static mark(name: string): void;
    static stop(name: string): void;
    static getEntries(): PerformanceEntry[];
    static clear(): void;
  }
  export = Marky;
}

or whatever of the many variations that would work in types/marky/index.d.ts


const EventEmitter = require('events').EventEmitter;

/**
* @template {Function} F
* @typedef {import('./type-helpers').ArgumentTypes<F>} ArgumentTypes
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get away with just the built-in Parameters<F> for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great!

*/

/**
* @template T
* @typedef {import('./type-helpers').IsFunction<T>} IsFunction
*/

/**
* @template Class
* @template {*[]} Args
* @typedef {{
* msg: string | ((this: Class, ...args: Args) => string),
* id?: string | ((this: Class, ...args: Args) => string),
Copy link
Member

Choose a reason for hiding this comment

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

what if we drop the dynamic messages and only support strings here (at least for now)? It would make this file simpler to read, simpler for type checking, and reduces the non-local reasoning needed where the timeDecorateClass calls are located.

AFAICT, the only places we need dynamically created strings are for

  • running beforePass/pass/afterPass for a gatherer, and
  • running an audit
  • computing a computed artifact.

Due to the structure of gatherRunner there's no good place to decorate the gatherer functions, so #6515 only included dynamic messages for last two. We could do those with manual log.time()/log.timeEnd() calls (only two pairs of them needed) and be able to get rid of a lot of machinery in here.

And then at a later date we can always add support if we find we want it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It'll be most immediately needed by #6410, since each call to sendCommand has a timing with an id derived from the input. As soon as this lands I'll be pushing that issue forward again. With that in mind, should this stay?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, yeah, that's useful enough that I guess we should probably keep it

* timeStartLogLevel?: LogAction,
* timeEndLogLevel?: LogAction,
* }} TimeDecorateOpts
*/

/**
* @typedef {'verbose'|'error'|'silent'|'log'|'info'|'warn'} LogLevel
*/
/**
* @typedef {LogLevel & keyof typeof Log} LogAction
Copy link
Member

Choose a reason for hiding this comment

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

this is probably a little too clever to be able to quickly see what's going on :)

I'd say at least have LogAction explicit and @typedef {LogAction |'silent'|'info'} LogLevel, so the build up is visible.

But really LogLevel and LogAction are different things (e.g. error level is distinct from the verbose level, but the error action is included in the verbose level) and log isn't even a a level (anymore than asdfasdfaasdf is, at least :), so listing both explicitly is probably best :)

*/

/**
* @typedef {{
* msg:string,
* id: string,
* args?: any[],
* }} Status
*/

/**
* @typedef {[any, ...any[]]} ArrayAtLeastOne
*/

const isWindows = process.platform === 'win32';

// process.browser is set when browserify'd via the `process` npm module
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

combine the comments? (// @ts-ignore - process.browser is set...)

const isBrowser = process.browser;

const colors = {
Expand All @@ -24,6 +66,7 @@ const colors = {
};

// whitelist non-red/yellow colors for debug()
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

combine comments (and maybe add something about it being non-public to explain the ts-ignore)

debug.colors = [colors.cyan, colors.green, colors.blue, colors.magenta];

class Emitter extends EventEmitter {
Expand All @@ -50,33 +93,46 @@ class Emitter extends EventEmitter {
}
}

/** @type {{[k: string] : debug.IDebugger}} */
Copy link
Member

Choose a reason for hiding this comment

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

nit: can equivalently do /** @type {Record<string, debug.IDebugger>} */, a little more readable

const loggersByTitle = {};

const loggingBufferColumns = 25;

/** @type {LogLevel} */
let level_;

class Log {
/**
* @param {string} title
* @param {ArrayAtLeastOne} argsArray
*/
static _logToStdErr(title, argsArray) {
const log = Log.loggerfn(title);
log(...argsArray);
}

/**
* @param {string} title
*/
static loggerfn(title) {
let log = loggersByTitle[title];
if (!log) {
log = debug(title);
loggersByTitle[title] = log;
// errors with red, warnings with yellow.
if (title.endsWith('error')) {
// @ts-ignore
log.color = colors.red;
} else if (title.endsWith('warn')) {
// @ts-ignore
log.color = colors.yellow;
}
}
return log;
}

/**
* @param {string} level
* @param {LogLevel} level
*/
static setLevel(level) {
level_ = level;
Expand All @@ -99,10 +155,10 @@ class Log {
* A simple formatting utility for event logging.
* @param {string} prefix
* @param {!Object} data A JSON-serializable object of event data to log.
* @param {string=} level Optional logging level. Defaults to 'log'.
* @param {LogLevel=} level Optional logging level. Defaults to 'log'.
*/
static formatProtocol(prefix, data, level) {
const columns = (!process || process.browser) ? Infinity : process.stdout.columns;
const columns = isBrowser ? Infinity : (process.stdout.columns || Infinity);
Copy link
Member

Choose a reason for hiding this comment

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

nice one

const method = data.method || '?????';
const maxLength = columns - method.length - prefix.length - loggingBufferColumns;
// IO.read blacklisted here to avoid logging megabytes of trace data
Expand All @@ -118,33 +174,149 @@ class Log {
return level_ === 'verbose';
}

static time({msg, id, args = []}, level = 'log') {
/**
* @param {Status} param0
Copy link
Member

Choose a reason for hiding this comment

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

maybe s/param0/status?

* @param {LogAction} level
*/
static time({msg, id, args}, level = 'log') {
marky.mark(id);
Log[level]('status', msg, ...args);
Log[level]('status', msg, ...(args || []));
}

static timeEnd({msg, id, args = []}, level = 'verbose') {
Log[level]('statusEnd', msg, ...args);
/**
* @param {Status} param0
* @param {LogAction} level
*/
static timeEnd({msg, id, args}, level = 'verbose') {
Log[level]('statusEnd', msg, ...(args || []));
marky.stop(id);
}

/* eslint-disable no-invalid-this */
/**
* Decorates a function, calling time/timeEnd before/after calling the original function.
* @template T
* @template {*} R
* @template {*[]} Args
* @param {(this: T, ...args: Args) => R} originalFn
Copy link
Member

Choose a reason for hiding this comment

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

hmm, it seems from the uses in #6515 and timeDecorateClass in this PR that there isn't actually (usually?) a this for the function? It ends up just resolving to any (for now...discussion is talking about defaulting to unknown), so no real checking is happening for it.

What about passing in a this arg explicitly (defaulting to undefined as a this-less call would)?

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 don't think that is an option with how timeDecorateClass uses this method - there's no instance of the class there.

anyhow, none of the uses cases in #6515 required this for timeDecorate (for bare functions), so I'll just remove that part of the typing.

* @param {TimeDecorateOpts<T, Args>} opts
* @return {(this: T, ...args: Args) => R}
*/
static timeDecorate(originalFn, opts) {
/**
* @type {(this: *, ...args: *[]) => string}
Copy link
Member

Choose a reason for hiding this comment

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

_this (it's concerning that no error is complaining about it :)

*/
const computeMsg = (_this, args) => {
Copy link
Member

Choose a reason for hiding this comment

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

could this fn be as simple as:

  const computeMsg = (_this, args) => {
    if (typeof opts.msg === 'string') return opts.msg;
    if (typeof opts.msg === 'function') return opts.msg.apply(_this, args);
    throw new Error('expected msg');
  };

and same with computeId()

checking that the first two cases don't return something falsy seems.. unnecessary to me, but w/e

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh ya for sure

this only made sense before I figured out how to strongly type all this. had to verify this at runtime. but now TSC does the heavy lifting here :)

if (typeof opts.msg === 'string') return opts.msg;
if (typeof opts.msg === 'function') return opts.msg.apply(_this, args);
throw new Error('expected msg');
};

/**
* @type {(this: *, ...args: *[]) => string}
*/
const computeId = (_this, args) => {
if (typeof opts.id === 'string') return opts.id;
if (typeof opts.id === 'function') return opts.id.apply(_this, args);
return `lh:${originalFn.name}`;
};

/**
* @type {(this: T, ...args: Args) => R}
*/
const fn = function timeDecoratedFn(...args) {
Copy link
Member

Choose a reason for hiding this comment

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

drop the const fn =, or is there a reason for it?

const timeStartLogLevel = opts.timeStartLogLevel || 'log';
const timeEndLogLevel = opts.timeEndLogLevel || 'verbose';
Copy link
Member

Choose a reason for hiding this comment

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

move outside fn?


const status = {msg: computeMsg(this, args), id: computeId(this, args)};
Log.time(status, timeStartLogLevel);

let result;
try {
result = originalFn.apply(this, args);
} catch (err) {
Log.timeEnd(status, timeEndLogLevel);
// intercept any errors and elide the time decoration from the stack trace
Copy link
Member

Choose a reason for hiding this comment

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

Sooooo...I should have been more clear in #6425 (comment). I really meant, if the stack traces are bad we should consider this. Are they? Because if not it can be really confusing why all the function calls that actually happened aren't in the trace.

Maybe we should punt on this and add it later if we find the stack traces difficult to work with? Or have you found it helpful to get clean stack traces so far?

(I'm also curious how official decorators plan on handling this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It didn't really come up, so I can't say I found it useful.

Monitoring tools like Rollbar do their best to clean up calls to their lib functions from stack traces. But our case is very minor, and theirs is an library meant for monitoring ...

I could go either way with this. If you think of a stack trace as primarily a tool developers use to navigate code flow, It's a nice idea to hide function calls. If you want to see exactly the whole picture, you wouldn't want things hidden. I imagine it won't ever come up again either way we go with.

Copy link
Member

Choose a reason for hiding this comment

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

It didn't really come up, so I can't say I found it useful.

OK, cool, sounds like we should remove it then? Keeping the implementation simple(r) is always 👍

err.stack = err.stack.replace(/.* at timeDecoratedFn .*\n/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

add a comment to explain this hack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya

throw err;
}

if (result && typeof result.then === 'function') {
return result.then((/** @type {any} */ value) => {
Log.timeEnd(status, timeEndLogLevel);
return value;
}).catch((/** @type {any} */ err) => {
Log.timeEnd(status, timeEndLogLevel);
// intercept any errors and elide the time decoration from the stack trace
err.stack = err.stack.replace(/.* at timeDecoratedFn .*\n/, '');
throw err;
});
} else {
Log.timeEnd(status, timeEndLogLevel);
return result;
}
};
return fn;
}
/* eslint-enable no-invalid-this */

/**
* Decorates (like timeDecorate) specified methods of a class.
* If decorating instance methods, use the class's prototype.
* If decorating static methods, use the class directly.
* @template {Object|Function} Class
Copy link
Member

Choose a reason for hiding this comment

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

can we be more specific? This is just any

Copy link
Collaborator Author

@connorjclark connorjclark Dec 13, 2018

Choose a reason for hiding this comment

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

I want to accept anything newable (classes, so basically any function) or objects (prototypes). I suppose objects and functions covers everything :P

What about something silly like this?

* @template {keyof Class} Prop
Copy link
Member

Choose a reason for hiding this comment

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

should be limited to properties that are functions? E.g. you can pass a number property in (and it will accept any parameters for msg and id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this part guards against non-functions:

* @param {{[key in Prop]: TimeDecorateOpts<Class, Parameters<IsFunction<Class[key]>>>}} methods

IsFunction<T> resolves to never if T does not extend Function.

* @param {Class} klass
* @param {{[key in Prop]: TimeDecorateOpts<Class, ArgumentTypes<IsFunction<Class[key]>>>}} methods
*/
static timeDecorateClass(klass, methods) {
for (const [method, opts] of Object.entries(methods)) {
if (!opts.id) {
const className = (typeof klass === 'function' ? klass : klass.constructor).name;
opts.id = `lh:${className}:${method}`;
}
/** @type {IsFunction<Class[typeof method]>} */
const original = klass[method];
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that basically everything in this and the above function are typed as any :)

I wonder if there's something simpler (or defining more in a d.ts file if necessary) to get us some constraints enforced. Or we go the opposite, keeping the interface as strict as possible and going explicitly any inside (this is partially where timeDecorate has gone, at least with the return value).

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'm not sure. My main goal was to only allow property names of functions of klass in methods. The current typing achieves that, but the local typing is a bit out of my reach (or TypeScript's?). But the ol' gut type checker seems right to me (AKA it looks correct).

if (!original) {
throw new Error('Cannot decorate non-existent method ${method}');
}
klass[method] = Log.timeDecorate(original, opts);
}
}

/**
* @param {string} title
* @param {ArrayAtLeastOne} args
Copy link
Member

Choose a reason for hiding this comment

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

Was ArrayAtLeastOne to make the debug types happy? Or to nicely make the message required?

Should we take another step and make it explicit? That would also have the benefit of avoiding the tuple magic.

Something like:

/**
 * @param {string} title
 * @param {any} message
 * @param {any[]} args
 */
static log(title, message, ...args) {
  // ...
}

(we could go further and make message a string, but I found at least one place where we pass an Error in directly)

Copy link
Member

Choose a reason for hiding this comment

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

a quick check also caught a knock on effect in console-quieter, where it's going to have to be convinced there's at least one entry.args:

_logs.forEach(entry => {
log.verbose(`${entry.prefix}-${entry.type}`, ...entry.args);
});

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, it was to make debug happy
image

I like making the message required and more explicit. I'll do that instead.

*/
static log(title, ...args) {
Log.events.issueStatus(title, args);
return Log._logToStdErr(title, args);
Log._logToStdErr(title, args);
}

/**
* @param {string} title
* @param {ArrayAtLeastOne} args
*/
static warn(title, ...args) {
Log.events.issueWarning(title, args);
return Log._logToStdErr(`${title}:warn`, args);
Log._logToStdErr(`${title}:warn`, args);
}

/**
* @param {string} title
* @param {ArrayAtLeastOne} args
*/
static error(title, ...args) {
return Log._logToStdErr(`${title}:error`, args);
Log._logToStdErr(`${title}:error`, args);
}

/**
* @param {string} title
* @param {ArrayAtLeastOne} args
*/
static verbose(title, ...args) {
Log.events.issueStatus(title, args);
return Log._logToStdErr(`${title}:verbose`, args);
Log._logToStdErr(`${title}:verbose`, args);
}

/**
Expand Down Expand Up @@ -228,14 +400,24 @@ class Log {
static get doubleLightHorizontal() {
return '──';
}

/**
* @return {PerformanceEntry[]}
*/
static takeTimeEntries() {
const entries = marky.getEntries();
marky.clear();
return entries;
}

/**
* @return {PerformanceEntry[]}
*/
static getTimeEntries() {
return marky.getEntries();
}
}

Log.events = new Emitter();
Log.takeTimeEntries = () => {
Copy link
Member

Choose a reason for hiding this comment

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

why were these declared here before? I can't remember. Was it important? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, afaict there was no reason for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there was a good reason. It only blames to me because I split it off from the mega-PR :)

Seems fine to me!

const entries = marky.getEntries();
marky.clear();
return entries;
};
Log.getTimeEntries = () => marky.getEntries();

module.exports = Log;
2 changes: 1 addition & 1 deletion lighthouse-logger/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "lighthouse-logger",
"version": "1.2.0",
"version": "1.3.0",
"license": "Apache-2.0",
"dependencies": {
"debug": "^2.6.8",
Expand Down
Loading