Merge pull request #608 from dstrop/config-metrics-fix

fix(metrics): remove statsProvider from metrics collectors sync.Map

6 files changed, 51 insertions, 41 deletions

diff --git a/.github/workflows/linters.yml b/.github/workflows/linters.yml
index a2294ed9..79e00f9b 100644
--- a/.github/workflows/linters.yml
+++ b/.github/workflows/linters.yml
@@ -1,14 +1,6 @@
 name: Linters
 
-on:
-  push:
-  pull_request:
-    branches:
-      # Branches from forks have the form 'user:branch-name' so we only run
-      # this job on pull_request events for branches that look like fork
-      # branches. Without this we would end up running this job twice for non
-      # forked PRs, once for the push and then once for opening the PR.
-      - "**:**"
+on: [push, pull_request]
 
 jobs:
   golangci-lint:
diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml
index c5fde431..a478726d 100644
--- a/.github/workflows/linux.yml
+++ b/.github/workflows/linux.yml
@@ -1,14 +1,6 @@
 name: Tests
 
-on:
-  push:
-  pull_request:
-    branches:
-      # Branches from forks have the form 'user:branch-name' so we only run
-      # this job on pull_request events for branches that look like fork
-      # branches. Without this we would end up running this job twice for non
-      # forked PRs, once for the push and then once for opening the PR.
-      - "**:**"
+on: [push, pull_request]
 
 jobs:
   golang:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2bcd726c..8b9ff269 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,9 +3,14 @@ CHANGELOG
 
 v2.0.2 (06.04.2021)
 -------------------
-- 🐛 Fix: Bug with required Root CA certificate for the SSL, not it's optional.
-- 🆕 New: HTTP/FCGI/HTTPS internal logs instead of going to the raw stdout will be displayed in the RR logger at the `Info` log level.
-
+- 🐛 Fix: Bug with required Root CA certificate for the SSL, now it's optional.
+- 🐛 Fix: Bug with incorrectly consuming metrics collector from the RPC calls (thanks @dstrop).
+- 🆕 New: HTTP/FCGI/HTTPS internal logs instead of going to the raw stdout will be displayed in the RR logger at
+  the `Info` log level.
+- ⚡ New: Builds for the Mac with the M1 processor (arm64).
+- 👷 Rework ServeHTTP handler logic. Use http.Error instead of writing code directly to the response writer. Other small
+  improvements.
+  
 v2.0.1 (09.03.2021)
 -------------------
 - 🐛 Fix: incorrect PHP command validation
diff --git a/plugins/metrics/plugin.go b/plugins/metrics/plugin.go
index fefe92bd..efcbfc50 100644
--- a/plugins/metrics/plugin.go
+++ b/plugins/metrics/plugin.go
@@ -23,11 +23,6 @@ const (
 	maxHeaderSize = 1024 * 1024 * 100 // 104MB
 )
 
-type statsProvider struct {
-	collectors []prometheus.Collector
-	name       string
-}
-
 // Plugin to manage application metrics using Prometheus.
 type Plugin struct {
 	cfg        *Config
@@ -74,10 +69,7 @@ func (m *Plugin) Init(cfg config.Configurer, log logger.Logger) error {
 
 	// Register invocation will be later in the Serve method
 	for k, v := range collectors {
-		m.collectors.Store(k, statsProvider{
-			collectors: []prometheus.Collector{v},
-			name:       k,
-		})
+		m.collectors.Store(k, v)
 	}
 	return nil
 }
@@ -92,13 +84,11 @@ func (m *Plugin) Serve() chan error {
 	errCh := make(chan error, 1)
 	m.collectors.Range(func(key, value interface{}) bool {
 		// key - name
-		// value - statsProvider struct
-		c := value.(statsProvider)
-		for _, v := range c.collectors {
-			if err := m.registry.Register(v); err != nil {
-				errCh <- err
-				return false
-			}
+		// value - prometheus.Collector
+		c := value.(prometheus.Collector)
+		if err := m.registry.Register(c); err != nil {
+			errCh <- err
+			return false
 		}
 
 		return true
@@ -211,10 +201,13 @@ func (m *Plugin) Collects() []interface{} {
 
 // Collector returns application specific collector by name or nil if collector not found.
 func (m *Plugin) AddStatProvider(name endure.Named, stat StatProvider) error {
-	m.collectors.Store(name.Name(), statsProvider{
-		collectors: stat.MetricsCollector(),
-		name:       name.Name(),
-	})
+	for _, c := range stat.MetricsCollector() {
+		err := m.registry.Register(c)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
diff --git a/tests/plugins/metrics/.rr-test.yaml b/tests/plugins/metrics/.rr-test.yaml
index bc68b90f..4890076f 100644
--- a/tests/plugins/metrics/.rr-test.yaml
+++ b/tests/plugins/metrics/.rr-test.yaml
@@ -10,6 +10,9 @@ metrics:
       help: "Custom application metric"
       labels: [ "type" ]
       buckets: [ 0.1, 0.2, 0.3, 1.0 ]
+    app_metric_counter:
+      type: counter
+      help: "Custom application counter."
 logs:
   mode: development
   level: error
\ No newline at end of file
diff --git a/tests/plugins/metrics/metrics_test.go b/tests/plugins/metrics/metrics_test.go
index b5a4fd4f..d552107e 100644
--- a/tests/plugins/metrics/metrics_test.go
+++ b/tests/plugins/metrics/metrics_test.go
@@ -83,6 +83,7 @@ func TestMetricsInit(t *testing.T) {
 	assert.NoError(t, err)
 
 	assert.Contains(t, out, "go_gc_duration_seconds")
+	assert.Contains(t, out, "app_metric_counter")
 
 	for {
 		select {
@@ -199,6 +200,7 @@ func TestMetricsDifferentRPCCalls(t *testing.T) {
 	mockLogger.EXPECT().Info("adding metric", "name", "sub_gauge_subVector", "value", gomock.Any(), "labels", []string{"core", "first"}).MinTimes(1)
 	mockLogger.EXPECT().Info("adding metric", "name", "sub_gauge_subMetric", "value", gomock.Any(), "labels", gomock.Any()).MinTimes(1)
 	mockLogger.EXPECT().Info("adding metric", "name", "test_metrics_named_collector", "value", gomock.Any(), "labels", gomock.Any()).MinTimes(1)
+	mockLogger.EXPECT().Info("adding metric", "name", "app_metric_counter", "value", gomock.Any(), "labels", gomock.Any()).MinTimes(1)
 
 	mockLogger.EXPECT().Info("metric successfully added", "name", "observe_observeMetricNotEnoughLabels", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
 	mockLogger.EXPECT().Info("metric successfully added", "name", "observe_observeMetric", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
@@ -216,6 +218,7 @@ func TestMetricsDifferentRPCCalls(t *testing.T) {
 	mockLogger.EXPECT().Info("metric successfully added", "name", "test_metrics_named_collector", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
 	mockLogger.EXPECT().Info("metric successfully added", "name", "test_metrics_named_collector", "labels", gomock.Any(), "value", gomock.Any()).MinTimes(1)
 	mockLogger.EXPECT().Info("metric successfully added", "name", "user_gauge_collector", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
+	mockLogger.EXPECT().Info("metric successfully added", "name", "app_metric_counter", "labels", gomock.Any(), "value", gomock.Any()).MinTimes(1)
 
 	mockLogger.EXPECT().Info("declaring new metric", "name", "observe_observeMetricNotEnoughLabels", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
 	mockLogger.EXPECT().Info("declaring new metric", "name", "observe_observeMetric", "type", gomock.Any(), "namespace", gomock.Any()).MinTimes(1)
@@ -361,9 +364,31 @@ func TestMetricsDifferentRPCCalls(t *testing.T) {
 
 	t.Run("ObserveMetricNotEnoughLabels", observeMetricNotEnoughLabels)
 
+	t.Run("ConfiguredCounterMetric", configuredCounterMetric)
+	genericOut, err = get()
+	assert.NoError(t, err)
+	assert.Contains(t, genericOut, "HELP app_metric_counter Custom application counter.")
+	assert.Contains(t, genericOut, `app_metric_counter 100`)
+
 	close(sig)
 }
 
+func configuredCounterMetric(t *testing.T) {
+	conn, err := net.Dial(dialNetwork, dialAddr)
+	assert.NoError(t, err)
+	defer func() {
+		_ = conn.Close()
+	}()
+	client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn))
+	var ret bool
+
+	assert.NoError(t, client.Call("metrics.Add", metrics.Metric{
+		Name:  "app_metric_counter",
+		Value: 100.0,
+	}, &ret))
+	assert.True(t, ret)
+}
+
 func observeMetricNotEnoughLabels(t *testing.T) {
 	conn, err := net.Dial(dialNetwork, dialAddr)
 	assert.NoError(t, err)