reidmorrison / semantic_logger

Semantic Logger is a feature rich logging framework, and replacement for existing Ruby & Rails loggers.
https://logger.rocketjob.io/
Apache License 2.0
873 stars 124 forks source link

Allow stderr and stdout appenders at the same time. (v5 feature request) #203

Open todd-a-jacobs opened 2 years ago

todd-a-jacobs commented 2 years ago

Environment

Problem Statement

Not Allowed with IO Appender

SemanticLogger::Appender::IO seems to be a special case where you can't have two IO appenders, even if you use filtering to separate the output streams based on severity level or other criteria. As an example:

stdout_filter = proc { %i[trace debug info error].include? _1.level }
stderr_filter = proc { %i[warn fatal unknown].include? _1.level }

SemanticLogger.add_appender(io: $stderr, formatter: :color, level: :warn, filter: stderr_filter)
SemanticLogger.add_appender(io: $stdout, formatter: :color, level: :trace, filter: stdout_filter)

2022-02-16 23:47:30.569633 W [55686:47380] SemanticLogger::Appenders -- Ignoring attempt to add a second console appender: SemanticLogger::Appender::IO since it would result in duplicate console output.

Allowed with File Appender

However, if you use SemanticLogger::Appender::File instead of IO, SemanticLogger doesn't complain and works as expected:

stdout_filter = proc { %i[trace debug info error].include? _1.level }
stderr_filter = proc { %i[warn fatal unknown].include? _1.level }

SemanticLogger.add_appender(file_name: "/dev/stderr", formatter: :color, level: :warn, filter: stderr_filter)
SemanticLogger.add_appender(file_name: "/dev/stdout", formatter: :color, level: :trace, filter: stdout_filter)

Since I can write to the output streams as named pipes, why won't SemanticLogger allow me to define two separate IO streams? While the filename workaround is fine, it doesn't seem like a useful distinction here.

firxworx commented 2 years ago

+1 this is relevant to my use-case on a current project and in general re common industry logging practices re outputting to stdout + stderr.

reidmorrison commented 2 years ago

A common problem several end users have run into is that sometimes multiple stdout/stderr appenders are being created automatically. Newer versions of Rails have the same check and prevent a second instance from being created.

We could create a middle of the ground solution that only allows one stdout and one stderr output?

This does not prevent someone writing the same output to stderr and stdout, but usually the duplicate should be to the same output (stdout/stderr).

Easy enough to split the logic and not group stderr and stdout into the same bucket.

keithrbennett commented 2 years ago

@reidmorrison Yes, that makes sense to me, it would be helpful to be able to log to both stdout and stderr, and I think that's what @todd-a-jacobs and @firxworx were suggesting.

keithrbennett commented 2 years ago

@todd-a-jacobs I suggest changing the title of this issue to something like:

Can't Have Appenders for Both stdout and stderr

todd-a-jacobs commented 2 years ago

@keithrbennett said:

I suggest changing the title of this issue to something like:

Can't Have Appenders for Both stdout and stderr

But you can have appenders for both, even today. There's nothing stopping anyone from having separate appenders for stdout and stderr. The issue is that you have to specify them as file/device streams using SemanticLogger::Appender::File rather than using the SemanticLogger::Appender::IO classes. Assuming you have /dev/stdout or /dev/stderr (often created by the shell rather than the OS on some systems) then you can do it, but if you don't have the devices for any reason (e.g. running /usr/bin/ruby app.rb on a distro that doesn't automatically include the console streams as devices) then you have problems.

firxworx commented 2 years ago

@todd-a-jacobs thanks for sharing -- interesting insight -- I think you raise a good case where the File approach could have issues.

+1 @reidmorrison suggestion for a more granular restriction that allows 1 of each stderr + stdout. I think it is sort of is in line with developer expectation when looking at SemanticLogger + the docs (at least in my case and I gather for others as well) and that it provides flexibility in terms of options.

I think perhaps the docs might benefit from coverage of this particular use-case: across the dev + devops landscape its a common practice / technical requirement to split logs between stdout and stderr based on the log level / severity and in a business environment there can be dependencies on this behaviour e.g. monitoring/metrics/events/alerts/etc.

keithrbennett commented 1 year ago

I'm not familiar with the risk of having multiple appenders to the console added automatically. By whom/what? Can someone elaborate on that? My gut feeling is that this could be prevented or overridden by the developer, no?

If it's practical, I think the simplest solution would be to remove the restriction altogether...especially since it can be circumvented anyway, using the device/file logger, and other ways as well.

I may be way off though, maybe it is important to compel the developer to take extra steps to override this constraint.

By the way, another way to circumvent this restriction is to subclass Appender::IO and override the console_output? method to return false. (The test is done in Appenders.add.) Then the appender would need to be specified in the SemanticLogger.add_appender call using appender: instead of io:. This is truly a kludge though because it relies on the implementation of Appenders.add using console_output? in its test, and that could change. This solution is inferior to the flie/device solution, but could be used where that solution is not available.