Add context/test for Tally<>Prometheus usage discrepancies (#114)

glide.yaml

@@ -1,5 +1,4 @@
 package: github.com/uber-go/tally
-version: 1.0.0
 import:
 - package: github.com/cactus/go-statsd-client
   version: ~3.1.0
@@ -17,6 +16,8 @@ import:
   version: ^0.8.1
 - package: go.uber.org/atomic
   version: ^1
+- package: github.com/pkg/errors
+  version: ^0.8.1
 testImport:
 - package: github.com/axw/gocov
   version: 54b98cfcac0c63fb3f9bd8e7ad241b724d4e985b
@@ -41,4 +42,3 @@ testImport:
   - require
 - package: gopkg.in/validator.v2
 - package: gopkg.in/yaml.v2
-

prometheus/reporter.go

@@ -21,15 +21,15 @@
 package prometheus
 
 import (
-	"errors"
 	"net/http"
+	"strings"
 	"sync"
 	"time"
 
-	"github.com/uber-go/tally"
-
 	prom "github.com/m3db/prometheus_client_golang/prometheus"
 	"github.com/m3db/prometheus_client_golang/prometheus/promhttp"
+	"github.com/pkg/errors"
+	"github.com/uber-go/tally"
 )
 
 const (
@@ -291,6 +291,18 @@ func NewReporter(opts Options) Reporter {
 	}
 	if opts.OnRegisterError == nil {
 		opts.OnRegisterError = func(err error) {
+			// n.b. Because our forked Prometheus client does not actually emit
+			//      this message as a concrete error type (it uses fmt.Errorf),
+			//      we need to check the error message.
+			if strings.Contains(err.Error(), "previously registered") {
+				err = errors.WithMessagef(
+					err,
+					"potential tally.Scope() vs Prometheus usage contract mismatch: "+
+						"if this occurs after using Scope.Tagged(), different metric "+
+						"names must be used than were registered with the parent scope",
+				)
+			}
+
 			panic(err)
 		}
 	}

prometheus/reporter_test.go

@@ -358,6 +358,65 @@ func TestOnRegisterError(t *testing.T) {
 	assert.Equal(t, 2, len(captured))
 }
 
+func TestAlreadyRegisteredCounter(t *testing.T) {
+	var captured []error
+
+	registry := prom.NewRegistry()
+	r := NewReporter(Options{
+		Registerer: registry,
+		OnRegisterError: func(err error) {
+			captured = append(captured, err)
+		},
+	})
+
+	// n.b. Prometheus metrics are different from M3 metrics in that they are
+	//      uniquely identified as "metric_name+label_name+label_name+...";
+	//      additionally, given that Prometheus ingestion is pull-based, there
+	//      is only ever one reporter used regardless of tally.Scope hierarchy.
+	//
+	//      Because of this, for a given metric "foo", only the first-registered
+	//      permutation of metric and label names will succeed because the same
+	//      registry is being used, and because Prometheus asserts that a
+	//      registered metric name has the same corresponding label names.
+	//      Subsequent registrations - such as adding or removing tags - will
+	//      return an error.
+	//
+	//      This is a problem because Tally's API does not apply semantics or
+	//      restrictions to the combinatorics (or descendant mutations of)
+	//      metric tags. As such, we must assert that Tally's Prometheus
+	//      reporter does the right thing and indicates an error when this
+	//      happens.
+	//
+	// The first allocation call will succeed. This establishes the required
+	// label names (["foo"]) for metric "foo".
+	r.AllocateCounter("foo", map[string]string{"foo": "bar"})
+
+	// The second allocation call is okay, as it has the same label names (["foo"]).
+	r.AllocateCounter("foo", map[string]string{"foo": "baz"})
+
+	// The third allocation call fails, because it has different label names
+	// (["bar"], vs previously ["foo"]) for the same metric name "foo".
+	r.AllocateCounter("foo", map[string]string{"bar": "qux"})
+
+	// The fourth allocation call fails, because while it has one of the same
+	// label names ("foo") as was previously registered for metric "foo", it
+	// also has an additional label name (["foo", "zork"] != ["foo"]).
+	r.AllocateCounter("foo", map[string]string{
+		"foo":  "bar",
+		"zork": "derp",
+	})
+
+	// The fifth allocation call fails, because it has no label names for the
+	// metric "foo", which expects the label names it was originally registered
+	// with (["foo"]).
+	r.AllocateCounter("foo", nil)
+
+	require.Equal(t, 3, len(captured))
+	for _, err := range captured {
+		require.Contains(t, err.Error(), "same fully-qualified name")
+	}
+}
+
 func gather(t *testing.T, r prom.Gatherer) []*dto.MetricFamily {
 	metrics, err := r.Gather()
 	require.NoError(t, err)

types.go

@@ -28,6 +28,11 @@ import (
 
 // Scope is a namespace wrapper around a stats reporter, ensuring that
 // all emitted values have a given prefix or set of tags.
+//
+// IMPORTANT: When using Prometheus reporters, users must take care to
+//            not create metrics from both parent scopes and subscopes
+//            that have the same metric name but different tag keys,
+//            as metric allocation will panic.
 type Scope interface {
 	// Counter returns the Counter object corresponding to the name.
 	Counter(name string) Counter