stripe / veneur

A distributed, fault-tolerant pipeline for observability data
MIT License
1.73k stars 175 forks source link

Logrus configuration #279

Open hermanbanken opened 6 years ago

hermanbanken commented 6 years ago

By default, logrus outputs to stderr. When running in GCE with the Stackdriver agent, stderr lines end up with severity 'warning', even when the level in the text-format is 'Info'.

As the logrus readme says this is how it is supposed to be used:

  if Environment == "production" {
    log.SetFormatter(&log.JSONFormatter{})
  } else {
    // The TextFormatter is default, you don't actually have to do this.
    log.SetFormatter(&log.TextFormatter{})
  }

Can we integrate something like that in Veneur? That way I believe stackdriver picks up the log level. I'm not sure though. In either case, stuff like "Completed flush to Datadog" and "Completed forward to upstream Veneur" should go to stdout, right?

ChimeraCoder commented 6 years ago

Thanks for pointing this out!

I don't feel particularly strongly one way or the other about distinguishing between STDOUT and STDERR. By default, both daemontools and systemd capture both in the same stream, so it doesn't really make a difference for us. Though if those are semantically different on GCE, we can figure out how to handle that.

I'm not familiar with the Stackdriver agent on GCE. It looks like the Formatter is used to change the structure of log lines (e.g. adding a timestamp), but it returns the bytes which then get output to logger.Out.

It sounds like what you're saying, then, is that instead of using os.Stderr, you'd like to have a different io.Writer, that does something like:

func (w *MyWriter) Write(p []byte) (int, error) {
    switch level := levelFromLine(p); level {
    case logrus.PanicLevel, logrus.FatalLevel, logrus.ErrorLevel:
        return os.Stderr.Write(p)

    default:
        return os.Stdout.Write(p)
    }
}

Is that correct?

hermanbanken commented 6 years ago

Well, I believe stackdriver does a “best guess”, while not interpreting things like strings that are not JSON. So:

So two changes come to my mind:

  1. make it possible to output using the logrus json formatter
  2. use stderr/info based on the level

I think number 1 should at least be fixed for GCE users, and it’s definitely nice for users of Logstash / other log aggregators too.

ChimeraCoder commented 6 years ago

Ah, I see. So even without implementing a separate writer to route between STDOUT and STDERR, providing JSON-formatted logs would solve this issue?

That should be straightforward for us to do. I'll see about (2) as well. It's mildly awkward to read the prefix of each line twice (once to parse the level, and once to actually log it), but Veneur doesn't have large logging volume.

hermanbanken commented 5 years ago

The problem we had was that GCE's default fluentd configuration parses everything on stderr as an ERROR/CRITICAL, which causes annoyances (false positive email alerts etc). My previous workaround involved wrapping the Docker build of stripe/veneur: not ideal.

The workaround that I implemented today was simply to use this Docker (compose/k8s spec) yaml:

  command: ["/bin/sh", "-c", "veneur -f /etc/veneur/config.yaml 2>&1 | awk '/level=(error|warning)/ { print  > \"/dev/stderr\"; next; }; 1'"]

which will redirect everything matching level=(error|warning) to stderr and the rest to stdout.

hermanbanken commented 5 years ago

Maybe add that as a suggestion to the readme... if you want to.