Skip to content

Commit

Permalink
fix: latency test race condition (#29)
Browse files Browse the repository at this point in the history
* fix: copy loop variable to avoid race conditions

* refactor: encapsulate latency check in functions

* test: refactor latency check unit test

* chore: readd latency run unit test

* chore: add vscode test flags

* chore: use testing cleanup instead of defering

* fix: use timeout from provided cfg

* chore: rm unused dep

* refactor: latency run unit test to tdt

* fix: use shared http client

* test: activate httpmock in outer run function

* docs: add godoc for SetClient

* chore: use configs timeout

* fix: set global http client for checks

---------

Signed-off-by: Bruno Bressi <[email protected]>
Co-authored-by: Bruno Bressi <[email protected]>
  • Loading branch information
lvlcn-t and puffitos authored Dec 8, 2023
1 parent 2facb4d commit 0e297bb
Show file tree
Hide file tree
Showing 18 changed files with 350 additions and 207 deletions.
22 changes: 11 additions & 11 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,15 +1,4 @@
repos:
- repo: https://github.com/tekwizely/pre-commit-golang
rev: v1.0.0-rc.1
hooks:
- id: go-mod-tidy-repo
- id: go-test-repo-mod
args: [ -race, -count=1 ]
- id: go-vet-repo-mod
- id: go-fumpt-repo
args: [ -l, -w ]
- id: golangci-lint-repo-mod
args: [ --config, .golangci.yaml, --, --fix ]
- repo: local
hooks:
- id: go-generate-repo
Expand All @@ -20,6 +9,17 @@ repos:
types: [ go ]
pass_filenames: false
always_run: true
- repo: https://github.com/tekwizely/pre-commit-golang
rev: v1.0.0-rc.1
hooks:
- id: go-mod-tidy-repo
- id: go-test-repo-mod
args: [ -race ]
- id: go-vet-repo-mod
- id: go-fumpt-repo
args: [ -l, -w ]
- id: golangci-lint-repo-mod
args: [ --config, .golangci.yaml, --, --fix ]
- repo: https://github.com/norwoodj/helm-docs
rev: "v1.11.3"
hooks:
Expand Down
6 changes: 6 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"go.testFlags": [
"-race",
"-cover"
]
}
2 changes: 1 addition & 1 deletion docs/sparrow.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ The check results are exposed via an API.
* [sparrow gen-docs](sparrow_gen-docs.md) - Generate markdown documentation
* [sparrow run](sparrow_run.md) - Run sparrow

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_completion.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ See each sub-command's help for details on how to use the generated script.
* [sparrow completion powershell](sparrow_completion_powershell.md) - Generate the autocompletion script for powershell
* [sparrow completion zsh](sparrow_completion_zsh.md) - Generate the autocompletion script for zsh

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_completion_bash.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ sparrow completion bash

* [sparrow completion](sparrow_completion.md) - Generate the autocompletion script for the specified shell

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_completion_fish.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,4 @@ sparrow completion fish [flags]

* [sparrow completion](sparrow_completion.md) - Generate the autocompletion script for the specified shell

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_completion_powershell.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,4 @@ sparrow completion powershell [flags]

* [sparrow completion](sparrow_completion.md) - Generate the autocompletion script for the specified shell

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_completion_zsh.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,4 @@ sparrow completion zsh [flags]

* [sparrow completion](sparrow_completion.md) - Generate the autocompletion script for the specified shell

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_gen-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ sparrow gen-docs [flags]

* [sparrow](sparrow.md) - Sparrow, the infrastructure monitoring agent

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
2 changes: 1 addition & 1 deletion docs/sparrow_run.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,4 @@ sparrow run [flags]

* [sparrow](sparrow.md) - Sparrow, the infrastructure monitoring agent

###### Auto generated by spf13/cobra on 6-Dec-2023
###### Auto generated by spf13/cobra on 7-Dec-2023
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ require (
github.com/spf13/cobra v1.8.0
github.com/spf13/viper v1.17.0
github.com/stretchr/testify v1.8.4
golang.org/x/sync v0.3.0
gopkg.in/yaml.v3 v3.0.1
)

Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,6 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ
golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=
golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Expand Down
7 changes: 6 additions & 1 deletion pkg/checks/checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ package checks

import (
"context"
"net/http"
"time"

"github.com/getkin/kin-openapi/openapi3"

"github.com/caas-team/sparrow/pkg/api"
)

// Available Checks will be registered in this map
// RegisteredChecks will be registered in this map
// The key is the name of the Check
// The name needs to map the configuration item key
var RegisteredChecks = map[string]func() Check{
Expand All @@ -51,6 +52,10 @@ type Check interface {
// This is also called while the check is running, if the remote config is updated
// This should return an error if the config is invalid
SetConfig(ctx context.Context, config any) error
// SetClient sets an HTTP client for the check. This method is used to configure
// the check with a specific HTTP client, which can be used for network requests
// during the check's execution
SetClient(c *http.Client)
// Should return an openapi3.SchemaRef of the result type returned by the check
Schema() (*openapi3.SchemaRef, error)
// Allows the check to register a handler on sparrows http server at runtime
Expand Down
5 changes: 5 additions & 0 deletions pkg/checks/health.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ func (h *Health) SetConfig(_ context.Context, config any) error {
return nil
}

// SetClient sets the http client for the health check
func (h *Health) SetClient(_ *http.Client) {
// TODO: implement with issue #31
}

// Schema provides the schema of the data that will be provided
// by the heath check
func (h *Health) Schema() (*openapi3.SchemaRef, error) {
Expand Down
133 changes: 77 additions & 56 deletions pkg/checks/latency.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ package checks

import (
"context"
"io"
"net/http"
"sync"
"time"

"github.com/getkin/kin-openapi/openapi3"
"github.com/mitchellh/mapstructure"
"golang.org/x/sync/errgroup"

"github.com/caas-team/sparrow/internal/helper"
"github.com/caas-team/sparrow/internal/logger"
Expand All @@ -19,18 +19,20 @@ var _ Check = (*Latency)(nil)

func NewLatencyCheck() Check {
return &Latency{
mu: sync.Mutex{},
cfg: LatencyConfig{},
c: nil,
done: make(chan bool, 1),
mu: sync.Mutex{},
cfg: LatencyConfig{},
c: nil,
done: make(chan bool, 1),
client: &http.Client{},
}
}

type Latency struct {
cfg LatencyConfig
mu sync.Mutex
c chan<- Result
done chan bool
cfg LatencyConfig
mu sync.Mutex
c chan<- Result
done chan bool
client *http.Client
}

type LatencyConfig struct {
Expand All @@ -57,11 +59,8 @@ func (l *Latency) Run(ctx context.Context) error {
case <-l.done:
return nil
case <-time.After(l.cfg.Interval):
results, err := l.check(ctx)
results := l.check(ctx)
errval := ""
if err != nil {
errval = err.Error()
}
checkResult := Result{
Data: results,
Err: errval,
Expand Down Expand Up @@ -103,6 +102,13 @@ func (l *Latency) SetConfig(_ context.Context, config any) error {
return nil
}

// SetClient sets the http client for the latency check
func (l *Latency) SetClient(c *http.Client) {
l.mu.Lock()
defer l.mu.Unlock()
l.client = c
}

func (l *Latency) Schema() (*openapi3.SchemaRef, error) {
return OpenapiFromPerfData(make(map[string]LatencyResult))
}
Expand All @@ -119,59 +125,74 @@ func (l *Latency) Handler(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
}

func (l *Latency) check(ctx context.Context) (map[string]LatencyResult, error) {
func (l *Latency) check(ctx context.Context) map[string]LatencyResult {
log := logger.FromContext(ctx).WithGroup("check")
log.Debug("Checking latency")
if len(l.cfg.Targets) == 0 {
log.Debug("No targets defined")
return map[string]LatencyResult{}
}

var resultMutex sync.Mutex
var mu sync.Mutex
var wg sync.WaitGroup
results := map[string]LatencyResult{}

wg, ctx := errgroup.WithContext(ctx)
for _, e := range l.cfg.Targets {
wg.Go(func(ctx context.Context, e string) func() error {
return func() error {
cl := http.Client{
Timeout: l.cfg.Timeout * time.Second,
}
req, err := http.NewRequestWithContext(ctx, http.MethodGet, e, http.NoBody)
if err != nil {
log.Error("Error while creating request", "error", err)
return err
}

var result LatencyResult

req = req.WithContext(ctx)

helper.Retry(func(ctx context.Context) error {
start := time.Now()
response, err := cl.Do(req) //nolint:bodyclose // Tom has refactored this function
if err != nil {
errval := err.Error()
result.Error = &errval
log.Error("Error while checking latency", "error", err)
} else {
result.Code = response.StatusCode
}
end := time.Now()

result.Total = end.Sub(start).Milliseconds()

resultMutex.Lock()
defer resultMutex.Unlock()
results[e] = result

return err
}, l.cfg.Retry)(ctx) //nolint: errcheck //ignore return value, since we set it in the closure
for _, tar := range l.cfg.Targets {
target := tar
wg.Add(1)
go func(target string) {
defer wg.Done()
lo := log.With("target", target)
lo.Debug("Starting retry routine to get latency", "target", target)

err := helper.Retry(func(ctx context.Context) error {
mu.Lock()
defer mu.Unlock()
l.client.Timeout = l.cfg.Timeout
res := getLatency(ctx, l.client, target)
results[target] = res
return nil
}, l.cfg.Retry)(ctx)
if err != nil {
lo.Error("Error while checking latency", "error", err)
}
}(ctx, e))
}(target)
}
wg.Wait()

if err := wg.Wait(); err != nil {
log.Info("Successfully got latency to all targets")
return results
}

// getLatency performs an HTTP get request and returns ok if request succeeds
func getLatency(ctx context.Context, client *http.Client, url string) LatencyResult {
log := logger.FromContext(ctx).With("url", url)
var res LatencyResult

req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody)
if err != nil {
log.Error("Error while creating request", "error", err)
errval := err.Error()
res.Error = &errval
return res
}

start := time.Now()
resp, err := client.Do(req) //nolint:bodyclose // Closed in defer below
if err != nil {
log.Error("Error while checking latency", "error", err)
return nil, err
errval := err.Error()
res.Error = &errval
return res
}

return results, nil
res.Code = resp.StatusCode
defer func(Body io.ReadCloser) {
_ = Body.Close()
}(resp.Body)

end := time.Now()

res.Total = end.Sub(start).Milliseconds()
return res
}
Loading

0 comments on commit 0e297bb

Please sign in to comment.