Metrics registered on wrong prometheus registry (#2246)

* - UT on metrics verifying that all plugins of all blocs have their metrics collectors declared

* - fix error msg

* - redirect Registry of metric to the one that handle the listener
- allow duplicate of metrics collector on the same Registry (case of same plugin in 2 blocs listening metrics on the same address)

* - fix change of signature

* - ensure cleaning metrics before starting the test (metrics collectors are global vars .. and re-used by several tests)

* - I think I fixed this test. Ensure correct mn of hits and clean metrics before test.

* - fix typo in error msg - proposed at review

* - fix typo in comment

* - remove ResetMetrics functions
- change a way to test the numeric metrics : get the diff between begin and end of test

* - oops. removing debug logs

5 files changed, 66 insertions, 21 deletions

diff --git a/plugin/metrics/metrics.go b/plugin/metrics/metrics.go
index 8efeff1e7..6b496cccc 100644
--- a/plugin/metrics/metrics.go
+++ b/plugin/metrics/metrics.go
@@ -57,7 +57,15 @@ func New(addr string) *Metrics {
 }
 
 // MustRegister wraps m.Reg.MustRegister.
-func (m *Metrics) MustRegister(c prometheus.Collector) { m.Reg.MustRegister(c) }
+func (m *Metrics) MustRegister(c prometheus.Collector) {
+	err := m.Reg.Register(c)
+	if err != nil {
+		// ignore any duplicate error, but fatal on any other kind of error
+		if _, ok := err.(prometheus.AlreadyRegisteredError); !ok {
+			log.Fatalf("Cannot register metrics collector: %s", err)
+		}
+	}
+}
 
 // AddZone adds zone z to m.
 func (m *Metrics) AddZone(z string) {
diff --git a/plugin/metrics/setup.go b/plugin/metrics/setup.go
index c00f44a83..ffc0466f3 100644
--- a/plugin/metrics/setup.go
+++ b/plugin/metrics/setup.go
@@ -31,6 +31,13 @@ func setup(c *caddy.Controller) error {
 		return plugin.Error("prometheus", err)
 	}
 
+	// register the metrics to its address (ensure only one active metrics per address)
+	obj := uniqAddr.Set(m.Addr, m.OnStartup, m)
+	//propagate the real active Registry to current metrics
+	if om, ok := obj.(*Metrics); ok {
+		m.Reg = om.Reg
+	}
+
 	dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
 		m.Next = next
 		return m
@@ -55,10 +62,6 @@ func setup(c *caddy.Controller) error {
 func prometheusParse(c *caddy.Controller) (*Metrics, error) {
 	var met = New(defaultAddr)
 
-	defer func() {
-		uniqAddr.Set(met.Addr, met.OnStartup)
-	}()
-
 	i := 0
 	for c.Next() {
 		if i > 0 {
diff --git a/plugin/metrics/test/scrape.go b/plugin/metrics/test/scrape.go
index a21c0061d..185627491 100644
--- a/plugin/metrics/test/scrape.go
+++ b/plugin/metrics/test/scrape.go
@@ -27,6 +27,7 @@ import (
 	"io"
 	"mime"
 	"net/http"
+	"strconv"
 	"testing"
 
 	"github.com/matttproud/golang_protobuf_extensions/pbutil"
@@ -78,6 +79,47 @@ func Scrape(t *testing.T, url string) []*MetricFamily {
 	return result
 }
 
+// ScrapeMetricAsInt provide a sum of all metrics collected for the name and label provided.
+// if the metric is not a numeric value, it will be counted a 0.
+func ScrapeMetricAsInt(t *testing.T, addr string, name string, label string, nometricvalue int) int {
+
+	valueToInt := func(m metric) int {
+		v := m.Value
+		r, err := strconv.Atoi(v)
+		if err != nil {
+			return 0
+		}
+		return r
+	}
+
+	met := Scrape(t, fmt.Sprintf("http://%s/metrics", addr))
+	found := false
+	tot := 0
+	for _, mf := range met {
+		if mf.Name == name {
+			// Sum all metrics available
+			for _, m := range mf.Metrics {
+				if label == "" {
+					tot += valueToInt(m.(metric))
+					found = true
+					continue
+				}
+				for _, v := range m.(metric).Labels {
+					if v == label {
+						tot += valueToInt(m.(metric))
+						found = true
+					}
+				}
+			}
+		}
+	}
+
+	if !found {
+		return nometricvalue
+	}
+	return tot
+}
+
 // MetricValue returns the value associated with name as a string as well as the labels.
 // It only returns the first metrics of the slice.
 func MetricValue(name string, mfs []*MetricFamily) (string, map[string]string) {
diff --git a/plugin/pkg/uniq/uniq.go b/plugin/pkg/uniq/uniq.go
index 6dd74883d..f4b4f54a6 100644
--- a/plugin/pkg/uniq/uniq.go
+++ b/plugin/pkg/uniq/uniq.go
@@ -10,6 +10,7 @@ type U struct {
 type item struct {
 	state int          // either todo or done
 	f     func() error // function to be executed.
+	obj   interface{}  // any object to return when needed
 }
 
 // New returns a new initialized U.
@@ -17,30 +18,21 @@ func New() U { return U{u: make(map[string]item)} }
 
 // Set sets function f in U under key. If the key already exists
 // it is not overwritten.
-func (u U) Set(key string, f func() error) {
-	if _, ok := u.u[key]; ok {
-		return
+func (u U) Set(key string, f func() error, o interface{}) interface{} {
+	if item, ok := u.u[key]; ok {
+		return item.obj
 	}
-	u.u[key] = item{todo, f}
+	u.u[key] = item{todo, f, o}
+	return o
 }
 
-// Unset removes the 'todo' associated with a key
+// Unset removes the key.
 func (u U) Unset(key string) {
 	if _, ok := u.u[key]; ok {
 		delete(u.u, key)
 	}
 }
 
-// SetTodo sets key to 'todo' again.
-func (u U) SetTodo(key string) {
-	v, ok := u.u[key]
-	if !ok {
-		return
-	}
-	v.state = todo
-	u.u[key] = v
-}
-
 // ForEach iterates for u executes f for each element that is 'todo' and sets it to 'done'.
 func (u U) ForEach() error {
 	for k, v := range u.u {
diff --git a/plugin/pkg/uniq/uniq_test.go b/plugin/pkg/uniq/uniq_test.go
index 5d58c924b..2093fc7ec 100644
--- a/plugin/pkg/uniq/uniq_test.go
+++ b/plugin/pkg/uniq/uniq_test.go
@@ -4,7 +4,7 @@ import "testing"
 
 func TestForEach(t *testing.T) {
 	u, i := New(), 0
-	u.Set("test", func() error { i++; return nil })
+	u.Set("test", func() error { i++; return nil }, nil)
 
 	u.ForEach()
 	if i != 1 {