HTTP status monitoring

Question

I am fairly new to programming. I am writing a script which gets URLs and parameters through a config and makes http requests to check status, finally push a json string to Open-falcon server.

I am not sure if the logic and writing is reasonable. Could I get some guidance from someone experienced with Go. Really appreciate that. Thanks!

Code:

package main
import (
 "bytes"
 "encoding/json"
 "fmt"
 "io/ioutil"
 "net/http"
 "os"
 "strings"
 "time"
 "github.com/tkanos/gonfig"
)
type Config struct {
 Params []struct {
 Metric string
 URL string
 RequestMethod string
 Data string
 ExpectedString string
 Timeout int8
 Step int8
 }
}
const path = "config/config.json"
var c Config
func parseConfig(path string) Config {
 err := gonfig.GetConf(path, &c)
 if err != nil {
 panic(err)
 }
 return c
}
type Data struct {
 Metric string
 Step int8
 Value int8
}
var (
 l []Data
 m Data
)
func makeRequest(c Config) []Data {
 for _, i := range c.Params {
 m.Metric = i.Metric
 m.Step = i.Step
 if i.RequestMethod == "get" {
 client := &http.Client{Timeout: time.Duration(i.Timeout) * time.Second}
 resp, err := client.Get(i.URL)
 if err != nil {
 panic(err)
 }
 defer resp.Body.Close()
 if i.ExpectedString == "" {
 if resp.StatusCode >= 200 && resp.StatusCode <= 299 {
 m.Value = 0
 } else {
 m.Value = 1
 }
 } else {
 body, err := ioutil.ReadAll(resp.Body)
 if err != nil {
 panic(err)
 }
 if strings.Contains(string(body), i.ExpectedString) {
 m.Value = 0
 } else {
 m.Value = 1
 }
 }
 } else {
 client := http.Client{Timeout: time.Duration(i.Timeout) * time.Second}
 resp, err := client.Post(i.URL, "application/json", strings.NewReader(i.Data))
 if err != nil {
 panic(err)
 }
 defer resp.Body.Close()
 if i.ExpectedString == "" {
 if resp.StatusCode >= 200 && resp.StatusCode <= 299 {
 m.Value = 0
 } else {
 m.Value = 1
 }
 } else {
 body, err := ioutil.ReadAll(resp.Body)
 if err != nil {
 panic(err)
 }
 if strings.Contains(string(body), i.ExpectedString) {
 m.Value = 0
 } else {
 m.Value = 1
 }
 }
 }
 l = append(l, m)
 }
 fmt.Println(l)
 return l
}
type item struct {
 Endpoint string `json:"endpoint"`
 Metric string `json:"metric"`
 Timestamp int64 `json:"timestamp"`
 Step int8 `json:"step"`
 Value int8 `json:"value"`
 CounterType string `json:"counterType"`
 Tags string `json:"tags"`
}
type message struct {
 Item []item `json:"item"`
}
func prepare(d message) {
 apiurl := "http://XXX.XXX.XXX.XXX:1988/v1/push"
 jsonStr, _ := json.Marshal(d.Item)
 req, err := http.NewRequest("POST", apiurl, bytes.NewBuffer([]byte(jsonStr)))
 req.Header.Set("Content-Type", "application/json")
 client := &http.Client{}
 resp, err := client.Do(req)
 if err != nil {
 panic(err)
 }
 defer resp.Body.Close()
}
func pushToFalcon(l []Data) {
 for _, i := range l {
 var v item
 hostName, _ := os.Hostname()
 v.Endpoint = hostName
 v.Timestamp = time.Now().Unix()
 v.Step = i.Step
 v.CounterType = "GAUGE"
 v.Tags = "service=intranet_service"
 v.Metric = i.Metric
 v.Value = i.Value
 var o message
 o.Item = append(o.Item, v)
 prepare(o)
 }
}
func main() {
 parseConfig(path)
 makeRequest(c)
 pushToFalcon(l)
}

Config:

 { 
 "Params" : [
 { 
 "Metric" : "Test",
 "URL" : "http://www.google.com",
 "RequestMethod" : "get",
 "Data" : "",
 "ExpectedString" : "",
 "Timeout" : 10,
 "Step" : 60
 },
 {
 "Metric" : "Test2",
 "URL" : "https://www.baidu.com",
 "RequestMethod" : "post",
 "Data" : "datapostes",
 "ExpectedString" : "haha",
 "Timeout" : 10,
 "Step" : 60
 }
]
}

Answer 1

Not a full review, but several issues pop up at a quick scan:

• Avoid panic, it's meant for reporting a bug in the program (e.g. dividing by zero, failing a bounds check, dereferencing a nil pointer, etc) and not for user errors (e.g. a failure to complete an HTTP request).

  Panics also produce stack traces by default. Users should (almost) never get shown stack traces but instead a simple error report (e.g. if in unix you do ls /non-existant-dir you don't get a stack trace of the internals of the ls command but a report that the directory /non-existant-dir does not exist).

• defer should almost never be used within a loop. The deferred function doesn't run until the enclosing function returns/exits. Here you should be calling resp.Body.Close on each loop iteration before the next one.

• Avoid using global variables. Here there is no reason that l and m cannot be local to makeRequest.

• From the net/http documentation:

  Clients and Transports are safe for concurrent use by multiple goroutines and for efficiency should only be created once and re-used.

  You create a new http.Client on each loop iteration. At minimum you could create just one outside of the loop and then set client.Timeout in each iteration.

• You repeat a lot of code in each branch of the if i.RequestMethod == "get". I'd instead conditionally create a nil or non-nil body and use a single req, err := http.NewRequest(i.RequestMethod, i.URL, body) and client.Do(req).

• Never ignore errors like this:

  jsonStr, _ := json.Marshal(d.Item)

  Either handle/report/return the error as regular or if the surrounding function doesn't have (and shouldn't have) an error return and if the code should never fail (e.g. the code is constructing a value that should always be marshallable) then a panic for err != nil is reasonable (i.e. if someone should change the code later that json.Marshal suddenly fails they'll get a panic telling them where they messed up rather than strange failures later on).

• Instead of bytes.NewBuffer([]byte(jsonStr)) do strings.NewReader(jsonStr)

  In general, try and avoid or limit []byte <-> string conversions as they require allocations.

• Config should probably be passed around as a pointer, *Config. Either that or it's Params field should be a slice of pointers ([]*struct). As-is if there the slice was 100 items each time it's passed ×ばつ5 strings need to be copied (which only copies a pointer to the string, but it's still O(n)).