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

A pluggable logger #676

Open
reconbot opened this issue Jun 5, 2019 · 4 comments
Open

A pluggable logger #676

reconbot opened this issue Jun 5, 2019 · 4 comments
Labels
feature A new feature, or a feature request

Comments

@reconbot
Copy link
Contributor

reconbot commented Jun 5, 2019

Right now we support stdout and local system syslog (as opposed to a remote syslog). I'd like to propose a plugable logging system where you'd pass in a logger object that you create in the config file. (including examples)

The would work around the issue of needing optionalDeps at all and would remove modern-syslog from the build allowing people to use any logging system they want (remote syslog, winston, pino, etc)

I'm volunteering to do this, but It's a major breaking change so I wanted to solicit feedback first.

@BlueHatbRit
Copy link
Member

Hey @reconbot, sorry for the delay. As I've noted on a few other issues my notifications were setup incorrectly for this repo 😢

This sounds like an interesting idea, it would be great if you could update your OP with a use case of why this would be useful to you. I'm also not totally sure we need to do this as a breaking API change, even if we were to change our logger we should still be able to offer everything we do now, we'd just be making additive changes to the configuration (I hope).

@BlueHatbRit BlueHatbRit added the feature A new feature, or a feature request label Jul 11, 2019
@reconbot
Copy link
Contributor Author

I'd like to use pino (with some custom extensions) so I can have JSON logging.

@reconbot
Copy link
Contributor Author

It's a slightly different topic but related. The Eval'd config file is strange and makes doing things like having a pluggable logger hard to do.

We could keep supporting the eval'd object but first check if require returns more than an empty module and then fall back to the current behavior. That would allow you to load and configure a logger object and export that in the config, instead of having to inline it into an object declaration. I think overall it would be a lot more powerful.

@BlueHatbRit
Copy link
Member

BlueHatbRit commented Jul 14, 2019

I agree, the eval is strange, the blame shows it's from 9 years ago with the first commit so I'd assume it was done because it was the easiest approach at the time.

I think your suggestion makes sense, it would then allow us to continue supporting generic JS files (that don't explicitly export), as well as traditional module.exported modules. We would also open up support for JSON files when using require, for light-weight and more standard config.

I think the next step would be to define an interface for the custom logger. Perhaps this could take the form of a new markdown doc submitted to the doc's directory and treated as an RFC?

If we are moving in that direction, I think we should consider two things.

  1. Keeping support for modern-syslog without breaking existing usages. I imagine a lot of people do make use of it and we wouldn't want to break that through upgrades.
  2. The solution would have to be light-weight and performant. The last thing we'd want to do is add a bunch of code and new dependencies to help with this which might slow things down. The simpler and smaller we can keep the implementation, the better.

What do you think, would you like to give the document a crack?

Side note, why have you chosen to use pino in particular over other loggers, what are you doing with the logs? It would be great to know so I have the full context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature, or a feature request
Projects
None yet
Development

No branches or pull requests

2 participants