Slf4jLogConsumer improvements #1986
Conversation
toadzky
commented
Oct 18, 2019
toadzky
commented
- Adds support for MDC
- Uses log levels based on output type
| logger.info("{}{}", prefix.isEmpty() ? "" : (prefix + ": "), utf8String); | ||
| break; | ||
| case STDERR: | ||
| logger.error("{}{}", prefix.isEmpty() ? "" : (prefix + ": "), utf8String); |
There was a problem hiding this comment.
This feels a tad risky; STDERR isn't solely used for 'error' output in practice, so it might cause confusion to map it to the logger's error level.
I think it's one of those situations where there's no right answer...
There was a problem hiding this comment.
i'm not sure there would be confusion. standard out and standard error are different output streams. IMO it's more confusing to have them on the same log level because they are different streams.
There was a problem hiding this comment.
Well, output appearing on standard error can encompass any kind of diagnostic output and is not necessarily an error - the severity is ambiguous. Logger error level really is for errors.
I don't think we should ascribe a meaning that isn't necessarily there, which is why the current implementation is as it is.
There was a problem hiding this comment.
it's literally the standard error stream. you have ascribed your own meaning to it by classifying something with a different name and purpose as normal output.
There was a problem hiding this comment.
That's the name, but the specification is for it to carry diagnostic output, and that's what it is used for in real life. I don't think we should by default treat it differently from stdout, which is why they are both currently mapped to the same log level.
How about this: we could have an opt-in parameter to control whether stderr is mapped to error level logs. This way, people can choose what's best for them according to how the specific container behaves.
| return this; | ||
| } | ||
|
|
||
| public Slf4jLogConsumer separateOutputStreams() { |
There was a problem hiding this comment.
| public Slf4jLogConsumer separateOutputStreams() { | |
| public Slf4jLogConsumer withSeparateOutputStreams() { |
|
Thanks for making the change - I just have one final tweak request and then I think we're good to merge. |
toadzky
force-pushed
the
slf4j-log-consumer-improvements
branch
from
473e930
to
be5eb82
Compare
|
Thanks @toadzky - will merge |
