expand logging capabilities or create output interface/plugins

[dbarrosop]

The idea is to avoid the pattern:

	results, err := gr.RunSync(
		&task.RemoteCommand{Command: "ip addr | grep \\/24 | awk '{ print $2 }'"},
	)
	if err != nil {
		log.Fatal(err)
	}
	if err := output.RenderResults(os.Stdout, results, true); err != nil {
		log.Fatal(err)
       }

To do that we could either modify existing Logging interface or create a new one like:

type Output interface {
    PreHost(Context, Host, Task)            // Called before executing a task on a host
    PostHost(Context, Host, Task)           // Called before executing a task on a host
    PreTask(Context, Task)                  // Called before executing the task on any host
    PostTask(Context, Task)                 // Called after executing the task on all the hosts
    Success(Context, TaskInstanceResult)    // Called if the task succeeded
    Fail(error)                             // Called if the task failed
}

I think that extending the logging interface should be enough and different plugins could do different things. For instance, a logrus plugin could just ignore those method (no ops) and implement Info, Error, etc., while a RenderResult plugin could do the opposite; implement logging like methods like Info and Error and implement instead Host, Task, Success and Fail.

To make this even more useful, it'd be great to allow multiple output loggers.

An alternative would be to leave things as they are and add a callback to RunAsync but I think there is value on the output interface as it will simplify the overall workflow and delegate responsibility to the output plugin.

I also think it's general enough that users could leverage the pattern to create their own "event based" workflow. For instance, your "Output" plugin could update a database on Success with data the gathered on the task, and send notifications to slack on Fail, PreTask and PostTask.

[nleiva]

Need to think about this one. Just wanted to mention I would try to keep the logging interface as simple as possible, so you can plug in the logger of your choice. In fact, we could even just leverage goph/logur interface that comes with out-of-the-box integration with most of the Go logging alternatives (logrus, go-kit, zap, etc.).

type MyLogger interface {
	Trace(msg string, fields ...map[string]interface{})
	Debug(msg string, fields ...map[string]interface{})
	// ...
	WithFields(fields map[string]interface{}) MyLogger
}

[dbarrosop]

Maybe it's worth keeping them separate, not sure. The idea is to make processing tasks asynchronously easier and to make it more flexible. Right now you need to call RunAsync and you can only process later on results in your typical block:

for {
		select {
		case res, ok := <-results:
			if !ok {
				// channel is closed
				return
			}
			if res.Err() != nil {
				fmt.Printf("ERROR: %s: %s\n", res.Host().Hostname, res.Err().Error())
			} else {
				fmt.Printf("OK: %s: %s\n", res.Host().Hostname, res.Data().(task.RemoteCommandResults).Stdout)
			}
		case <-time.After(time.Second * 10):
			return
		}
	}

With an interface like the one described above you'd implement your processor (probably a better name than output actually) in any way you want and (a) get more hooks where you can do things and (b) simplify that block as instead what you have to implement is the interface and do whatever you want there with the data provided.

type MyCustomProcessor struct {
}

func (p MyCustomProcessor) PreHost(ctx Context, host Host, task Task) error {
    fmt.Printf("I am going to start processing host '%s'", host.Hostname)
    return nil
}
func (p MyCustomProcessor) Success(ctx Context, res TaskInstanceResult) error {
    fmt.Printf("OK: %s: %s\n", res.Host().Hostname, res.Data()
    return nil
}

func (p MyCustomProcessor) Fail(ctx Context, err Error) error {
    fmt.Printf("ERROR: %s: %s\n", res.Host().Hostname, res.Err().Error())
    return nil
}
...

[nleiva]

Do you have any other use-case in mind other than additional logging?. I'm trying to wrap my head around this, to understand whether the complexity that it adds provides enough benefits, at least at this point in time.

[dbarrosop]

it’s not for logging, it is to process the events as they occur. If you want to log you can log but that’s not the only use case. For instance, I am planning to use this in a grpc service I am writing to send the client the results of doing various things immediately (via a stream) rather than waiting for everything to complete and without having to write that for loop above. Also, with the current methods I can’t send events before a host is about to be executed unless I write my own runner. With this everything is much simpler and I don’t have to write my own runner.