sirupsen / logrus

Structured, pluggable logging for Go.
MIT License
24.3k stars 2.26k forks source link

Improve how data > 64KB is logged #1389

Closed ashmckenzie closed 1 year ago

ashmckenzie commented 1 year ago

This PR extends https://github.com/sirupsen/logrus/pull/1384 to support the desired approach of create new log lines when the log data is greater than 64KB.

The following is a decent test program that demonstrates this working:

package main

import (
    "bufio"
    "strings"
    "time"

    logrus "github.com/sirupsen/logrus"
)

func main() {
    output1 := strings.Repeat("A", bufio.MaxScanTokenSize) + "Z"
    output2 := strings.Repeat("B", bufio.MaxScanTokenSize) + "Z"

    logger := logrus.New()
    writer := logger.Writer()

    writer.Write([]byte("start"))
    writer.Write([]byte(output1))
    writer.Write([]byte(output2))
    writer.Write([]byte("end"))

    writer.Close()

    time.Sleep(500 * time.Millisecond)
}
ashmckenzie commented 1 year ago

WDYT @Luap99, @sirupsen ?

ashmckenzie commented 1 year ago

@Luap99:

I don't know how serious backwards compatibility is taken but we never know if users of logrus depend on the current newline split behaviour which is buffer the write until a newline is found. Note that I personally do not care about the exact behaviour so I can live with either one but this project is used by so many we can never be sure if there are people who depend on the current behaviour.

This is a good point and one I think @sirupsen and the other maintainers need to decide.

I personally would prefer an implementation were we would just get rid of the extra goroutine and pipe implementation and just return a writer that does exactly what you are doing here. This would also removes the race condition that you can see in the test as everything would be single threaded. However because io.PipeWriter is a struct we cannot change the return type it without breaking the logrus API which is even worse.

This sounds like an excellent follow-up refactor issue. Want to pair up on it? 🙂

In any case these are just my thoughts, I wouldn't be opposed to merge this.

Thank-you, we really appreciate your time and input 🙇