Provide Output method to Entry

[nvcnvn]

Standar log pakcage have the Output method for Logger http://golang.org/pkg/log/#Logger.Output
Maybe Entry should have that method so it interface exactly with std Logger.

[maxekman]

+1. As an example, not having this makes SetLogger() in go-nsq hard to use.

[sundarv85]

+1. Same as @maxpersson has said. Want to use logrus with go-nsq and Output method is mandatory for it to be used with go-nsq's SetLogger.

[sirupsen]

@sundarv85 @maxpersson why not just use WithFields in go-nsq? Set your logger where you need those fields on every statement somewhere with:

logger := logrus.WithFields(logrus.Fields{
  "id": r.id,
  "topic": r.topic,
  "channel": r.channel
})

And then use it:

logger.Info("OMG messages")
logger.Warn("OOMG too many messages!")

That'll log with all those fields. That's a much cleaner pattern in my opinion.

Although logrus should implement Output() for full backwards compatibility with the stdlogger, but I wouldn't encourage its behaviour (unless I'm missing a case).

[nvcnvn]

The issue is some libs (in this case NSQ Go client) require the specific
"Output" method for their logger interface.
Example: https://godoc.org/github.com/bitly/go-nsq#Conn.SetLogger
Logrus should implement Output method so it can be replacment for std log
package anyway!

On Sun, Mar 15, 2015 at 10:18 PM, Simon Eskildsen [email protected]
wrote:

@sundarv85 https://github.com/sundarv85 @maxpersson
https://github.com/maxpersson why not just use WithFields in go-nsq?
Set your logger where you need those fields on every statement somewhere
with:

logger := logrus.WithFields(logrus.Fields{
"id": r.id,
"topic": r.topic,
"channel": r.channel
})

And then use it:

logger.Info("OMG messages")
logger.Warn("OOMG too many messages!")

That'll log with all those fields. That's a much cleaner pattern in my
opinion.

Although logrus should implement Output() for full backwards
compatibility with the stdlogger, but I wouldn't encourage its behaviour
(unless I'm missing a case).

—
Reply to this email directly or view it on GitHub
#118 (comment).

[sirupsen]

@nvcnvn ya, I agree it should be in, I'm just saying I don't see it as a Logrus best practise.

[nvcnvn]

Hi Simon,
May be I'm not understand your previewous comment correctly.
The reason why we can't use logrus.WithFields is we're not the author of
go-nsq so it will take time to fork then read all go-nsq code and finally
replace all the log lines with logrus.
That will easier if Logrus have Output method.

On Sun, Mar 15, 2015 at 10:29 PM, Simon Eskildsen [email protected]
wrote:

@nvcnvn https://github.com/nvcnvn ya, I agree it should be in, I'm just
saying I don't see it as a Logrus best practise.

—
Reply to this email directly or view it on GitHub
#118 (comment).

[sirupsen]

oooh, that use case makes a lot more sense. Gotcha! ✨

Either way, I'll happily accept a patch for this. I might do this at some point if nobody else does.

[nvcnvn]

Does any one know what is expected behavior of "Output" method Calldepth arg?
Reading out std log package http://golang.org/pkg/log/#Logger.Output say that:
"Calldepth is used to recover the PC and is provided for generality, although at the moment on all pre-defined paths it will be 2."
That make me quite confuse to implement it.

[fdelbos]

+1 mgo use this method as well.

[schickling]

Any news on this?

[robert-uhl]

Got bitten by this today…

[sirupsen]

Can someone write a patch?

[michaeltrobinson]

The way that go-nsq is using log.Output is that they are prepending the log-level to the log string before hand. As a workaround, I wrote a temporary in-between layer that parses the first 3 chars of each log message to decode which log level method to pass it through to in logrus, but that is solution specific to go-nsq.

I think the only way to write a generic log.Output implementation for logrus would be similar to Printf in that it would always log it to INFO level. However, if you then dropped that into go-nsq, you would get log messages out at INFO level, that look like INFO[12412] ERR nsq failed to connect.

[applee]

+1 mgo need it.

[sporkmonger]

@michaeltrobinson I'd be interested in looking at your work-around.

[michaeltrobinson]

@sporkmonger - This is how we are using logrus with go-nsq: https://gist.github.com/michaeltrobinson/4d521cb4267bcf7bda9b0c775df4a440

[sporkmonger]

Thanks for the super-quick response. Looks to me like this is really more of an issue w/ the go-nsq package than w/ logrus.

[michaeltrobinson]

Well, if logrus implemented Output(), then we could have passed an Entry directly into go-nsq's SetLogger(). It wouldn't log things at the right level, but it would at least work.

[sporkmonger]

True.

[gravis]

I don't understand why the MR #390 was closed without being merged :(

[WhisperingChaos]

There are three reasons for implementing Output method:

• It allows the logger to be embedded in an interface and properly report the package name and line number as the calldepth parameter value can be adjusted to skip over the extra call stack entries due to the indirection of the interface's implementation.
• It promotes seamless integration when configuring logrus output format, as options like: Llongfile can be specified using logrus method calls before being encapsulated in an interface and all works as expected. Due to Output's omission, specifying options reliant on the stack, like Llongfile generate references to the wrong package name and line number. Therefore, logrus users must be directed not to use these flags when configuring their logging within an app that wishes to decouple itself from a specific logging implementation.
• It permits creating a simple interface, in my case a single method one, whose implementation can be replaced by any logging package. Golang's logger method set is highly complected : - Why should logging issue a panic?.

[pasztorpisti]

In my opinion logrus should provide a very simple interface that represents its core feature set in the simplest and cleanest way possible. This is why I wouldn't include utility methods like Output.

I've recently mentioned (here and here) that Logger.Writer should also be excluded from the interface of the core logger objects.

Most of these issues are about implementing a logrus-to-something adapter. Whether that something is a Writer, an Output interface or an interface similar to that of the standard logger: it can be implemented as an adapter object between logrus and the client code (client -> adapter -> logrus). The core of logrus doesn't have to know about this.

The problem discussed here (Output method) can be solved with something like this:

package main

import (
	"github.com/sirupsen/logrus"
	"log"
	"runtime"
)

func main() {
	o := &logrusOutput{
		entry: logrus.StandardLogger().WithField("logger", "nsq"),
	}
	o.Output(1, "woof")

	log.SetFlags(log.Llongfile)
	log.Output(1, "meow")
}

type logrusOutput struct {
	entry *logrus.Entry
}

func (o *logrusOutput) Output(calldepth int, s string) error {
	// Since logrus doesn't support reporting filenames and line numbers
	// we could simply ignore calldepth or we can add them as fields.
	if _, file, line, ok := runtime.Caller(calldepth); ok {
		o.entry.WithFields(logrus.Fields{
			"file": file,
			"line": line,
		}).Info(s)
		return nil
	}

	o.entry.Info(s)
	return nil
}

Similarly to the above logrusOutput one could implement a type that has an interface similar to that of the standard logger and forwards everything to logrus (without logrus knowing about the whole thing).

A problem like this isn't a good reason to put extra code into the logrus core. It would be needless liability. Some adapters could be provided as a separate package that isn't a dependency of logrus core.

Here is another example that creates a new standard logger on top of logrus:

package main

import (
	"github.com/sirupsen/logrus"
	"log"
)

func main() {
	w := &log2LogrusWriter{
		entry: logrus.StandardLogger().WithField("logger", "log2Logrus"),
	}
	logger := log.New(w, "", 0)
	logger.Output(1, "woof")
}

// log2LogrusWriter exploits the documented fact that the standard
// log pkg sends each log entry as a single io.Writer.Write call:
// https://golang.org/pkg/log/#Logger
type log2LogrusWriter struct {
	entry *logrus.Entry
}

func (w *log2LogrusWriter) Write(b []byte) (int, error) {
	n := len(b)
	if n > 0 && b[n-1] == '\n' {
		b = b[:n-1]
	}
	w.entry.Info(string(b))
	return n, nil
}

[WhisperingChaos]

I certainly agree with @pasztorpisti that certain logging interfaces/APIs are unnecessarily complex and that one could, as demonstrated by pasztorpisti's post, code around Output's absence. However, logrus specifically states that it's "completely API compatible with the standard library logger" and by doing so, a developer can easily replace golang's logger with its implementation by changing a dependent package's 'log" import statement. No further coding is required, unless of course there's a logrus feature that you wish to exploit. I'm also confident, that most developers that chose to replace golang's logger did so seamlessly but as evidenced by this thread, there were those of us who attempted this easy replacement method and encountered a compilation error. This error exposed the absence of golang's public Output() function/method from logrus API. Therefore the logrus API is not completely API compatible as asserted.

Inconsistencies between a project's stated intent and an actual experience using it beget questions like:

• Are there any other missing functions?
• Perhaps the constant values differ between the two packages. Therefore, values sourced from a config file assuming the use of golang's log package don't elicit the same behavior in logrus?

It boils down to this: if logrus wants to be "completely API compatible..." then implement Output, otherwise, advertise that they are "almost API compatible..." and identify the differences.

[pasztorpisti]

@WhisperingChaos Your point is completely valid regarding the claims of the documentation.

Being std log compatible might have some "marketing" related value because "compatible" sounds to be a good/safe choice. However from a technical perspective its usefulness is questionable. Providing backward compatibility interfaces as separate adapters is a better design. And in a lot of cases there is no need for adapters:

My approach to adapting logrus in an existing codebase probably wouldn't include replacing standard log imports with aliased logrus imports (especially in a larger project).

The first (and last) step would be redirecting all standard logs in main() to logrus using the above log2LogrusWriter and log.SetOutput. Tadaaaa, it's done. Now everything is logged with logrus including the external dependency packages if they are using log. Those who need an std log compatible logger (e.g.: an Output method) can continue to use the std log directly or they can create new standard logger instances using the log2LogrusWriter.

In the future I'd insert new log statements by importing logrus without import alias and calling it directly.

[mrclayman]

Like @WhisperingChaos said, if the library claims to be completely API-compatible with the standard logging package, then it really should be that. I was hoping to use logrus with mgo (MongoDB client library), which uses standard log package, but cannot since logrus misses the Output method that the log.Logger interface declares.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.