plexus / chestnut

Application template for Clojure + ClojureScript web apps
Eclipse Public License 1.0
1.32k stars 99 forks source link

add logger by default #219

Closed featheredtoast closed 7 years ago

featheredtoast commented 7 years ago

Adds a default log setup through onelogger. this dependency no longer is shipped with the newer version of ring.middleware.logger, so we need to explicitly include it.

closes #218

plexus commented 7 years ago

Sorry I didn't have the time to give you better input yesterday. Looking at it now though, I'm not convinced this is the way to go.

From the README RadicalZephyr/ring.middleware.logger :

The main thrust of the changes is to remove the (IMHO) probelmatic onelog library in favor of simply relying on clojure.tools.logging.

And clojure.tools.logging says

At runtime a specific implementation is selected from, in order, slf4j, Apache commons-logging, log4j2, log4j, and finally java.util.logging.

My understanding is that adding onelog will pull in slf4j, which clojure.tools.logging will pick up, but onelog itself is not actually used. So if this is the way to go then including slf4j or log4j directly would be a better solution.

Going forward maybe malcolmsparks/clj-logging-config could be a better choice for us, as it would allow us to move the current logging .properties files into Clojure code.

I don't think this would require changes in ring.middleware.logging, but if it does I'd be happy to put out another version of lambdaisland/ring.middleware.logger.

featheredtoast commented 7 years ago

No worries, I'm not married to onelog myself, just looked for a simple solution for this. I'll poke around at clj-logging-config soon!

featheredtoast commented 7 years ago

Just updated the PR with the clj-logging-config dependency. Confirmed that the ring-logger uses the log4j dependency and correctly prints to log/server.log.

plexus commented 7 years ago

If I understand this correctly then the reason this works is because of a transitive dependency, it's not really using clj-logging-config. I'll merge it though as it's a step in the right direction. I'll look into replacing the current .properties logging config with a .clj version.

plexus commented 7 years ago

Thanks, as always!

aaron_approves