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

lighthouse-logger improvments #6313

Closed
1 of 2 tasks
brendankenny opened this issue Oct 17, 2018 · 6 comments
Closed
1 of 2 tasks

lighthouse-logger improvments #6313

brendankenny opened this issue Oct 17, 2018 · 6 comments
Assignees
Labels

Comments

@brendankenny
Copy link
Member

brendankenny commented Oct 17, 2018

It's kind of annoying to update lighthouse-logger, but there are some rough edges that we'd like to smooth out. Came up in #6297:

  • Some way to query what the logging level is. Not usually needed, but sometimes you really want to skip work for verbose-only logs (e.g. querying things, slicing strings, etc) when it's not needed.
  • logging level in between verbose and default (debug or info perhaps?) that doesn't contain every protocol message
@connorjclark
Copy link
Collaborator

+1 for the first point. I was going to add some verbose timing around driver.sendCommand, but saw that I couldn't query the log level to prevent creating Dates unnecessarily.

@brendankenny
Copy link
Member Author

@hoten you can take the first if you want it :)

lighthouse-logger is a little unusual in that we publish it from within the lighthouse repo, require it elsewhere in lighthouse off of npm (not locally), but we don't use any tool to handle this for us.

In practice, this really only means we have to publish a version before we can make use of it in lighthouse, but since we don't have anything currently relying on being able to query the log level it shouldn't be a problem.

@brendankenny
Copy link
Member Author

one other design idea:
the method mentioned above, it could be used like

if (logger.level === 'verbose') { // or whatever
  const logValue = expensiveCalculation();
  log.verbose('Thing', logValue);
}

or we could extend logger to take functions that are only called by lighthouse-logger itself

log.verbose('Thing', _ => expensiveCalculation());

and if not at a verbose level, then that function would never be run

The benefit would be (I think) less work on the caller side. The downside is it might obscure what's going on. It might also just be unusual enough that it's dumb :)

@patrickhulce if you have thoughts

@connorjclark
Copy link
Collaborator

connorjclark commented Oct 24, 2018

I'll take this.

Spoke to @brendankenny offline. We decided to do just logger.isVerbose() for now. That should cover all of our current use cases / wish list items.

@connorjclark connorjclark self-assigned this Oct 24, 2018
@connorjclark
Copy link
Collaborator

I will also expose the time entries to help with #3745.

@brendankenny
Copy link
Member Author

First one is long done, haven't needed the second one since then, so no need to track it with an issue :)

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

No branches or pull requests

3 participants