search

yaorg / node-measured

A Node metrics library for measuring and reporting application-level metrics, inspired by Coda Hale, Yammer Inc's Dropwizard Metrics Libraries
https://yaorg.github.io/node-measured/
MIT License
516 stars 52 forks source link

Switch to pino #61

Closed Qard closed 5 years ago

Qard commented 5 years ago

Bunyan has an optional dependency on dtrace-provider, which is a native module. With optional dependencies, it's fine for them to fail to install. On Windows, however, the build process uses node-gyp and therefore fails the install with a big scary error.

To avoid the build error, I've switched the logger to pino.

Fixes #60

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 358

Totals Coverage Status
Change from base Build 356: 0.0%
Covered Lines: 749
Relevant Lines: 827
💛 - Coveralls
Qard commented 5 years ago

@fieldju Does this look good to you? I'd like to land and release this soon, if I can.

fieldju commented 5 years ago

Oh yeah, this look good to me. I've been on vacation sorry.

fieldju commented 5 years ago

@watson idea of just falling back to console log and having pino just be a dev dep.

I don't have strong opinions on this one

Qard commented 5 years ago

Alternatively, we could use a null logger by default which implements the logger interface but just drops the messages.

Qard commented 5 years ago

I discussed a bit more with @watson and we're thinking it would be better to move the LoggingReporter out to a separate module so the log module dependencies can be removed entirely from our install. This would instead leave measured-reporting as mainly just a bundle of abstract classes. This would mean neither bunyan nor pino would be required in this module and would only be required in the module which provides the concrete implementation for the LoggingReporter class.

If this seems like a reasonable approach to you, let me know what you would want to call the new log reporter module and I can write a PR to split the two.

fieldju commented 5 years ago

I think just defaulting to console.log would be good enough.

The main purpose of the logging reporter is to have a reference reporter as well as have something that is handy for local developing / debugging. I think just defaulting to console.log is decent since it doesn't require an external lib and still is functional and can be overridden by supplying your own impl.

Can you update this to use console log and we can merge and deploy it out?

Qard commented 5 years ago

In that case, @watson, what do you think of using https://www.npmjs.com/package/console-log-level rather than pino here?

watson commented 5 years ago

@Qard Fine with me 👍

Qard commented 5 years ago

I just opened a new PR to use console-log-level instead. #62