sirupsen / logrus

Structured, pluggable logging for Go.
MIT License
24.59k stars 2.27k forks source link

logrus_syslog ignoring log level #1369

Closed tommyblue closed 1 year ago

tommyblue commented 1 year ago

Following the docs at https://github.com/sirupsen/logrus/#hooks I'm trying to use an external syslog server to send only WARNING (and above) logs there, while sending debug to local logs. Unfortunately it doesn't seem to work. The external syslog server receives whatever I setup with log.SetLevel() instead of the priority argument passed to logrus_syslog.NewSyslogHook.

Am I doing anything wrong here? Maybe I misinterpreted the docs?

package main

import (
    log "github.com/sirupsen/logrus"
    logrus_syslog "github.com/sirupsen/logrus/hooks/syslog"
)

func main() {
    log.SetLevel(log.DebugLevel)

    hook, err := logrus_syslog.NewSyslogHook("tcp", "localhost:5140", syslog.LOG_WARNING, "test")
    if err != nil {
        panic(err)
    }
    log.AddHook(hook)

    // These 2 should go both to local and remote logs
    log.Warn("ciao with warn")
    log.Error("ciao with error")

    // These should only go to local logs and not to external syslog,
    // but instead they're shown there too :(
    log.Debug("ciao with debug")
    log.Info("ciao with info")
}
thaJeztah commented 1 year ago

I only had a quick glance, but would have to look up what the intended design was for this.

Having a look at the code, I see that the loglevel passed to NewSyslogHook is passed through to syslog.Dial(); https://github.com/sirupsen/logrus/blob/a448f8228b920021d792e0767626068db5f0e38d/hooks/syslog/syslog.go#L23-L26

Where syslog.Dial() uses that level for each log message sent;

Each write to the returned writer sends a log message with the facility and severity (from priority) and tag

However, the Hook itself advertises what log-levels it supports, which is "all log levels" in this case; https://github.com/sirupsen/logrus/blob/a448f8228b920021d792e0767626068db5f0e38d/hooks/syslog/syslog.go#L53-L55

When the hook is added, that information is used to add a hook for each level it supports; https://github.com/sirupsen/logrus/blob/a448f8228b920021d792e0767626068db5f0e38d/hooks.go#L16-L22

Which, when hooks are triggered, is used to fire the hook; https://github.com/sirupsen/logrus/blob/a448f8228b920021d792e0767626068db5f0e38d/hooks.go#L26-L34

From the above, this either was by design (hooks always are tiggered for any level they support), or this was an oversight, and either the LevelHooks.Add() should only add hooks for levels above the logger's level, or entry.firehooks() should exclude it when firing; https://github.com/sirupsen/logrus/blob/a448f8228b920021d792e0767626068db5f0e38d/entry.go#L274-L283

thaJeztah commented 1 year ago

Would probably be worth (if you have time for that) to look into history to find what the intended design was here (and if it was by design, or an oversight).

tommyblue commented 1 year ago

yup, implementing the interface changing what Levels() returns does the trick:

package main

import (
    "log/syslog"

    log "github.com/sirupsen/logrus"
    logrus_syslog "github.com/sirupsen/logrus/hooks/syslog"
)

type testHook struct {
    *logrus_syslog.SyslogHook
}

func (h *testHook) Levels() []log.Level {
    return []log.Level{log.WarnLevel}
}

func main() {
    log.SetLevel(log.DebugLevel)

    hook, err := logrus_syslog.NewSyslogHook("tcp", "localhost:5140", syslog.LOG_WARNING, "test")
    if err != nil {
        panic(err)
    }

    log.AddHook(&testHook{hook})

    //...
}

The original behaviour seems very strange to me. You can specify a level but then it is ignored 🤔 But at the same time is seems strange nobody noticed this until now... That's why I was thinking I was misusing it

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity.

github-actions[bot] commented 1 year ago

This issue was closed because it has been inactive for 14 days since being marked as stale.