Is it possible to use timbre as logger?

[timur-han]

Hi everyone, Can I integrate timbre as default logger?

https://github.com/ptaoussanis/timbre

[cprice404]

Not yet, but this is something that we would like to be able to support in the future, so we'll try to get it on the roadmap.

I haven't looked at that part of the code in a while, but I don't think we interact with logback in many places, so hopefully it wouldn't be too hard to change. Would just need to think about how to toggle it via configuration.

[timur-han]

Ok thanks for the quick response :+1:

[cprice404]

@timur87 I wouldn't mind keeping this open as a reminder for us to consider this whenever we get a chance to look into the configuration for the next major version of TK... what do you think?

[dparis]

Hello Puppet team! Any action on this issue going on behind the scenes? We're revamping the logging system for our TK-based app, and we'd love to standardize around timbre. To that end, I've started adding timbre dependencies to our app and attempted to use slf4j-timbre to capture all log action throughout our dep tree. This causes a conflict with the logback-classic slf4j bindings which are explicitly required by trapperkeeper in the logging namespace.

Would you invite an external PR which attempts to modularize logging functionality? If so, I'd be happy to chat about what level of effort might be required, and what factors would need to be considered for upstream inclusion. I know that logging is a pretty critical bit of functionality, so if you folks don't have the bandwidth right now to wrangle this change, I might create an internal fork with some rough fixes to use in the meantime. That's not ideal for us, but our internal timeline is pretty tight on this, so we'll make do if necessary. :smile:

Thanks for your work on TK!

[cprice404]

@dparis we are definitely open to the idea, but also definitely won't have dev hours to spend on it any time soon. :( We should have no problem making time to review PRs and cut a release if you are interested in filing some.

First question: is it only the import statements of the logback classes in the logging namespace that cause the problem? Or does the simple fact that we have the logback jars on the classpath cause a problem for trying to use timbre? If it's just the import statements, then refactoring that to a separate namespace might be doable. But I fear it may be the magic that happens by just having those jars on the classpath, in which case the refactor would necessitate doing something like adding profiles to the lein project file to select the logging framework, or removing the logging deps from TK altogether and relying on downstream projects to add them to the deps list. That isn't a show-stopper, but would probably make it more difficult for us to coordinate the patch since it might mean that we'd have to modify our downstream projects to get things working. Definitely willing to talk through the options, in either case.

I can't recall off of the top of my head whether we load the logging before or after loading the config service. If the config service is loaded first, then I'd love it if we could add a new config setting in the global namespace for configuring alternate loggers. But if the logging is loaded first then we won't have access to the config data at the time that we need to initialize the logging, so that would probably force us to handle the logging backend either via a CLI arg, or lein profiles, or something along those lines.

If you have any ideas on how to make it modular, we'd love to talk through them. If there's a way we can integrate a patch that allows us to support timbre while maintaining backward compat for existing apps that want to stick with logback, that sounds like a patch we'd certainly be open to.

Thanks!

[KevinCorcoran]

I can't recall off of the top of my head whether we load the logging before or after loading the config service. If the config service is loaded first, then I'd love it if we could add a new config setting in the global namespace for configuring alternate loggers.

If memory serves, config is loaded first, since there is a setting in there (under global) which specifies the path of the logback configuration file. That might be a convenient place to add some kind of support for other logging systems like timbre.

[dparis]

Thanks for your replies!

@cprice404 The issue I'm seeing isn't with timbre per se, it's with the slf4j-timbre bindings which allows SFL4J to use timbre as the backend rather than (in this case) logback. If SLF4J offers an abstracted alternative to the functions/namespaces being imported by the TK logging namespace, perhaps refactoring to use those instead of logback directly and then making logback an optional dependency would work? TK applications could then specify logback in their class path and, I think, the behavior would be the same as it currently is while also allowing for different SLF4J bindings to be loaded as well. If there aren't equivalent functions in SLF4J, maybe all of the logback-specific initialization could be moved to a trapperkeeper-logging-config-logback service which is included by default. Then downstream TK apps could provide their own implementation of the logging-config service.

@KevinCorcoran Yup, looks like logging is initialized as part of the configuration startup with this function, after the config has already been parsed: https://github.com/puppetlabs/trapperkeeper/blob/007ac60a6fbc83d08921c81dff2d395d8d672279/src/puppetlabs/trapperkeeper/config.clj#L127

Ideally, for our specific use case, we'd be able to drop slf4j-timbre into our app's dependency list and TK (along with everything else supporting SLF4J) would start using timbre. We could then configure timbre as needed in our own service. Having logging config and init broken into a service that depends on the config service would seem to provide modularity while requiring minimal back-compat changes.

Looking forward to further discussion, and thanks again for all your work on trapperkeeper!

[cprice404]

The thing that will make it tricky for us is if we need to remove the logback deps from the TK project. We had issues in the past where different downstream services might use conflicting versions of some of the logback jars from one another, so standardizing on a specific set of them in the TK lein project has been helpful in avoiding those sorts of issues.

Perhaps it would be enough to just support 'exclusions' on those, though, and allow downstream apps to swap in timbre or anything else in their place? This would require, as you pointed out, removal of the references to the logback-specific classes in the imports in the logging namespace. If you are able to work up a PR that does that (and I definitely agree with you that trying to go to generic slf4j classes would be the first thing to try) we would be happy to review it!

[dparis]

@cprice404 Cool, I'll take a look at it and see if I can't get some traction in that direction.