Feature: add ability to configure logger file provider

Adds new concepts like a FileWriterProvider which provides a
mechanism for a witchcraft.Server and invoking code to share
common io.Writer objects for performing logging operations.

Fixes #96

integration/common_test.go

@@ -47,7 +47,8 @@ const (
 
 // createAndRunTestServer returns a running witchcraft.Server that is initialized with simple default configuration in a
 // temporary directory. Returns the server, the path to the temporary directory, a channel that returns the error
-// returned by the server when it stops and a cleanup function that will remove the temporary directory.
+// returned by the server when it stops and a cleanup function that will remove the temporary directory. All logger
+// output is written to the provided logOutputBuffer, or to os.Stdout if no logOutputBuffer is specified.
 func createAndRunTestServer(t *testing.T, initFn witchcraft.InitFunc, logOutputBuffer io.Writer) (server *witchcraft.Server, port int, managementPort int, serverErr <-chan error, cleanup func()) {
 	var err error
 	port, err = httpserver.AvailablePort()

integration/logger_test.go

@@ -0,0 +1,122 @@
+// Copyright (c) 2019 Palantir Technologies. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package integration
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"path"
+	"testing"
+	"time"
+
+	"github.com/nmiyake/pkg/dirs"
+	"github.com/palantir/pkg/httpserver"
+	"github.com/palantir/witchcraft-go-logging/wlog"
+	"github.com/palantir/witchcraft-go-logging/wlog/svclog/svc1log"
+	"github.com/palantir/witchcraft-go-server/config"
+	"github.com/palantir/witchcraft-go-server/rest"
+	"github.com/palantir/witchcraft-go-server/witchcraft"
+	"github.com/palantir/witchcraft-go-server/witchcraft/refreshable"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestUseLoggerFileWriterProvider tests the behavior of sharing the output writer for a logger before and after a
+// server is started. Tests the workflow where an external program creates a logger for a file at a specific path and
+// expects a witchcraft.Server that is constructed later to use the same io.Writer for its logger (to ensure that the
+// file properly handles writes from different sources).
+func TestUseLoggerFileWriterProvider(t *testing.T) {
+	testDir, cleanup, err := dirs.TempDir("", "")
+	require.NoError(t, err)
+	defer cleanup()
+
+	// set working directory
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+	defer func() {
+		err := os.Chdir(wd)
+		require.NoError(t, err)
+	}()
+	err = os.Chdir(testDir)
+	require.NoError(t, err)
+
+	cachingWriterProvider := witchcraft.NewCachingFileWriterProvider(witchcraft.DefaultFileWriterProvider())
+	svc1Logger := svc1log.New(witchcraft.CreateLogWriter("var/log/service.log", false, os.Stdout, cachingWriterProvider), wlog.DebugLevel)
+	svc1Logger.Info("Test output from before server start")
+
+	port, err := httpserver.AvailablePort()
+	require.NoError(t, err)
+	managementPort, err := httpserver.AvailablePort()
+	require.NoError(t, err)
+
+	server := witchcraft.NewServer().
+		WithInitFunc(func(ctx context.Context, initInfo witchcraft.InitInfo) (func(), error) {
+			// register handler that returns "ok"
+			err := initInfo.Router.Get("/ok", http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+				rest.WriteJSONResponse(rw, "ok", http.StatusOK)
+			}))
+			if err != nil {
+				return nil, err
+			}
+			return nil, nil
+		}).
+		WithInstallConfig(config.Install{
+			ProductName: productName,
+			Server: config.Server{
+				Address:        "localhost",
+				Port:           port,
+				ManagementPort: managementPort,
+				ContextPath:    basePath,
+			},
+		}).
+		WithLoggerFileWriterProvider(cachingWriterProvider).
+		WithRuntimeConfigProvider(refreshable.NewDefaultRefreshable([]byte{})).
+		WithECVKeyProvider(witchcraft.ECVKeyNoOp()).
+		WithDisableGoRuntimeMetrics().
+		WithSelfSignedCertificate()
+
+	serverChan := make(chan error)
+	go func() {
+		serverChan <- server.Start()
+	}()
+
+	success := <-waitForTestServerReady(port, "example/ok", 5*time.Second)
+	if !success {
+		errMsg := "timed out waiting for server to start"
+		select {
+		case err := <-serverChan:
+			errMsg = fmt.Sprintf("%s: %+v", errMsg, err)
+		default:
+		}
+		require.Fail(t, errMsg)
+	}
+
+	svc1Logger.Info("Test output from after server start")
+
+	// verify service log output
+	serviceLogBytes, err := ioutil.ReadFile(path.Join("var", "log", "service.log"))
+	require.NoError(t, err)
+
+	msgs := getLogFileMessages(t, serviceLogBytes)
+	assert.Equal(t, []string{
+		"Test output from before server start",
+		"Listening to https",
+		"Listening to https",
+		"Test output from after server start",
+	}, msgs)
+}

witchcraft/loggers.go

@@ -19,6 +19,7 @@ import (
 	"io"
 	"os"
 	"strings"
+	"sync"
 
 	"github.com/palantir/witchcraft-go-logging/wlog"
 	"github.com/palantir/witchcraft-go-logging/wlog/auditlog/audit2log"
@@ -46,8 +47,13 @@ func (s *Server) initLoggers(useConsoleLog bool, logLevel wlog.LogLevel) {
 		loggerStdoutWriter = s.loggerStdoutWriter
 	}
 
-	logOutputFn := func(logOutputPath string) io.Writer {
-		return newDefaultLogOutput(logOutputPath, useConsoleLog, loggerStdoutWriter)
+	loggerFileWriterProvider := DefaultFileWriterProvider()
+	if s.loggerFileWriterProvider != nil {
+		loggerFileWriterProvider = s.loggerFileWriterProvider
+	}
+
+	logOutputFn := func(logFileName string) io.Writer {
+		return CreateLogWriter(fmt.Sprintf("var/log/%s.log", logFileName), useConsoleLog, loggerStdoutWriter, loggerFileWriterProvider)
 	}
 
 	s.svcLogger = svc1log.New(logOutputFn("service"), logLevel, svc1LogParams...)
@@ -64,21 +70,76 @@ func (s *Server) initLoggers(useConsoleLog bool, logLevel wlog.LogLevel) {
 	)
 }
 
-func newDefaultLogOutput(logOutputPath string, logToStdout bool, stdoutWriter io.Writer) io.Writer {
-	if logToStdout || logToStdoutBasedOnEnv() {
-		return stdoutWriter
-	}
+type FileWriterProvider interface {
+	FileWriter(logOutputPath string) io.Writer
+}
+
+func DefaultFileWriterProvider() FileWriterProvider {
+	return &defaultFileWriterProvider{}
+}
+
+type defaultFileWriterProvider struct{}
+
+func (p *defaultFileWriterProvider) FileWriter(logOutputPath string) io.Writer {
 	return &lumberjack.Logger{
-		Filename:   fmt.Sprintf("var/log/%s.log", logOutputPath),
+		Filename:   logOutputPath,
 		MaxSize:    1000,
 		MaxBackups: 10,
 		MaxAge:     30,
 		Compress:   true,
 	}
 }
 
-// logToStdoutBasedOnEnv returns true if the runtime environment is a non-jail Docker container, false otherwise.
-func logToStdoutBasedOnEnv() bool {
+type cachingFileWriterProvider struct {
+	delegate FileWriterProvider
+	cache    map[string]io.Writer
+	lock     sync.Mutex
+}
+
+// NewCachingFileWriterProvider returns a FileWriterProvider that uses the provided FileWriterProvider to create writers
+// and always returns the same io.Writer for a given path. Is thread-safe.
+func NewCachingFileWriterProvider(delegate FileWriterProvider) FileWriterProvider {
+	return &cachingFileWriterProvider{
+		delegate: delegate,
+		cache:    make(map[string]io.Writer),
+	}
+}
+
+func (p *cachingFileWriterProvider) FileWriter(logOutputPath string) io.Writer {
+	p.lock.Lock()
+	defer p.lock.Unlock()
+	if _, ok := p.cache[logOutputPath]; !ok {
+		// if not in cache, create and set
+		p.cache[logOutputPath] = p.delegate.FileWriter(logOutputPath)
+	}
+	return p.cache[logOutputPath]
+}
+
+// CreateLogWriter returns the io.Writer that should be used to write logs given the specified parameters. This function
+// is used internally by witchcraft.Server, and is thus useful in cases where code that executes before a
+// witchcraft.Server wants to perform logging operations using the same writer as the one that the witchcraft.Server
+// will use later (as opposed to the invoking code and server both using separate io.Writers for the same output
+// destination, which could cause issues like overwriting the same file).
+//
+// The following is an example usage:
+//
+//   cachingWriterProvider := witchcraft.NewCachingFileWriterProvider(witchcraft.DefaultFileWriterProvider())
+//	 svc1Logger := svc1log.New(witchcraft.CreateLogWriter("var/log/service.log", false, os.Stdout, cachingWriterProvider), wlog.DebugLevel)
+//   server := witchcraft.NewServer().
+//	 	 WithLoggerFileWriterProvider(cachingWriterProvider).
+//		 ...
+//
+// In this example, the svc1Logger and the service logger for witchcraft.NewServer will both use the same writer
+// provided by cachingWriterProvider.
+func CreateLogWriter(logOutputPath string, useConsoleLog bool, consoleLogWriter io.Writer, fileWriterProvider FileWriterProvider) io.Writer {
+	if useConsoleLog || logToConsoleBasedOnEnv() {
+		return consoleLogWriter
+	}
+	return fileWriterProvider.FileWriter(logOutputPath)
+}
+
+// logToConsoleBasedOnEnv returns true if the runtime environment is a non-jail Docker container, false otherwise.
+func logToConsoleBasedOnEnv() bool {
 	return isDocker() && !isJail()
 }
 

witchcraft/witchcraft.go

@@ -164,6 +164,10 @@ type Server struct {
 	// they should write to Stdout. If nil, os.Stdout is used by default.
 	loggerStdoutWriter io.Writer
 
+	// loggerFileWriterProvider specifies the FileWriterProvider that is used to create the io.Writer used to write log
+	// files at the specified path. If nil, uses DefaultFileWriterProvider.
+	loggerFileWriterProvider FileWriterProvider
+
 	// loggers
 	svcLogger    svc1log.Logger
 	evtLogger    evt2log.Logger
@@ -421,6 +425,14 @@ func (s *Server) WithLoggerStdoutWriter(loggerStdoutWriter io.Writer) *Server {
 	return s
 }
 
+// WithLoggerFileWriterProvider configures the server to use the provided FileWriterProvider to obtain the writer used
+// by loggers to write to a specific file path. This is typically only done in cases were logging may occur outside the
+// server and the caller wants to ensure that file-based logging uses the same backing writer.
+func (s *Server) WithLoggerFileWriterProvider(loggerFileWriterProvider FileWriterProvider) *Server {
+	s.loggerFileWriterProvider = loggerFileWriterProvider
+	return s
+}
+
 const (
 	defaultMetricEmitFrequency = time.Second * 60