Make the NewX() monitoring variables safe to re-register (#83)

* make it safe to re-register monitoring variables

* try to fix CI

monitoring/metrics.go

@@ -20,6 +20,7 @@ package monitoring
 import (
 	"encoding/json"
 	"expvar"
+	"fmt"
 	"math"
 	"strconv"
 	"sync"
@@ -43,8 +44,14 @@ type Int struct{ i atomic.Int64 }
 // variable will be available via expvars package as well, but can not be removed
 // anymore.
 func NewInt(r *Registry, name string, opts ...Option) *Int {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Int)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Int", name, existingVar))
+		}
 	}
 
 	v := &Int{}
@@ -71,8 +78,14 @@ type Uint struct{ u atomic.Uint64 }
 // variable will be available via expvars package as well, but can not be removed
 // anymore.
 func NewUint(r *Registry, name string, opts ...Option) *Uint {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Uint)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Uint", name, existingVar))
+		}
 	}
 
 	v := &Uint{}
@@ -102,8 +115,14 @@ type Float struct{ f atomic.Uint64 }
 // variable will be available via expvars package as well, but can not be removed
 // anymore.
 func NewFloat(r *Registry, name string, opts ...Option) *Float {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Float)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Float", name, existingVar))
+		}
 	}
 
 	v := &Float{}
@@ -137,8 +156,14 @@ type Bool struct{ f atomic.Bool }
 // variable will be available via expvars package as well, but can not be removed
 // anymore.
 func NewBool(r *Registry, name string, opts ...Option) *Bool {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Bool)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Bool", name, existingVar))
+		}
 	}
 
 	v := &Bool{}
@@ -164,8 +189,14 @@ type String struct {
 // variable will be available via expvars package as well, but can not be removed
 // anymore.
 func NewString(r *Registry, name string, opts ...Option) *String {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*String)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as String", name, existingVar))
+		}
 	}
 
 	v := &String{}
@@ -209,8 +240,14 @@ type Func struct {
 }
 
 func NewFunc(r *Registry, name string, f func(Mode, Visitor), opts ...Option) *Func {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Func)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Func", name, existingVar))
+		}
 	}
 
 	v := &Func{f}
@@ -246,8 +283,14 @@ type Timestamp struct {
 
 // NewTimestamp creates and registers a new timestamp variable.
 func NewTimestamp(r *Registry, name string, opts ...Option) *Timestamp {
-	if r == nil {
-		r = Default
+	existingVar, r := setupMetric(r, name, opts)
+	if existingVar != nil {
+		cast, ok := existingVar.(*Timestamp)
+		if ok {
+			return cast
+		} else {
+			panicErr(fmt.Errorf("variable name %s was first registered as a %T, tried to register as Timestamp", name, existingVar))
+		}
 	}
 
 	v := &Timestamp{}
@@ -290,3 +333,23 @@ func (v *Timestamp) toString() string {
 	}
 	return v.cached
 }
+
+func setupMetric(reg *Registry, name string, opts []Option) (Var, *Registry) {
+	if reg == nil {
+		return nil, Default
+	}
+	vis := reg.Get(name)
+	if vis == nil {
+		return nil, reg
+	}
+
+	// Just being overly cautious.
+	// If someone tries to re-register a variable with options, should we try to "reset"
+	// the variable somehow? Even if we did, this wouldn't work 100% of the time, as we can't
+	// un-publish to expvar.
+	if len(opts) > 0 {
+		panicErr(fmt.Errorf("the variable %s cannot be re-registered with options", name))
+	}
+	return vis, reg
+
+}

monitoring/metrics_test.go

@@ -0,0 +1,47 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package monitoring
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSafeVars(t *testing.T) {
+	uintValName := "testUint"
+	testReg := Default.NewRegistry("safe_registry")
+	testUint := NewUint(testReg, uintValName)
+	testUint.Set(5)
+	// Add the first time
+	require.NotNil(t, testUint)
+
+	// Add the metric a second time
+	testSecondUint := NewUint(testReg, uintValName)
+	require.NotNil(t, testSecondUint)
+	// make sure we fetch the same unit
+	require.Equal(t, uint64(5), testSecondUint.Get())
+}
+
+func TestNilReg(t *testing.T) {
+	uintValName := "testUint"
+	// This can also just panic if there's a bug
+	testUint := NewUint(nil, uintValName)
+	require.NotNil(t, testUint)
+
+}