Refactor zmetrics to include appStart, appVersion and appRevision metrics and more (#82)

lucaslopezf · web-flow · commit c06bcdd32e50 · 2024-03-06T16:37:38.000-03:00
* Fix cache metrics and minor refactor on zmetrics

* Refactor zmetrics: add app-name and app-version to labels

* minor change

* test

* Fix tests

* Fix middleware order

* linter

* Set version and revision in metrics

* Update readme

* Add version and revision metric

* minor change
diff --git a/cmd/demo-metrics/main.go b/cmd/demo-metrics/main.go
@@ -13,7 +13,7 @@ func main() {
 	logger.InitLogger(logger.Config{Level: constants.DebugLevel})
 	r := runner.NewRunner()
 
-	r.AddTask(metrics.NewTaskMetrics("/metrics", "9090"))
+	r.AddTask(metrics.NewTaskMetrics("/metrics", "9090", "demo"))
 
 	// Now start all the tasks
 	r.StartAndWait()
diff --git a/cmd/demo-panic/main.go b/cmd/demo-panic/main.go
@@ -14,7 +14,7 @@ func main() {
 	r := runner.NewRunner()
 
 	// This will panic
-	r.AddTask(metrics.NewTaskMetrics("BADURL", "8080"))
+	r.AddTask(metrics.NewTaskMetrics("BADURL", "8080", "demo"))
 
 	// Now start all the tasks
 	r.StartAndWait()
diff --git a/pkg/metrics/collectors/counter.go b/pkg/metrics/collectors/counter.go
@@ -24,3 +24,25 @@ func (c *Counter) Update(collector prometheus.Collector, value float64, labels .
 
 	return fmt.Errorf("invalid metric type, expected Counter")
 }
+
+func (c *Counter) Inc(collector prometheus.Collector, labels ...string) error {
+	if len(labels) > 0 {
+		if metricVec, ok := collector.(*prometheus.CounterVec); ok {
+			metric := metricVec.WithLabelValues(labels...)
+			metric.Inc()
+			return nil
+		}
+		return fmt.Errorf("invalid metric type, expected CounterVec for labels")
+	}
+
+	if metric, ok := collector.(prometheus.Counter); ok {
+		metric.Inc()
+		return nil
+	}
+
+	return fmt.Errorf("invalid metric type, expected Counter")
+}
+
+func (c *Counter) Dec(_ prometheus.Collector, _ ...string) error {
+	return fmt.Errorf("dec is not supported by counter metric")
+}
diff --git a/pkg/metrics/collectors/counter_test.go b/pkg/metrics/collectors/counter_test.go
@@ -15,3 +15,12 @@ func TestCounterUpdate(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, float64(10), testutil.ToFloat64(counter))
 }
+
+func TestCounterInc(t *testing.T) {
+	counter := prometheus.NewCounter(prometheus.CounterOpts{Name: "test_counter"})
+	c := &Counter{}
+
+	err := c.Inc(counter)
+	assert.NoError(t, err)
+	assert.Equal(t, float64(1), testutil.ToFloat64(counter))
+}
diff --git a/pkg/metrics/handler.go b/pkg/metrics/handler.go
@@ -17,20 +17,17 @@ type IncrementDecrementHandler interface {
 }
 
 func (t *taskMetrics) performMetricAction(name string, action func(MetricHandler, prometheus.Collector, ...string) error, labels ...string) error {
-	name = formatMetricName(name)
+	labels = append(labels, t.appName)
+
 	t.mux.RLock()
 	metricDetail, ok := t.metrics[name]
 	t.mux.RUnlock()
 
 	if !ok {
-		return fmt.Errorf("error: Metric not found for %s", name)
-	}
-
-	if err := action(metricDetail.Handler, metricDetail.Collector, labels...); err != nil {
-		return fmt.Errorf("error performing action on metric %s. Err: %s", name, err.Error())
+		return fmt.Errorf("metric %s not registered", name)
 	}
 
-	return nil
+	return action(metricDetail.Handler, metricDetail.Collector, labels...)
 }
 
 func (t *taskMetrics) UpdateMetric(name string, value float64, labels ...string) error {
diff --git a/pkg/metrics/prometheus.go b/pkg/metrics/prometheus.go
@@ -23,6 +23,7 @@ type TaskMetrics interface {
 	IncrementMetric(name string, labels ...string) error
 	DecrementMetric(name string, labels ...string) error
 	Name() string
+	AppName() string
 	Stop() error
 }
 
@@ -36,12 +37,18 @@ type taskMetrics struct {
 	port    string
 	metrics map[string]MetricDetail
 	mux     sync.RWMutex
+	appName string
 }
 
-func NewTaskMetrics(path string, port string) TaskMetrics {
+func NewTaskMetrics(path, port, appName string) TaskMetrics {
+	if appName == "" {
+		panic("appName is mandatory")
+	}
+
 	return &taskMetrics{
 		path:    path,
 		port:    port,
+		appName: appName,
 		metrics: make(map[string]MetricDetail),
 	}
 }
@@ -50,6 +57,10 @@ func (t *taskMetrics) Name() string {
 	return "metrics"
 }
 
+func (t *taskMetrics) AppName() string {
+	return t.appName
+}
+
 func (t *taskMetrics) Start() error {
 	router := chi.NewRouter()
 
diff --git a/pkg/metrics/prometheus_test.go b/pkg/metrics/prometheus_test.go
@@ -9,13 +9,13 @@ import (
 )
 
 func TestName(t *testing.T) {
-	metrics := NewTaskMetrics("/metrics", "9090")
+	metrics := NewTaskMetrics("/metrics", "9090", "test")
 	assert.Equal(t, "metrics", metrics.Name())
 }
 
 func TestStart(t *testing.T) {
 	logger.InitLogger(logger.Config{})
-	metrics := NewTaskMetrics("/metrics", "9091")
+	metrics := NewTaskMetrics("/metrics", "9091", "test")
 
 	go func() {
 		err := metrics.Start()
diff --git a/pkg/metrics/readme.md b/pkg/metrics/readme.md
@@ -4,6 +4,7 @@
 
 - [Introduction](#introduction)
 - [Features](#features)
+- [Default Labels](#default-labels)
 - [Code Structure](#code-structure)
 - [Metrics Collectors](#metrics-collectors)
   - [Counter](#counter)
@@ -28,6 +29,22 @@ This project provides a comprehensive metrics collection and reporting framework
 - Thread-safe metric updates with read-write mutexes.
 - Auto-registering of metrics based on type.
 
+## Default Labels
+
+Every metric reported by the framework automatically includes the following labels to provide more context about the application's environment:
+
+- `app_name`: Identifies the name of the application.
+- `app_revision`: Specifies the current revision of the application, typically a Git commit hash or a similar identifier.
+- `app_version`: Indicates the current version of the application.
+
+These labels help in distinguishing metrics across different environments, versions, and revisions of the application. For example, a metric for request duration might look like this:
+
+```
+request_duration_ms{app_name="ledger-live",app_revision="main-9a60456",app_version="v0.8.3",method="GET",path="/addresses/{address}/transactions",status="200"} 435
+```
+
+This approach ensures that metrics data can be correlated with specific releases and deployments of the application, providing valuable insights during analysis and troubleshooting.
+
 ## Code Structure
 
 - `/metrics/collectors/`: Contains the implementation of various metrics collectors (Counter, Gauge, Histogram).
diff --git a/pkg/metrics/register.go b/pkg/metrics/register.go
@@ -8,6 +8,10 @@ import (
 	"strings"
 )
 
+const (
+	APPNameLabel = "app_name"
+)
+
 type collectorRegister func(name, help string, labels []string, handler MetricHandler) (prometheus.Collector, error)
 
 var (
@@ -19,6 +23,7 @@ var (
 )
 
 func (t *taskMetrics) RegisterMetric(name string, help string, labels []string, handler MetricHandler) error {
+	labels = append(labels, APPNameLabel)
 	var metric prometheus.Collector
 
 	name = formatMetricName(name)
diff --git a/pkg/metrics/register_test.go b/pkg/metrics/register_test.go
@@ -14,7 +14,7 @@ func TestRegisterMetric(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotNil(t, tm.metrics["test_counter"])
 	assert.IsType(t, MetricDetail{}, tm.metrics["test_counter"])
-	assert.IsType(t, prometheus.NewCounter(prometheus.CounterOpts{}), tm.metrics["test_counter"].Collector)
+	assert.IsType(t, prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"app_name", "app_version"}), tm.metrics["test_counter"].Collector)
 }
 
 func TestFormatMetricName(t *testing.T) {
diff --git a/pkg/zrouter/zmiddlewares/cache.go b/pkg/zrouter/zmiddlewares/cache.go
@@ -32,22 +32,22 @@ func CacheMiddleware(metricServer metrics.TaskMetrics, cache zcache.ZCache, conf
 			path := r.URL.Path
 			fullURL := constructFullURL(r)
 
+			rw := &responseWriter{ResponseWriter: w}
 			for _, pPath := range processedPaths {
 				if pPath.Regex.MatchString(path) {
 					key := constructCacheKey(fullURL)
 
-					if tryServeFromCache(w, r, cache, key, metricServer) {
+					if tryServeFromCache(rw, r, cache, key, metricServer) {
 						return
 					}
 
-					rw := &responseWriter{ResponseWriter: w}
 					next.ServeHTTP(rw, r) // Important: this line needs to be BEFORE setting the cache.
 					cacheResponseIfNeeded(rw, r, cache, key, pPath.TTL, metricServer)
 					return
 				}
 			}
 
-			next.ServeHTTP(w, r)
+			next.ServeHTTP(rw, r)
 		})
 	}
 }
@@ -100,6 +100,7 @@ func cacheResponseIfNeeded(rw *responseWriter, r *http.Request, cache zcache.ZCa
 		logger.GetLoggerFromContext(r.Context()).Errorf("Error incrementing cache_sets metric: %v", err)
 	}
 }
+
 func ParseCacheConfigPaths(paths map[string]string) (domain.CacheConfig, error) {
 	parsedPaths := make(map[string]time.Duration)
 
diff --git a/pkg/zrouter/zmiddlewares/metrics.go b/pkg/zrouter/zmiddlewares/metrics.go
@@ -1,7 +1,6 @@
 package zmiddlewares
 
 import (
-	"fmt"
 	"github.com/go-chi/chi/v5"
 	"github.com/zondax/golem/pkg/logger"
 	"github.com/zondax/golem/pkg/metrics"
@@ -13,16 +12,16 @@ import (
 )
 
 const (
-	activeConnectionsMetricType    = "active_connections"
-	durationMillisecondsMetricType = "duration_milliseconds"
-	responseSizeMetricType         = "response_size_bytes"
-	totalRequestMetricType         = "total_requests"
+	activeConnectionsMetricName    = "active_connections"
+	durationMillisecondsMetricName = "request_duration_ms"
+	responseSizeMetricName         = "response_size"
+	totalRequestsMetricName        = "total_requests"
 	pathLabel                      = "path"
 	methodLabel                    = "method"
 	statusLabel                    = "status"
 )
 
-func RegisterRequestMetrics(appName string, metricsServer metrics.TaskMetrics) []error {
+func RegisterRequestMetrics(metricsServer metrics.TaskMetrics) []error {
 	var errs []error
 
 	register := func(name, help string, labels []string, handler metrics.MetricHandler) {
@@ -31,34 +30,29 @@ func RegisterRequestMetrics(appName string, metricsServer metrics.TaskMetrics) [
 		}
 	}
 
-	totalRequestsMetricName := getMetricName(appName, "total_requests")
-	responseSizeMetricName := getMetricName(appName, "response_size")
-	durationMillisecondsMetricName := getMetricName(appName, "request_duration_ms")
-	activeConnectionsMetricName := getMetricName(appName, "active_connections")
 	register(totalRequestsMetricName, "Total number of HTTP requests made.", []string{"method", "path", "status"}, &collectors.Counter{})
-	register(durationMillisecondsMetricName, "Duration of HTTP requests in milliseconds.", []string{"method", "path", "status"}, &collectors.Histogram{})
+	register(durationMillisecondsMetricName, "Duration of HTTP requests in milliseconds.", []string{"method", "path", "status"}, &collectors.Gauge{})
 	register(responseSizeMetricName, "Size of HTTP response in bytes.", []string{"method", "path", "status"}, &collectors.Histogram{})
 	register(activeConnectionsMetricName, "Number of active HTTP connections.", []string{"method", "path"}, &collectors.Gauge{})
 
-	cacheHitsMetricName := getMetricName(appName, cacheHitsMetric)
-	cacheMissesMetricName := getMetricName(appName, cacheMissesMetric)
-	cacheSetsMetricName := getMetricName(appName, cacheSetsMetric)
+	cacheHitsMetricName := cacheHitsMetric
+	cacheMissesMetricName := cacheMissesMetric
+	cacheSetsMetricName := cacheSetsMetric
 	register(cacheHitsMetricName, "Number of cache hits.", []string{pathLabel}, &collectors.Counter{})
 	register(cacheMissesMetricName, "Number of cache misses.", []string{pathLabel}, &collectors.Counter{})
 	register(cacheSetsMetricName, "Number of responses added to the cache.", []string{pathLabel}, &collectors.Counter{})
 
 	return errs
 }
 
-func RequestMetrics(appName string, metricsServer metrics.TaskMetrics) Middleware {
+func RequestMetrics(metricsServer metrics.TaskMetrics) Middleware {
 	var activeConnections int64
 	var mu sync.Mutex
 
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			path := chi.RouteContext(r.Context()).RoutePattern()
 			startTime := time.Now()
-			activeConnectionsMetricName := getMetricName(appName, activeConnectionsMetricType)
 
 			mu.Lock()
 			activeConnections++
@@ -84,18 +78,15 @@ func RequestMetrics(appName string, metricsServer metrics.TaskMetrics) Middlewar
 
 			labels := []string{r.Method, path, strconv.Itoa(responseStatus)}
 
-			durationMillisecondsMetricName := getMetricName(appName, durationMillisecondsMetricType)
-			_ = metricsServer.UpdateMetric(durationMillisecondsMetricName, duration, labels...)
-
-			responseSizeMetricName := getMetricName(appName, responseSizeMetricType)
-			_ = metricsServer.UpdateMetric(responseSizeMetricName, float64(bytesWritten), labels...)
-
-			totalRequestsMetricName := getMetricName(appName, totalRequestMetricType)
-			_ = metricsServer.UpdateMetric(totalRequestsMetricName, 1, labels...)
+			if err := metricsServer.UpdateMetric(durationMillisecondsMetricName, duration, labels...); err != nil {
+				logger.GetLoggerFromContext(r.Context()).Errorf("error updating request duration metric: %v", err.Error())
+			}
+			if err := metricsServer.UpdateMetric(responseSizeMetricName, float64(bytesWritten), labels...); err != nil {
+				logger.GetLoggerFromContext(r.Context()).Errorf("error updating response size metric: %v", err.Error())
+			}
+			if err := metricsServer.UpdateMetric(totalRequestsMetricName, 1, labels...); err != nil {
+				logger.GetLoggerFromContext(r.Context()).Errorf("error updating total requests metric: %v", err.Error())
+			}
 		})
 	}
 }
-
-func getMetricName(appName, metricType string) string {
-	return fmt.Sprintf("zrouter_request_%s_%s", appName, metricType)
-}
diff --git a/pkg/zrouter/zmiddlewares/middleware.go b/pkg/zrouter/zmiddlewares/middleware.go
@@ -23,6 +23,11 @@ func (rw *responseWriter) Write(p []byte) (int, error) {
 	if rw.body == nil {
 		rw.body = new(bytes.Buffer)
 	}
+
+	if rw.status == 0 {
+		rw.WriteHeader(http.StatusOK)
+	}
+
 	rw.body.Write(p)
 	n, err := rw.ResponseWriter.Write(p)
 	rw.written += int64(n)
diff --git a/pkg/zrouter/zmiddlewares/zcache_test.go b/pkg/zrouter/zmiddlewares/zcache_test.go
@@ -23,7 +23,7 @@ func TestCacheMiddleware(t *testing.T) {
 		"/cached-path": 5 * time.Minute,
 	}}
 
-	r.Use(CacheMiddleware(metrics.NewTaskMetrics("", ""), mockCache, cacheConfig))
+	r.Use(CacheMiddleware(metrics.NewTaskMetrics("", "", "appName"), mockCache, cacheConfig))
 
 	// Simulate a response that should be cached
 	r.Get("/cached-path", func(w http.ResponseWriter, r *http.Request) {
diff --git a/pkg/zrouter/zrouter.go b/pkg/zrouter/zrouter.go
diff --git a/pkg/zrouter/zrouter_test.go b/pkg/zrouter/zrouter_test.go

Original file line number	Diff line number	Diff line change
`@@ -9,13 +9,13 @@ import (`
`9`	`9`	`)`
`10`	`10`
`11`	`11`	`func TestName(t *testing.T) {`
`12`		`- metrics := NewTaskMetrics("/metrics", "9090")`
	`12`	`+ metrics := NewTaskMetrics("/metrics", "9090", "test")`
`13`	`13`	`assert.Equal(t, "metrics", metrics.Name())`
`14`	`14`	`}`
`15`	`15`
`16`	`16`	`func TestStart(t *testing.T) {`
`17`	`17`	`logger.InitLogger(logger.Config{})`
`18`		`- metrics := NewTaskMetrics("/metrics", "9091")`
	`18`	`+ metrics := NewTaskMetrics("/metrics", "9091", "test")`
`19`	`19`
`20`	`20`	`go func() {`
`21`	`21`	`err := metrics.Start()`
Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,10 @@ import (`
`8`	`8`	`"strings"`
`9`	`9`	`)`
`10`	`10`
	`11`	`+const (`
	`12`	`+ APPNameLabel = "app_name"`
	`13`	`+)`
	`14`	`+`
`11`	`15`	`type collectorRegister func(name, help string, labels []string, handler MetricHandler) (prometheus.Collector, error)`
`12`	`16`
`13`	`17`	`var (`
`@@ -19,6 +23,7 @@ var (`
`19`	`23`	`)`
`20`	`24`
`21`	`25`	`func (t *taskMetrics) RegisterMetric(name string, help string, labels []string, handler MetricHandler) error {`
	`26`	`+ labels = append(labels, APPNameLabel)`
`22`	`27`	`var metric prometheus.Collector`
`23`	`28`
`24`	`29`	`name = formatMetricName(name)`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ func TestRegisterMetric(t *testing.T) {`
`14`	`14`	`assert.NoError(t, err)`
`15`	`15`	`assert.NotNil(t, tm.metrics["test_counter"])`
`16`	`16`	`assert.IsType(t, MetricDetail{}, tm.metrics["test_counter"])`
`17`		`- assert.IsType(t, prometheus.NewCounter(prometheus.CounterOpts{}), tm.metrics["test_counter"].Collector)`
	`17`	`+ assert.IsType(t, prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"app_name", "app_version"}), tm.metrics["test_counter"].Collector)`
`18`	`18`	`}`
`19`	`19`
`20`	`20`	`func TestFormatMetricName(t *testing.T) {`