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

Does it support a rolling file appender? #22

Open
ieugen opened this issue Nov 8, 2022 · 6 comments
Open

Does it support a rolling file appender? #22

ieugen opened this issue Nov 8, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@ieugen
Copy link
Contributor

ieugen commented Nov 8, 2022

For long running apps that write to disk it might be good to have rolling file appenders.

There is a good overview of these in https://www.baeldung.com/java-logging-rolling-file-appenders .

I'm also wondering if it's possible to use existing slf4j appender implementations in dialog ?!
This might be hard if they are bundled with slf4j implementation - like logback has it's appenders.
But there are appenders out there that might not be.

@ieugen ieugen changed the title Does it support a rotating file appender ? Does it support a rolling file appender? Nov 8, 2022
@greglook
Copy link
Contributor

Not yet, but certainly open to adding that capability to the file output writer.

@greglook greglook added the enhancement New feature or request label Nov 15, 2022
@ieugen
Copy link
Contributor Author

ieugen commented Nov 17, 2022

I've experimented with the output writer and I got it working using the FileAppender.
This should work ok with with RollingFile appender as well.
It is required to (.stop appender) during logger re-loading.
FileAppender and related use ch.qos.logback.core.OutputStreamAppender .
And stop will close the underlying stream.

@greglook : does dialog offer any helpers to implement a lifecycle for this or do we need to roll our own ?

(ns dialog.examples.logback
  (:require [dialog.logger :as log]
            [dialog.format.simple :as simple])
  (:import (java.nio.charset StandardCharsets)
           (ch.qos.logback.core ContextBase FileAppender)
           (ch.qos.logback.core.encoder Encoder)))

(defn encoder
  "In Logback, an encoder converts a log event to byte array."
  [output]
  (let [formatter (simple/formatter output)]
    (reify Encoder
    ;; no header 
      (headerBytes [_this] nil)
      (encode [_this event]
        (let [event-str (formatter event)
              event-str (str event-str "\n")]
          (.getBytes event-str StandardCharsets/UTF_8)))
      (footerBytes [_this] nil))))

(defn file-appender
  "Create a ^FileAppender from dialog output map."
  [opts]
  (let [{:keys [appender-name file-name context encoder
                immediate-flush]
         :or {appender-name "file-appender"
              file-name "dialog-file-appender.log"
              immediate-flush true}} opts
        appender (doto (FileAppender.)
                   (.setEncoder encoder)
                   (.setName appender-name)
                   (.setFile file-name)
                   (.setContext context)
                   (.setImmediateFlush immediate-flush)
                   (.start))]
    appender))


(defn file-appender-logger
  "dialog function to write log events using ^FileAppender."
  [output]
  (let [context (ContextBase.)
        encoder (encoder output)
        output (assoc output
                      :context context
                      :encoder encoder)
        appender (file-appender output)]
    (fn write-event
      [event _message]
      (.doAppend appender event))))

(comment
  (log/initialize!)
  (log/info "Hello")
  )

dialog.edn configuration file

{:level :debug
 :levels {"vault" :info}
 :outputs {:logback {:type dialog.examples.logback/file-appender-logger
                     :format :simple
                     :timestamp :short
                     :file-name "target/dialog-logback.log"}}}

deps.edn

{:paths ["src" "resources"]
 :deps {;; run clj -X:deps prep 
        amperity/dialog {:local/root "../../"}
        ch.qos.logback/logback-core {:mvn/version "1.2.6"}}
}

@greglook
Copy link
Contributor

does dialog offer any helpers to implement a lifecycle for this or do we need to roll our own ?

Ah, an interesting question - given the original goal of simplicity and basic stdout/file outputs, this wasn't a concern. I think there's room to move to a writer protocol which could provide this kind of advanced capability. 🤔 By "logger reloading" I'm assuming you mean a full reinitialization, not just changing the current logger levels.

It seems like this stop protocol method should probably also be invoked by a JVM shutdown hook.

@ieugen
Copy link
Contributor Author

ieugen commented Nov 17, 2022

Thanks. Probably this will work okish by adding a call to Runtime.getRuntime().AddShutDownHook() .
I don't imagine people will re-initialize it that often in production.
It's hacky and a known issue so far.

One way to do it now would be to register all the initialized appenders in an atom and make sure to stop them before / after initialize! .
But it's not pretty without support in the library.

If you have any ideas to make this work while keeping it simple that would be great.

I would imagine that the output map could define a set of special keys like :dialog.logger.hooks/output-init, :dialog.logger.hooks/output-start, :dialog.logger.hooks/output-stop .

Then initialize! could check for those functions and call them at specific points ?!
(I did not put a lot of thought in this :) )

I'm going to let the issue open for a bit more time so I can test RollingFileAppender, but IMO it's good to know we can reuse logback appenders.

@greglook
Copy link
Contributor

I was thinking of something along the lines of:

(defprotocol Writer

  (write-event
    [writer event message]
    "Write an event with a formatted message to the output.")

  (close
    [writer]
    "Shut down the output writer."))

Then to support the current use-cases in the library:

(extend-protocol Writer

  clojure.lang.Fn

  (write-event
    [f event message]
    (f event message))

  (close
    [f]
    nil))

Then of course there would need to be some logic in the initialize! call to check whether there was already a populated configuration map, and call close on each writer.

It occurs to me that one way you could handle this with the current logic is to provide a custom :init function which does the FileAppender construction and adds those to each of the :outputs maps, then your output :type function could use those to construct the writer function. That would at least preserve references to the appenders themselves for your custom stop call to use before it invokes dialog.logger/initialize! again. 🤔

@ieugen
Copy link
Contributor Author

ieugen commented Nov 19, 2022

Thanks. I'll focus on delivering sentry first and do the migration and then maybe give this a try.

It occurs to me that one way you could handle this with the current logic is to provide a custom :init function which does ...

This might work if you build your app with this in mind.
If you are on the devops side and just want to configure things without having access to source, then only the protocol option will work, provided there is a library of appenders that implement Writer .

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

No branches or pull requests

2 participants