3
\$\begingroup\$

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)
    }
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – bodgix Jan 4 '18 at 18:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.