1
\$\begingroup\$

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
    }
]

}
\$\endgroup\$
0
\$\begingroup\$

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 100×5 strings need to be copied (which only copies a pointer to the string, but it's still O(n)).

\$\endgroup\$
  • \$\begingroup\$ Thank you for your guidance! \$\endgroup\$ – Azheng Aug 7 at 5:57

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.