amperity / dialog

Simple just-enough logging library for Clojure
MIT License
39 stars 2 forks source link

Support event filtering per output #29

Closed ieugen closed 2 years ago

ieugen commented 2 years ago

Hi,

I think for some use-cases, event filtering should be done per appender. I noticed event filtering is done globally (see example in code) .

I remember logback / log4j allows setting the event filtering per appender. This is explained here for log4j: https://logging.apache.org/log4j/2.x/manual/filters.html

dialog.logger ns doing global filtering with enabled?

(defn log-event
  "Pass an event into the logging system."
  [event]
  (when-not config
    (initialize!))
  (when-let [event (and (string? (:logger event))
                        (keyword? (:level event))
                        (enabled? (:logger event) (:level event))
                        (-> event
                            (apply-defaults)
                            (apply-middleware (:middleware config))))]
    (run!
      (fn write-event
        [[id output]]
        (when-let [event (apply-middleware event (:middleware output))]
          (write-output! id output event)))
      (:outputs config))))
ieugen commented 2 years ago

Specifically for sentry, there are inbound filters: https://docs.sentry.io/platforms/java/configuration/filtering/ . But this is still an issue for other appenders: you might want some levels to console and some to file

greglook commented 2 years ago

You can do this by adding output-specific middleware which returns nil for the events you want to drop. I don't think the complexity of output-specific levels is worth adding to the core implementation, given this ability.

ieugen commented 2 years ago

thanks @greglook , I missed this line in the config:

Top-level middleware will be applied to all events; middleware may also be applied to individual outputs for more specific processing.

Probably because it's not in the example dialog.edn that I got :) .

ieugen commented 2 years ago

Discussion on clojurians slack

Responded in the ticket, but you can already do this with output middleware.

Eugen implementing filtering via middleware would work but the tooling to do so is either private or designed to work with a single instance. For example enable?

(defn enabled?
"True if the given logger is enabled at the provided level."
[logger level]
(Level/isAllowed
(Level/ofKeyword (get-level logger))
(Level/ofKeyword level))) 
and get-level
(defn get-level
"Get the current level setting for a logger. If no logger name is provided,
this returns the root logger's level."
([]
(or (:level config) :info))
([logger]
(or (get @level-cache logger)
(let [level (or (match-block logger)
(match-level logger)
(get-level))]
(swap! level-cache assoc logger level)
level))))

so this shuld probably be either enhanced in dialog or re-implemented

Eugen implementing filtering via middleware would work but the tooling to do so is either private or designed to work with a single instance. For example enable?

(defn enabled?
"True if the given logger is enabled at the provided level."
[logger level]
(Level/isAllowed
(Level/ofKeyword (get-level logger))
(Level/ofKeyword level))) 

and get-level

(defn get-level
"Get the current level setting for a logger. If no logger name is provided,
this returns the root logger's level."
([]
(or (:level config) :info))
([logger]
(or (get @level-cache logger)
(let [level (or (match-block logger)
(match-level logger)
(get-level))]
(swap! level-cache assoc logger level)
level))))

so this shuld probably be either enhanced in dialog or re-implemented

Eugen I would reopen the issue with this information and see where it goes. WDYT?

Greg Look ah, perhaps I was a little too terse in my response - there is only a single global set of logger levels defined, so you'd have to implement customized logic in the middleware to track what you wanted for "level overrides" on that output

Greg Look we primarily use that feature to suppress certain logs from going to one output or another - i.e. our code goes to STDOUT, spark internal logs go to STDERR

Eugen so if I want an event to go to one logger and not the other I would have to leave the global filter empty and add filters to each loggers (or just the logger I want to block the events) ? (edited)

Eugen if that is the case I would like to reuse some of dialog functionality regarding loggers, matching and filtering - is that possible? (I just started looking into this)

Greg Look yeah currently I think the global levels would need to allow events to be reported to all outputs, then middleware in each output would have to check those loggers against the defined per-output levels you wanted :thinking_face:

Greg Look open to making it easier to reuse the various logic pieces, but a bunch of the code is built around the idea of a central level cache so that we can stop handling logging mesages as early as possible

Greg Look since that was the source of a big perf hit we ran into with timbre

Eugen ok. I will see what I can do. instead of a single global cache, maybe named caches can be used ?

Greg Look ah, perhaps I was a little too terse in my response - there is only a single global set of logger levels defined, so you'd have to implement customized logic in the middleware to track what you wanted for "level overrides" on that output

Greg Look we primarily use that feature to suppress certain logs from going to one output or another - i.e. our code goes to STDOUT, spark internal logs go to STDERR

Eugen so if I want an event to go to one logger and not the other I would have to leave the global filter empty and add filters to each loggers (or just the logger I want to block the events) ? (edited)

Eugen if that is the case I would like to reuse some of dialog functionality regarding loggers, matching and filtering - is that possible? (I just started looking into this)

Greg Look yeah currently I think the global levels would need to allow events to be reported to all outputs, then middleware in each output would have to check those loggers against the defined per-output levels you wanted :thinking_face:

Greg Look open to making it easier to reuse the various logic pieces, but a bunch of the code is built around the idea of a central level cache so that we can stop handling logging mesages as early as possible

Greg Look since that was the source of a big perf hit we ran into with timbre

Eugen ok. I will see what I can do. instead of a single global cache, maybe named caches can be used ? I will reopen the issue and add the above