Scraping metrics from log files

Question

https://github.com/bodgix/log2metric/pull/1

This is my very first Go program so please forgive any beginner errors.

This is a rewrite of a monitoring script I had written in Ruby.

Coming from Ruby, I had two biggest challenges:

• do less "object oriented" and more "communication oriented" code using channels
• unit tests and mocking disk IO in particular

Although this is a little lengthy pull request, I hope someone will have the time and interest to at least glimpse over it and provide some feedback to a very beginner Go learner :)

logfile.go

package main
import (
 "bufio"
 "fmt"
 "io"
 "os"
)
type statefulLogFile struct {
 logFile file
 stateFile file
}
// Close save the current position and close the file
func (lf *statefulLogFile) Close() error {
 defer lf.logFile.Close()
 defer lf.stateFile.Close()
 // read the current position of the log file and save it to the state file
 pos, err := lf.logFile.Seek(0, os.SEEK_CUR)
 if err != nil {
 return err
 }
 _, err = fmt.Fprintf(lf.stateFile, "%d", pos)
 return err
}
func readLogFile(name, stateFile string, outCh chan<- string, errCh chan<- error) {
 logFile, err := openLogFile(name, stateFile)
 defer close(outCh)
 if err != nil {
 errCh <- err
 } else {
 defer logFile.Close()
 var line string
 reader := bufio.NewReader(logFile.logFile)
 for {
 line, err = reader.ReadString('\n')
 if err != nil {
 if err != io.EOF { // report all errors except io.EOF
 errCh <- err
 break
 } else { // EOF reached. Send the last line and stop reading
 outCh <- line
 break
 }
 }
 outCh <- line
 }
 }
}
func openLogFile(name, stateFile string) (*statefulLogFile, error) {
 sfLog := &statefulLogFile{}
 f, err := fs.OpenFile(name, os.O_RDONLY, 0660)
 if err != nil {
 return sfLog, err
 }
 sfLog.logFile = f
 f, err = openStateFile(stateFile, fs)
 if err != nil {
 return sfLog, err
 }
 sfLog.stateFile = f
 var lastPos int64
 lastPos, err = getLastPos(sfLog.stateFile)
 if err != nil {
 return sfLog, err
 }
 sfLog.stateFile.Seek(0, os.SEEK_SET)
 sfLog.logFile.Seek(lastPos, os.SEEK_SET)
 return sfLog, err
}
func openStateFile(name string, fs fileSystem) (file, error) {
 if _, err := fs.Stat(name); os.IsNotExist(err) {
 return fs.Create(name)
 }
 return fs.OpenFile(name, os.O_RDWR, 0660)
}
func getLastPos(stateFile io.Reader) (int64, error) {
 var lastPos int64
 n, err := fmt.Fscanf(stateFile, "%d", &lastPos)
 if n == 0 {
 lastPos = 0
 }
 if err == io.EOF {
 err = nil
 }
 return lastPos, err
}

filesystem.go

package main
import (
 "io"
 "os"
)
// All functions in this pakage interact with the file system via this package variable
var fs fileSystem = osFS{}
type fileSystem interface {
 Create(name string) (file, error)
 OpenFile(name string, flag int, perm os.FileMode) (file, error)
 Stat(name string) (os.FileInfo, error)
}
type file interface {
 io.Closer
 io.Reader
 io.Seeker
 io.Writer
}
type osFS struct{}
func (osFS) Open(name string) (file, error) {
 return os.Open(name)
}
func (osFS) Create(name string) (file, error) {
 return os.Create(name)
}
func (osFS) OpenFile(name string, flag int, perm os.FileMode) (file, error) {
 return os.OpenFile(name, flag, perm)
}
func (osFS) Stat(name string) (os.FileInfo, error) {
 return os.Stat(name)
}

parser.go

package main
import (
 "regexp"
 "strconv"
)
type metricType int
const (
 simple metricType = iota
)
type metric struct {
 t metricType
 name string
 value float64
}
func parseLogFile(input <-chan string, output chan<- metric, regExp string) {
 defer close(output)
 exp := regexp.MustCompile(regExp)
 for line := range input {
 matches := exp.FindStringSubmatch(line)
 if matches == nil {
 continue
 }
 for i, name := range exp.SubexpNames() {
 if name == "" {
 continue
 }
 val, err := strconv.ParseFloat(matches[i], 64)
 if err != nil {
 continue
 }
 m := metric{t: simple, name: name, value: val}
 output <- m
 }
 }
}

main.go

package main
import (
 "log"
)
func main() {
 logLinesCh := make(chan string)
 errCh := make(chan error)
 metricsCh := make(chan metric)
 defer close(errCh)
 go readLogFile("apache.log", "/tmp/apache_log_state", logLinesCh, errCh)
 go parseLogFile(logLinesCh, metricsCh, `(?P<resp_time>[\d.]+)`)
 fin := false
 for !fin {
 select {
 case m, ok := <-metricsCh:
 if ok {
 log.Println("Received a new metric: ", m)
 } else {
 log.Println("Metrics channel was closed")
 fin = true
 }
 case err := <-errCh:
 log.Println("Received an error: ", err)
 fin = true
 }
 }
}

logfile_test.go

package main
import (
 "bytes"
 "errors"
 "os"
 "strings"
 "testing"
)
var logFileLines = []string{"line1", "line2", "line3"}
var stateFileLines = []string{"0"}
var openLogError error
var openStateError error
type testFile struct {
 *bytes.Reader
 *bytes.Buffer
}
func (testFile) Close() error {
 return nil
}
func (f testFile) Write(p []byte) (int, error) {
 return f.Buffer.Write(p)
}
func (f testFile) Read(p []byte) (int, error) {
 return f.Reader.Read(p)
}
type mockFS struct{}
func (mockFS) Create(name string) (file, error) {
 return new(testFile), nil
}
func (fs mockFS) OpenFile(name string, flag int, perm os.FileMode) (file, error) {
 var lines []string
 var err error
 if name == "log" {
 lines = logFileLines
 err = openLogError
 } else {
 lines = stateFileLines
 err = openStateError
 }
 buf := testFile{bytes.NewReader([]byte(strings.Join(lines, "\n"))), &bytes.Buffer{}}
 return buf, err
}
func (mockFS) Stat(name string) (os.FileInfo, error) {
 return nil, nil
}
func TestReadLogFileNoErrors(t *testing.T) {
 fs = mockFS{}
 logLinesCh := make(chan string)
 errCh := make(chan error)
 go readLogFile("log", "state", logLinesCh, errCh)
 i := 0
 for line := range logLinesCh {
 line = strings.TrimRight(line, "\n")
 if line != logFileLines[i] {
 t.Errorf("Expected: %x got %x\n", logFileLines[i], line)
 }
 i++
 }
}
func TestReadLogFileOpenLogFileError(t *testing.T) {
 fs = mockFS{}
 logLinesCh := make(chan string)
 errCh := make(chan error)
 openLogError = errors.New("Error opening the log file")
 go readLogFile("log", "state", logLinesCh, errCh)
 err := <-errCh
 if err != openLogError {
 t.Errorf("Expected error %v, got %v\n", openLogError, err)
 }
}
func TestReadLogFileOpenStateFileError(t *testing.T) {
 fs = mockFS{}
 logLinesCh := make(chan string)
 errCh := make(chan error)
 openStateError = errors.New("Error opening the state file")
 go readLogFile("log", "state", logLinesCh, errCh)
 err := <-errCh
 if err != openLogError {
 t.Errorf("Expected error %v, got %v\n", openLogError, err)
 }
}
func TestCloseStatefulLogFile(t *testing.T) {
 logFile := new(statefulLogFile)
 testFile := &testFile{bytes.NewReader([]byte(strings.Join(logFileLines, "\n"))), new(bytes.Buffer)}
 logFile.logFile = testFile
 logFile.stateFile = testFile
 logFile.Close()
 got := testFile.String()
 if got != "0" {
 t.Error("Expected 0 written to the state file, got ", got)
 }
}

Answer 1

Looks good overall. The choice of channels is okay, though I don't yet see a huge benefit here outside of exercising it. Maybe the main loop should also be a function though, so you could potentially run it in another goroutine as well, even if not necessary here.

• Consider going with the "official" style and also start methods/types with an uppercase character.
• If you'd want to parse multiple files consider passing the regex object directly instead of recompiling it in parseLogFile.
• The globals openLogError and openStateError seem pointless. Just construct the values up front and not in the tests themselves, the fewer initialisation steps you have in them the better.

On the one hand wrapping the standard library is okay, I'd probably not do it this way and just make some temporary files; on the other hand why not, the additional code isn't much anyway.

That said the fileSystem object isn't passed to openLogFile, but it is to openStateFile. Also the global variable in filesystem.go is a bit problematic, at least you should have an explicit rule somewhere that this interface must be thread-safe.

• bodgix: Very helpful and informed comments. Having published this code review, I continued working on the tests and had a similar idea that instead of wrapping the filesystem, I could use temp files (fixtures). This would work well for the times when files are read from, but I didn't like the idea of unit tests leaving state files behind. On the other hand, unit tests could remove all files created by the test in a clean up function.