Level 1 and 2+ should map to corresponding Logrus Debug and Trace levels

[longsleep]

It would be great if this library would map level 0 to logrus.Info, level 1 to logrus.debug and all higher levels to logrus.trace to preserve the log level indicator in logrus output. That would make instant adoption of existing logrus code easier and impact free.

[bombsimon]

Hello, thanks for the report!

I thought this was how it was implemented, is it not? Right now, V(2) and up is only visible with trace level, V(1) with debug level and V(0) with info level. Is this not what you mean?

Have a look at this example:

package main

import (
	"fmt"

	"github.com/bombsimon/logrusr/v2"
	"github.com/sirupsen/logrus"
)

func main() {
	testWithlevel(logrus.InfoLevel)
	testWithlevel(logrus.DebugLevel)
	testWithlevel(logrus.TraceLevel)
}

func testWithlevel(level logrus.Level) {
	logrusLog := logrus.New()
	logrusLog.SetLevel(level)
	log := logrusr.New(logrusLog)

	log.V(0).WithName(level.String()).Info("logging v0")
	log.V(1).WithName(level.String()).Info("logging v1")
	log.V(2).WithName(level.String()).Info("logging v2")

	fmt.Println("---")
}

This will output the following which I think is correct.

INFO[0000] logging v0    logger=info
---
INFO[0000] logging v0    logger=debug
INFO[0000] logging v1    logger=debug
---
INFO[0000] logging v0    logger=trace
INFO[0000] logging v1    logger=trace
INFO[0000] logging v2    logger=trace
---

[longsleep]

Thanks for checking. Indeed I confirm that the example you posted is not correct. The lines logged with V1 should show up as DEBUG not as INFO.

[bombsimon]

The background of logr is to only have Info and Error logs, which I also think is explained well in the linked article from the logr readme.

Further down in the readme you'll find both the section Why V-levels? and Why not named levels, like Info/Warning/Error? explaining this.

So basically the key concept is to only have Info and Error logs and control the chattiness with V-levels. With that in mind I think that logrusr should only ever log either Error or (if the logger is enabled based on logrus severity) Info logs since it doesn't make sense to call Info() but have the log show up as anything else.

[longsleep]

Yes I understand - still why not emulate logrus behavior by deriving from the level similar to what is described in https://github.com/go-logr/zapr#increasing-verbosity for zap and https://github.com/go-logr/zerologr#implementation-details for zerolog. I think whatever logging system one chooses, the logr levels should be translated as close as possible with modules like this. We cannot change that logrus has debug and trace levels and they can be easily emulated by level 1 and level 2+ not only for the actual enabled check bit also on the actual output side so that logrus getting triggered via logr actually behave similar like native logrus logging. Maybe I got it wrong but isn't that the purpose of this module?

[bombsimon]

Thanks for the examples! Since they both come from the maintainers of logr it looks like I'm the one that got this wrong. I thought given the background the idea was to think about severity as a relative thing without named levels but as you showed that's not really the case.

I guess the expected output given the above examples would be this:

INFO[0000] logging v0    logger=info
---
INFO[0000] logging v0    logger=debug
DEBU[0000] logging v1    logger=debug
---
INFO[0000] logging v0    logger=trace
DEBU[0000] logging v1    logger=trace
TRAC[0000] logging v2    logger=trace

[bombsimon]

Since this is a breaking change I'll draft a new major release but I want to add some more things before doing that as well!

[longsleep]

I guess the expected output given the above examples would be this:

Yes - thank you 👍

[longsleep]

Since this is a breaking change I'll draft a new major release but I want to add some more things before doing that as well!

Sure, i don't see much of a "break" in the sense that something will stop working because of this. To me this seems more like a bug fix.

[bombsimon]

Since this is a breaking change I'll draft a new major release but I want to add some more things before doing that as well!

Sure, i don't see much of a "break" in the sense that something will stop working because of this. To me this seems more like a bug fix.

You never know peoples workflows and how they work with logs. If someone has a workflow where they filter on log severity this would break filters expecting info, or a filter that only looks for debug, so in that sense it's a breaking change.