diff --git a/.scripts/test_all.sh b/.scripts/test_all.sh index aefb293..7d85877 100755 --- a/.scripts/test_all.sh +++ b/.scripts/test_all.sh @@ -26,3 +26,5 @@ go get -u github.com/prometheus/client_golang/prometheus # Test all packages which are allowed on all Go versions go test $packages + +go vet $packages diff --git a/appveyor.yml b/appveyor.yml index f97e076..444b35b 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -23,3 +23,5 @@ test_script: - go test ./memorylocker - go test ./consullocker - go test ./s3store + + - go vet ./prometheuscollector diff --git a/metrics.go b/metrics.go index 177d965..3639f23 100644 --- a/metrics.go +++ b/metrics.go @@ -1,6 +1,9 @@ package tusd import ( + "errors" + "net" + "sync" "sync/atomic" ) @@ -13,7 +16,7 @@ type Metrics struct { // RequestTotal counts the number of incoming requests per method RequestsTotal map[string]*uint64 // ErrorsTotal counts the number of returned errors by their message - ErrorsTotal map[string]*uint64 + ErrorsTotal *ErrorsTotalMap BytesReceived *uint64 UploadsFinished *uint64 UploadsCreated *uint64 @@ -29,16 +32,9 @@ func (m Metrics) incRequestsTotal(method string) { } // incErrorsTotal increases the counter for this error atomically by one. -func (m Metrics) incErrorsTotal(err error) { - msg := err.Error() - - if addr, ok := m.ErrorsTotal[msg]; ok { - atomic.AddUint64(addr, 1) - } else { - addr := new(uint64) - *addr = 1 - m.ErrorsTotal[msg] = addr - } +func (m Metrics) incErrorsTotal(err HTTPError) { + ptr := m.ErrorsTotal.retrievePointerFor(err) + atomic.AddUint64(ptr, 1) } // incBytesReceived increases the number of received bytes atomically be the @@ -80,7 +76,73 @@ func newMetrics() Metrics { } } -func newErrorsTotalMap() map[string]*uint64 { - m := make(map[string]*uint64, 20) +// ErrorsTotalMap stores the counters for the different HTTP errors. +type ErrorsTotalMap struct { + lock sync.RWMutex + counter map[simpleHTTPError]*uint64 +} + +type simpleHTTPError struct { + Message string + StatusCode int +} + +func simplifyHTTPError(err HTTPError) simpleHTTPError { + var msg string + // Errors for read timeouts contain too much information which is not + // necessary for us and makes grouping for the metrics harder. The error + // message looks like: read tcp 127.0.0.1:1080->127.0.0.1:53673: i/o timeout + // Therefore, we use a common error message for all of them. + if netErr, ok := err.(net.Error); ok && netErr.Timeout() { + msg = "read tcp: i/o timeout" + } else { + msg = err.Error() + } + return simpleHTTPError{ + Message: msg, + StatusCode: err.StatusCode(), + } +} + +func newErrorsTotalMap() *ErrorsTotalMap { + m := make(map[simpleHTTPError]*uint64, 20) + return &ErrorsTotalMap{ + counter: m, + } +} + +// retrievePointerFor returns (after creating it if necessary) the pointer to +// the counter for the error. +func (e *ErrorsTotalMap) retrievePointerFor(err HTTPError) *uint64 { + serr := simplifyHTTPError(err) + e.lock.RLock() + ptr, ok := e.counter[serr] + e.lock.RUnlock() + if ok { + return ptr + } + + // For pointer creation, a write-lock is required + e.lock.Lock() + // We ensure that the pointer wasn't created in the meantime + if ptr, ok = e.counter[serr]; !ok { + ptr = new(uint64) + e.counter[serr] = ptr + } + e.lock.Unlock() + + return ptr +} + +// Load retrieves the map of the counter pointers atomically +func (e *ErrorsTotalMap) Load() map[HTTPError]*uint64 { + m := make(map[HTTPError]*uint64, len(e.counter)) + e.lock.RLock() + for err, ptr := range e.counter { + httpErr := NewHTTPError(errors.New(err.Message), err.StatusCode) + m[httpErr] = ptr + } + e.lock.RUnlock() + return m } diff --git a/prometheuscollector/prometheuscollector.go b/prometheuscollector/prometheuscollector.go index 25c8943..8486f23 100644 --- a/prometheuscollector/prometheuscollector.go +++ b/prometheuscollector/prometheuscollector.go @@ -9,6 +9,7 @@ package prometheuscollector import ( + "strconv" "sync/atomic" "github.com/tus/tusd" @@ -23,8 +24,8 @@ var ( []string{"method"}, nil) errorsTotalDesc = prometheus.NewDesc( "tusd_errors_total", - "Total number of erorrs per cause.", - []string{"cause"}, nil) + "Total number of errors per status.", + []string{"status", "message"}, nil) bytesReceivedDesc = prometheus.NewDesc( "tusd_bytes_received", "Number of bytes received for uploads.", @@ -39,7 +40,7 @@ var ( nil, nil) uploadsTerminatedDesc = prometheus.NewDesc( "tusd_uploads_terminated", - "Number of terminted uploads.", + "Number of terminated uploads.", nil, nil) ) @@ -73,12 +74,13 @@ func (c Collector) Collect(metrics chan<- prometheus.Metric) { ) } - for error, valuePtr := range c.metrics.ErrorsTotal { + for httpError, valuePtr := range c.metrics.ErrorsTotal.Load() { metrics <- prometheus.MustNewConstMetric( errorsTotalDesc, prometheus.GaugeValue, float64(atomic.LoadUint64(valuePtr)), - error, + strconv.Itoa(httpError.StatusCode()), + httpError.Error(), ) } diff --git a/unrouted_handler.go b/unrouted_handler.go index 73425e8..6fd8d9c 100644 --- a/unrouted_handler.go +++ b/unrouted_handler.go @@ -5,7 +5,6 @@ import ( "errors" "io" "log" - "net" "net/http" "os" "regexp" @@ -612,17 +611,9 @@ func (handler *UnroutedHandler) sendError(w http.ResponseWriter, r *http.Request err = ErrNotFound } - // Errors for read timeouts contain too much information which is not - // necessary for us and makes grouping for the metrics harder. The error - // message looks like: read tcp 127.0.0.1:1080->127.0.0.1:53673: i/o timeout - // Therefore, we use a common error message for all of them. - if netErr, ok := err.(net.Error); ok && netErr.Timeout() { - err = errors.New("read tcp: i/o timeout") - } - - status := 500 - if statusErr, ok := err.(HTTPError); ok { - status = statusErr.StatusCode() + statusErr, ok := err.(HTTPError) + if !ok { + statusErr = NewHTTPError(err, http.StatusInternalServerError) } reason := err.Error() + "\n" @@ -632,12 +623,12 @@ func (handler *UnroutedHandler) sendError(w http.ResponseWriter, r *http.Request w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.Header().Set("Content-Length", strconv.Itoa(len(reason))) - w.WriteHeader(status) + w.WriteHeader(statusErr.StatusCode()) w.Write([]byte(reason)) - handler.log("ResponseOutgoing", "status", strconv.Itoa(status), "method", r.Method, "path", r.URL.Path, "error", err.Error()) + handler.log("ResponseOutgoing", "status", strconv.Itoa(statusErr.StatusCode()), "method", r.Method, "path", r.URL.Path, "error", err.Error()) - go handler.Metrics.incErrorsTotal(err) + go handler.Metrics.incErrorsTotal(statusErr) } // sendResp writes the header to w with the specified status code.