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): add time/timeEnd methods #5905

Merged
merged 6 commits into from
Aug 24, 2018
Merged

misc(logger): add time/timeEnd methods #5905

merged 6 commits into from
Aug 24, 2018

Conversation

patrickhulce
Copy link
Collaborator

In order to land #3745, we need logger to be published with the new functionality. This is just the lighthouse-logger portion of #3745 with a version bump to 1.1.0

@@ -6,6 +6,8 @@
'use strict';

const debug = require('debug');
const marky = require('marky');
Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

nit and some bike shedding, but otherwise LGTM. Thanks for splitting this off!


static timeEnd({msg, id, args=[]}, level='verbose') {
Log[level]('statusEnd', msg, ...args);
return marky.stop(id);
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 going to be used? Should update lighthouse-logger/index.d.ts or drop the return here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -207,5 +219,10 @@ class Log {
}

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

Choose a reason for hiding this comment

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

I realize this (appears) to be going for the same getEntries() name as the Performance.getEntries(), but it's definitely confusing in the context of a logger (where you might expect it to return all log entries).

WDYT about Log.getTimeEntries()?

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 had the same feeling as well, will do 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

settled on takeTimeEntries to mimic the semantics and name of PerfObserver 👍

Copy link
Collaborator

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -207,5 +219,10 @@ class Log {
}

Log.events = new Emitter();
Log.getEntries = _ => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe make this a getter instead of a function. No big deal though

@patrickhulce patrickhulce merged commit 3ad529d into master Aug 24, 2018
@patrickhulce patrickhulce deleted the logger_time branch August 24, 2018 22:39
@paulirish
Copy link
Member

lighthouse-logger@1.1.0 published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants