Open TomFrost opened 6 years ago
This is spot on. People understand middleware and is a generic way to solve a lot of use cases such as excluding and only including certain fields.
I have written 2 targets, one for Sentry and one for Stackdriver, and in both cases I had to provide my own formatter, so glad this is being changed.
Love the log forking, been doing it for a while by hacking internal state in Bristol.
I think we should remove the custom log level names, and instead add more builtin ones to cover a wider spectrum. For example, the custom levels makes it hard to translate between Bristol and whatever target system you are outputting to that uses something else. Also makes TS typings harder (but not impossible).
Looking forward to it! Icing on the cake would be official typings for TypeScript 😄
Thanks Jeff!
People understand middleware and is a generic way to solve a lot of use cases such as excluding and only including certain fields.
To be clear, exclude/onlyInclude were mechanisms of restricting which messages would be passed to a certain target, and my thoughts around middleware didn't include a mechanism to prevent a message from being logged entirely. Though it might be cool to have a symbol flag for that right on the log data... then you could do this from middleware: logData[Bristol.WRITE_TO_LOG] = false
. The message would go the whole way through all the middleware, and each could flip that flag as they see fit. If it's false when it's time to call the target, the target doesn't get called.
I think we should remove the custom log level names, and instead add more builtin ones to cover a wider spectrum.
Hahaha this is exactly why the custom log level names exist :). When I first wrote Bristol, there was no agreement on what the log levels should be. Some argued that it should support all 7 syslog levels, others said that all those levels give logging too much friction on a team (because it blurs the line of which level to use for certain cases, and different engineers would choose different levels for the same type of message). Both are 100% valid use cases, so the levels became configurable similarly to how Winston works. This is used so heavily that I don't think we can get rid of it.
Just another vote for removing custom log levels and embracing the 7 Syslog Levels (of Hell). Custom log levels are an unnecessary source of bike shedding, they don't clear up dev confusion, they just make it worse at the level where Bristol operates. Levels like error
, warning
, etc are more than sufficient when working with log data, if there's a requirement or need to clarify what constitutes those types of logs it feels more like an application concern that can/should be abstracted.
@TomFrost ah, didn't think about that, I didnt use those options so I assume they meant filtering the log data which is exactly what middleware would let you do. All good. :)
And about log levels, I echo @Fluidbyte in that it just opens up for further bikeshedding. "warning" and above are the ones that are used the most I think, and people tend to know when to use them. I think the Syslog levels, given it's more or less a standard, makes sense.
I could be swayed to make syslog levels the default, but removing customizability of the levels isn't an option. That would be a dealbreaker for many current users of Bristol. I'm ok with breaking changes when there's a simple migration path (meaning that most/all changes happen in bootstrapping and not in every log call throughout one's app), and I can build an argument about removing functionality that I've never seen any evidence anyone's ever used (excluding/onlyIncluding), but I know for certain that levels customization is heavily used and I can't leave those users hanging. It's also a feature of Winston and has enabled many users to cut over to Bristol without unbearable changes to their codebases.
Regarding bikeshedding though -- while I see the possibility for bikeshedding the levels, I've heard of and been a part of numerous bike shedding discussions about what's a warn and what's a notice, and what's a notice and what's an info. Which pales in comparison to the rampant bikeshedding around emergency/alert/critical/error. Bunyan skirts this entirely by using fatal/error/warn/info/debug/trace. I dropped fatal
in Bristol because in some applications, whether or not an error is fatal can be dynamic. Ex: DB fails to connect, error. DB fails 3 times in a row, we force a crash.
The point is: You probably have a really strong opinion one way or the other. I can choose one and alienate a portion of Bristol's current userbase, or keep a heavily-used current feature and let everyone choose what's already working for them. I have to prioritize the userbase.
That's totally fair, making syslog the default is reasonable I'd say.
Thanks @TomFrost for taking into account #53 and #56. Agree with keeping log levels configurable, even though in order to avoid the paradox of choice, I was happy to see there are standards such as syslog.
Please keep the README readable - I found it much easier to parse than Winston's (1, 2).
Other issues that drove me away from choosing Winston despite its popularity, and may fall into the 1.0 scope:
Just wanted to bring one issue to your attention, @TomFrost, in case addressing it may lead to breaking changes: flushing logs before process.exit()
(#55).
From https://github.com/winstonjs/winston/issues/228:
Loosing logs, especially at end of process - where errors are most likely to occur - seems hazardous at the least.
That is a good point @dandv - i think adding a dispose function that returns a promise when all loggers have been disposed (up to the target implementation to resolve a promise when it is done flushing)
I use this pattern often for graceful shutdown, like waiting for writes to complete.
Hey @TomFrost, how's it going towards 1.0? :)
Bristol 1.0
The 1.0 release of Bristol is in the works, and will be a major breaking release with a focus on performance, flexibility, and configurability.
Shortcomings of 0.x
console.log
is a blocking operation.Solutions in 1.0
fn({Object} logData, {Object} options = {})
, where logData will contain the timestamp, severity, and any other data that Bristol attaches to log messages internally. Options can be specified when the middleware is attached. Middleware must operate synchronously (so that messages write to the target in order) and make their changes directly to thelogData
object.logFork.use(logData => { logData.user = req.tokenData.user })
. Pass your fork along to your request handler and now everything you log for that specific request gets tagged with the user that generated it. The fork will be garbage collected when the request is handled.excluding
andonlyIncluding
target configs will be removed, barring rampant complaints. No one restricts by anything but severities in real-world use cases.Limiting scope
1.0 will be a breaking update, and I'd like to define its scope by changing only the things, like the above, that are either breaking changes or features that users absolutely must learn about (such as using a stdout target instead of console target). While we could all list gobs of additional features we want on top of 1.0, I'd prefer to keep the 1.0 project small and iterate through the non-breaking changes later.
I also intend for 1.0 to draw a firm line in what Bristol ships with by default. Targets that are generalized or universal to most users will be included in Bristol -- file, console, stdout for sure in 1.0, syslog makes sense, I could even see an argument for a generic HTTP target that POSTs JSON logs to a configured URL with configured headers. Targets that are specific to aggregator products or narrow platforms will be excluded from Bristol core (Loggly has already been removed from
master
, for example), but the readme will encourage folks to publish middleware and targets to npm with the prefixbristol-
.RFC?
Paging @MarkHerhold @Fluidbyte @jeffijoe @dandv for your thoughts, as each of you have communicated challenging use cases that I'm hoping to largely solve with the above. I already know it solves my own ;-).