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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shain/client logger #23

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 92 additions & 0 deletions client-logger-framework.md
@@ -0,0 +1,92 @@
# Chef Client logging framework refactor

Proposal for refactoring the way Chef Client creates, configures, and uses loggers, with the intention of allow a more modular, extensible log framework.

## Motivation

The current Chef Client logging system is confusing to configure and relatively limited in terms of adding functionality. In the past, people have attempted to extend the logging facilities with varying degrees of success. This change would allow for a simpler, documented way configure logging.

As a secondary motivation, some new logger classes will also be introduced to help with some common log frameworks. For more details on these, see below.

## Overview

In general, the idea here is to create to have fine grained control over the logs that are created, instead of having the client do it for you. (See backwards compatibility information below) Creating a new namespace `Chef::Loggers` allows anyone to easily create a new logger class and add the appropriate configuration elements needed to use it without having to navigate the existing logs.

This is an 'either/or' change, so using this (by defining one or more loggers with `add_logger`) will cause the old style logging to be ignored.

### Configuration

#### (potentially) Deprecated Configurations
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to deprecate any of this functionality in order to make this work. So I'd replace this section with a section talking about how the backwards compat will be achieved.

Currently the Chef Client logger is configured with the following attributes:
(see [cli docs](http://docs.opscode.com/ctl_chef_client.html) and [config.rb docs](http://docs.opscode.com/config_rb_client.html) for more information)
- `--logfile` - represents the IO stream for log output, examples being `STDOUT` and `/var/log/chef` (can also be set in config.rb as `log_location)
<br/>Default - `STDOUT`
- `--force-formatter` - if true, will force anything being sent to `STDOUT` to run through a Chef Client Formatter such as the doc` or minimal` formatters.
<br/>Default - false
- `--force-logger` - if true, will show anything being sent to the logger on `STDOUT`
<br/>Default - false

#### New Configuration Method
`add_logger(log_type, args)` - new method to add and configure a logger that Chef Client will log to.

- `log_type` - represents the class name of the logger to instantiate. This assumes that the logger follows the `Chef::Loggers::LoggerName` pattern. The Chef Client will attempt to find the class, instantiate it with the `args` and add it to the list of registered loggers. **Required**
- `args` - represents a hash of key-value pairs that will be passed in when the logger class is instantiated. This will vary completely on the logger class, which ideally will have the inputs clearly documented. **Defaults to `nil`**

Example of configurations (config.rb)

```ruby
# Complicated logger -
add_logger "ComplexLogger",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have this functionality implemented in mixlib/log, instead of having add_logger we can have enable_logger :SyslogLogger true, { option: "blah"} or enable_logger :WindowsEventLogger true, { option: "blah" } or

loggers [ 
  [ :SyslogLogger, {option: "blah"} ],
  [ :WindowsEventLogger, {option: "blah"} ]
]

This way one can also disable the default loggers cleanly rather than trying to decode the other logging options.

Because add_logger doesn't give one the option to disable the default loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this approach. On the other hand, the default (if no loggers were specified) would be a fallback to using MonoLogger initialized at runtime with STDOUT etc. If you want to disable the loggers I believe you'd just say loggers [ ].

That being said, I like this way better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A problem I have with this is that you're now locked to a specific interface for the initializer for a given logger, E.g., you could have different log classes that are created in very different ways:

logger_a = LoggerTypeA.new(device, option, option2)
logger_a.level = level

logger_b = LoggerTypeB.new(device, option: value, option2: value2, level: desired_log_level)

If we try to standardize these, then you need a wrapper for each supported logger, whereas if users initialize them on their own, we don't have to care what the constructor arguments are, we just assume (or we can explicitly check) that the objects a user gives us provide a certain API (e.g., they respond to :debug, :normal, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was trying to go for with the "args" hash (if I'm understanding you correctly). The only thing that is required in my initial idea is the class name, and the args themselves are totally up to the user to specific (or not specific if they're defaulted)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scotthain The problem is that the method signatures are not compatible. For example:

LoggerTypeA.new(device, option, option2)

This is how the stdlib logger works, it takes one required argument and two optional arguments which are integers (for log rotation stuff)

Contrarily:

logger_b = LoggerTypeB.new(device, option: value, option2: value2, level: desired_log_level)

This hypothetical logger takes one required argument and an (optional) options Hash. So the method signatures are incompatible in that A takes 3 arguments, while B takes 2, and the type of the second argument to A is an Integer that specifies the "shift_age", while the second argument to B is a Hash. To make this work, you'd need an adapter, like:

class StdlibLoggerAdapter
  def initialize(log_dev, options)
    shift_age, shift_size = extract_log_shift_opts_from_opts(options)
    @actual_log_object = Logger.new(log_dev, shift_age, shift_size)
    apply_options_to_logger
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think that is what I'm advocating. We provide the "default" way of initializing a logger adapter (this was my 'args' variable), which then actually is up to the implementer of the particular log device what they need. I may have been using the wrong/confusing terminology for these in the rfc doc and prior comments, which I can fix.

So I guess it would mean adding a new logger requires 3 things:

  • a config entry with appropriate configuration
  • a logger adapter that initializes
  • the logger implementation that the adapter actual initializes

What would alternatives to this be? @danielsdeleo @sersut

{
:log_device => "STDOUT",
:some_variable => "true",
:another_var => "some kind of important log config info"
}

# Simple logger
add_logger "MiniLogger"

# Replicate the default logger from prior to this change
add_logger "ChefLogger"


```
## Logger Information
### New Loggers
Note that these will only be enabled if you explicitly add them with the add_logger method

#### Chef::Loggers::ChefLogger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to change the name of MonoLogger?

New default logger for the Chef Client (essentially the same as MonoLogger, with a new name and location to preserve backwards compatibility). The ChefLogger defaults to `STDOUT` unless you pass in a log_location in the `args`

#### Chef::Loggers::SyslogLogger
TBD - name may change - more information soon -
New logger for output to syslog

#### Chef::Loggers::WindowsEventLogger
TBD - name may change - see [Windows Logging RFC](https://github.com/opscode/chef-rfc/blob/adamed/windows-logging/rfc0002-windows-logging.md) (link may change)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the number of distributions that are moving to systemd, it might be interesting to include a journald logger in this list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @stevendanna this works:
(adding syslog-logger to the Gemfile first)
require 'syslog-logger'
Chef::Log.use_log_devices( [ Logger::Syslog.new("chef-client", 8) ] )

### Writing a logger
Writing a logger should be as simple as creating a new class similar to the following:

```ruby
class Chef
module Loggers
class FreakingAwesomeLogger

def initialize(args)
# set arguments
end
# whatever else is needed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get more crisp in here in terms of the functions one needs to implement and the options one needs to adhere (e.g. log_level) here if we want to enable custom loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - I haven't dug into what those are yet.

end
end
end

```

## Additional Information
### Current Framework
Chef Client currently has a few possible "modes" of running, depending on the situation. There are also currently two config options to help get the desired behavior (force_formatter and force_logger). This would essentially deprecate these modes in favor of highly modular, configurable loggers for whatever purpose.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These options override the default behavior where chef tests if standard out is a TTY to determine if it should enable the use of output formatters (this also adjusts the default log level between :info and :warn). So force_formatter makes chef enable the default formatter when standard out is not a TTY, and force_logger makes chef disable the default formatter when standard out is a TTY.

Side note, while that mostly does what the user expects, there are some cases where it's surprising, like when running chef-client | grep PATTERN. It's been requested that we remove the auto-switching behavior in Chef 12, we should bring that up on the relevant RFC.

### Retaining backwards compatibility
Backwards compatibility is maintained via a simple check where if you have no instances of `add_logger` in your `config.rb`, the client will operate the same as it did prior to this change, defaulting to a `STDOUT` based `MonoLogger` with log level set to :warn.
### Limitations/Unanswered Questions
- This does not allow you to override via command line in the current state, all configuration must be done via config.rb. While it would be possible to add a command line argument that can mimic the `add_logger` functionality (much like the `--format` option mimics `add_formatter`), passing all the required fields to configure a logger may be very complicated for anything but the simplest of logger classes.