Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
todd-a-jacobs opened this issue Feb 17, 2022 · 7 comments

Comments

@todd-a-jacobs
Copy link
Contributor

Environment

  • Ruby 3.1.0
  • SemanticLogger 4.10.0

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
Copy link

+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
Copy link
Owner

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link

@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.

@reidmorrison reidmorrison changed the title SemanticLogger::Appender::IO throws a warning, but SemanticLogger::Appender::File works as expected Allow stderr and stdout appenders at the same time. (v5 feature request) Nov 6, 2022
@keithrbennett
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants