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
Support clojure.tools.logging's debugf, infof, warnf, etc. #8
Comments
Hi, Sorry for the delay replying - was travelling the past few weeks. Working through a backlog now, will try get back to you in a couple days! |
Okay, I'd like a little time to think about this if possible - it's not entirely obvious what the best way of implementing this would be since the current log fns are already variadic. In most cases any extra args are just concatenated into an output string - is there a particular reason you're not happy with that approach? E.g. |
Alternately, you could just provide an implementation of clojure.tools.logging.impl.LoggerFactory protocol, and then everyone could just call the functions and macros that clojure.tools.logging provides. No need to duplicate everything. |
Hi Alexander,
I'm not a big fan of this idea to be honest. There are some significant differences between the tools.logging and Timbre APIs, particularly on the config side. Appender caching, namespace filtering, middleware, rate limiting, etc.: these are all relatively performant and simple with Timbre's current implementation. Trying to patch over the differences between tools.logging and Timbre would require sacrifices for not much inherent benefit from what I can tell (?). If there's a good reason to provide Cheers! :-) |
Edited my last comment for unintended abruptness :-) |
The most common request I've received is some way to change the logging config from inside the repl, and I've pushed back on it precisely because config is outside the scope of the library. It'd be difficult to make a common api that was both useful and worked consistently across the known logging implementations, which can have pretty complex config rules, multiple appenders, etc. I would welcome a repl-compatible logging implementation. Notice that nothing in
I strongly encourage you to try to provide a new protocol implementation for |
BTW, here's a (entirely untested) snippet of code I cobbled together from what you're doing inside
Calling |
Hi Alexander, thanks for your input - will try get back to you properly in a day or two! |
Hey, sorry about the crazy delay replying to this - ended up coming down with a flu that's had me completely out of action. Appreciate the clarification on the role of Will definitely try get a As an aside, thank you for all your awesome work on |
Okay, looks like it should be possible to add both The biggest issue I can see will be the difference in the way In most cases this difference is relatively inconsequential, and I'm inclined to match My current thinking is to provide the following args to appenders:
Where we hit a problem is in the cases where Timbre actually makes use of the unjoined arguments. For example, both the rate limiter middleware and email appender treat the first logging argument specially (as the hash argument and email subject respectively). There are also cases (e.g. logging to database) where it's useful for appenders to receive the unjoined arguments. Will keep thinking about this and experimenting. Would of course also be very happy to get your input if you have any. Cheers! |
There's an experimental work-in-progress on the Relevant excerpts (as of the current commit): (defmacro log*
"Prepares given arguments for, and then dispatches to all level-relevant
appender-fns."
;; For tools.logging.impl/Logger support
([base-appender-args level log-vargs ns throwable message juxt-fn]
`(when-let [juxt-fn# (or ~juxt-fn (@appenders-juxt-cache ~level))]
(juxt-fn#
(conj (or ~base-appender-args {})
{:instant (Date.)
:ns ~ns
:level ~level
:error? (error-level? ~level)
:log-args ~log-vargs ; NB: no tools.logging support (?)
:throwable ~throwable
:message ~message}))
true))
([base-appender-args level log-args message-fn]
`(when-let [juxt-fn# (@appenders-juxt-cache ~level)]
(let [log-vargs# (vector ~@log-args)
[x1# & xn# :as xs#] log-vargs#
has-throwable?# (and xn# (instance? Throwable x1#))]
(log* ~base-appender-args
~level
log-vargs#
~(str *ns*)
(when has-throwable?# x1#)
(if has-throwable?#
(apply ~message-fn xn#)
(apply ~message-fn xs#))
juxt-fn#)))))
(defmacro logp
"When logging is enabled, actually logs given arguments with level-relevant
appender-fns using print-style args."
{:arglists '([level & message] [level throwable & message])}
[level & sigs]
`(when (logging-enabled? ~level)
(log* {} ~level ~sigs print-str)))
(defmacro logf
"When logging is enabled, actually logs given arguments with level-relevant
appender-fns using format-style args."
{:arglists '([level fmt & fmt-args] [level throwable fmt & fmt-args])}
[level & sigs]
`(when (logging-enabled? ~level)
(log* {} ~level ~sigs format))) and (ns taoensso.timbre.tools.logging
"clojure.tools.logging.impl/Logger implementation"
(:require [taoensso.timbre :as timbre]))
(deftype Logger [logger-ns]
clojure.tools.logging.impl/Logger
(enabled? [_ level] (timbre/logging-enabled? level))
(write! [_ level throwable message]
(timbre/log* {} level [message] ; TODO Better alternatives?
logger-ns throwable message nil)))
(deftype LoggerFactory []
clojure.tools.logging.impl/LoggerFactory
(name [_] "Timbre")
(get-logger [_ logger-ns] (->Logger logger-ns)))
(defn use-timbre []
(alter-var-root clojure.tools.logging/*logger-factory*
(constantly (->LoggerFactory)))) |
Just a quick note about the message, it is a single value, but it does not need to be limited to a string. If an appender knows how to persist, say, a map of values, then it can do so when passed one as a message. I think Sean Corfield is doing something with custom appenders for known message objects and pushing them into a database. |
Just to clarify: this is true only for |
It's true for |
@MerelyAPseudonym Timbre @ataggart Barring any better suggestions, the current implementation is semi-compatible with (timbre/logp :info "a") ; {:message "a" :args ["a"]} goes to appenders
(tools.logging/logp :info "a") ; {:message "a" :args ["a"]} goes to appenders
;; :args necessarily differs
(timbre/logp :info "a" "b" "c") ; {:message "a b c" :args ["a" "b" "c"]} goes to appenders
(tools.logging/logp :info "a" "b" "c") ; {:message "a b c" :args ["a b c"]} goes to appenders
(timbre/log :info {}) ; {:message nil :args [{}]} goes to appenders
(tools.logging/log :info {}) ; {:message nil :args [{}]} goes to appenders
;; :message differs
(timbre/log :info "a") ; {:message nil :args ["a"]} goes to appenders
(tools.logging/log :info "a") ; {:message "a" :args ["a"]} goes to appenders
;; API incompatibility
(timbre/log :info {} {} {}) ; {:message nil :args [{} {} {}]} goes to appenders
(tools.logging/log :info {} {} {}) ; not possible The only one I see as problematic is the 2nd example since it prevents arg-sensitive features like the rate limiter and email appender from working the same way between Timbre and Timbre-via-tools.logging. In any case, some compatibility is nicer than no compatibility and am in no rush to make any decisions about this so still going to take time to think about + experiment. Feedback welcome. Cheers! |
Okay, have decided to go ahead cut a |
Oh my gosh, I apologize for not contributing/following up! (I had been experimenting Gmail filters and hadn't noticed all the updates to this issue) Well, it doesn't seem like there's anything else that needs to be done. And @ptaoussanis, regarding your initial question (about whether it's good enough to just have the extra args concatenated): that probably would have been sufficient, but I didn't know about that functionality at the time. 😐 Thanks for incorporating this feature request, and I'm happy that you see that you feel like the codebase has improved as a result. |
@MerelyAPseudonym No problem, am happy with the outcome either way :-) |
What do you think about providing equivalents to the standard logging-level fns that automatically do formatting?
At least SOME kind of interpolation would be useful. Right now, I have a bunch of logging expressions in a codebase that are all like
(debug (format …message and values to record…))
.The text was updated successfully, but these errors were encountered: