14
\$\begingroup\$

I'm writing my first somewhat production ready (or not) Go program and could do with some feedback from someone more experienced with go.

The code reads a list of URLs from a JSON file and then makes a GET request to each URL in a loop to check that they are returning a 200 OK response. I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

monitor.go

package main

import (
    "encoding/json"
    "fmt"
    "github.com/bradfitz/gomemcache/memcache"
    "os"
    "time"
)

// Configuration encapsulates the result of reading the JSON configuration
// file.
type Configuration struct {
    URLs      []string
    Memcached string
}

// loadConfig loads a configuration file in JSON format and returns a
// Configuration instance.
func loadConfig(path string) (Configuration, error) {
    file, _ := os.Open(path)
    defer file.Close()

    decoder := json.NewDecoder(file)
    configuration := Configuration{}
    err := decoder.Decode(&configuration)

    return configuration, err
}

func main() {
    type Result struct {
        url    string
        status Status
    }

    rc := make(chan Result)

    configuration, err := loadConfig("config.json")

    if err != nil {
        fmt.Println("Error :", err)
        return
    }

    sites := make(map[string]*Site, len(configuration.URLs))

    for _, url := range configuration.URLs {
        sites[url] = &Site{url, UNCHECKED}
    }

    mc := memcache.New(configuration.Memcached)

    sites_output := make(map[string]bool)

    for {
        for _, site := range sites {
            go func(site *Site, rc chan Result) {
                status, _ := site.Status()
                rc <- Result{site.url, status}
            }(site, rc)
        }

        for i := 0; i < len(sites); i++ {
            res := <-rc
            site := sites[res.url]
            if site.last_status != res.status {
                sites[res.url].last_status = res.status
            }
        }

        for k, v := range sites {
            sites_output[k] = v.last_status == 2
        }

        site_json, err := json.Marshal(sites_output)

        if err != nil {
            panic(err)
        }
        mc.Set(&memcache.Item{
            Key:   "mission_control.sites",
            Value: site_json,
        })
        time.Sleep(time.Second * 5)
    }
}

site.go

package main

import (
    "net/http"
)

type Status int

const (
    UNCHECKED Status = iota
    DOWN
    UP
)

// The Site struct encapsulates the details about the site being monitored.
type Site struct {
    url         string
    last_status Status
}

// Site.Status makes a GET request to a given URL and checks whether or not the
// resulting status code is 200.
func (s Site) Status() (Status, error) {
    resp, err := http.Get(s.url)
    status := s.last_status

    if (err == nil) && (resp.StatusCode == 200) {
        status = UP
    } else {
        status = DOWN
    }

    return status, err
}
\$\endgroup\$
9
\$\begingroup\$

Style

  • Use CamelCase only. last_status -> lastStatus, site_json -> siteJSON, etc.

  • Even in constants. UP -> Up or StatusUp, etc.

  • Imports are usually split into groups by source, so separate memcache from stdlib with a newline.

  • Stuttering. E.g. configuration := Configuration{} could be c := Configuration{}.

  • People would search for main() in main.go. So I would rename monitor.go to main.go.

Code

file, _ := os.Open(path)

I see you skip error handling in some places. I'm not sure whether this is just a brief version for this site, or an actual code, but this is a very bad practice.

v.last_status == 2

if (err == nil) && (resp.StatusCode == 200) {

Avoid magic numbers. Use Up/StatusUp and http.StatusOK respectively.

for i := 0; i < len(sites); i++ {

I'd use range. In Go 1.4 it'll be legal to write for range sites as well.

site := sites[res.url]
if site.last_status != res.status {
    sites[res.url].last_status = res.status
}

You index the map twice and make an unnecessary comparison. Wouldn't sites[res.url] = res.status do?

Architecture

If you can change the output JSON format, I'd suggest saving the status code returned by http.Get rather than a simple bool. That will give you more info on why the site is down (4XX and 5XX error) or that the site have moved (3XX).

I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

Your synchronisation seems mostly fine to me. (Don't forget to test with -race though). An alternative would be to use sync.WaitGroup and a map with a sync.Mutex.

\$\endgroup\$
  • \$\begingroup\$ Thanks a ton for your detailed response. One quick further question: you mention moving monitor.go to main.go which I agree with, however, one thing I'm still confused about is how I go about having the package being called monitor whilst retaining the ability to build an executable. \$\endgroup\$ – Prydie Oct 2 '14 at 17:23
  • \$\begingroup\$ @Prydie I don't quite get your question. From what I saw you only have one package and it's main. \$\endgroup\$ – Ainar-G Oct 2 '14 at 19:21
  • \$\begingroup\$ For now that's the case, however, I'm going to be building upon this to add a web front end which serves the data from memacached over HTTP so that a JS front end can display the results. \$\endgroup\$ – Prydie Oct 3 '14 at 17:50
  • \$\begingroup\$ @Prydie I'd still suggest just keeping it all in package main. The task seems simple enough, and splitting it into several packages doesn't seem to make it any more manageable. How to split the code into several files (and whether to do so) is up to you. \$\endgroup\$ – Ainar-G Oct 3 '14 at 20:11

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.