Merge pull request #130 from launchdarkly/eb/ch81724/metrics3

(v6 - #7) refactoring of metrics code to clarify scopes of components

cmd/ld-relay/ld-relay.go

@@ -83,10 +83,6 @@ func main() {
 		os.Exit(0)
 	}
 
-	if err := r.InitializeMetrics(); err != nil {
-		loggers.Errorf("Error initializing metrics: %s", err)
-	}
-
 	errs := make(chan error)
 	defer close(errs)
 
@@ -96,7 +92,6 @@ func main() {
 		loggers.Errorf("Error starting http listener on port: %d  %s", c.Main.Port, err)
 		os.Exit(1)
 	}
-
 }
 
 func startHTTPServer(c *config.Config, r *relay.Relay, loggers ldlog.Loggers, errs chan<- error) {

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/gregjones/httpcache v0.0.0-20171119193500-2bcd89a1743f
 	github.com/hashicorp/golang-lru v0.5.3 // indirect
 	github.com/kardianos/minwinsvc v0.0.0-20151122163309-cad6b2b879b0
-	github.com/launchdarkly/eventsource v1.4.2
+	github.com/launchdarkly/eventsource v1.4.3
 	github.com/launchdarkly/gcfg v0.0.0-20160218190638-83c3f001aeeb
 	github.com/launchdarkly/go-test-helpers/v2 v2.2.0
 	github.com/pborman/uuid v0.0.0-20170612153648-e790cca94e6c
@@ -24,8 +24,9 @@ require (
 	go.opencensus.io v0.21.0
 	google.golang.org/api v0.0.0-20180717000714-0025a57598c0 // indirect
 	gopkg.in/DataDog/dd-trace-go.v1 v1.16.1 // indirect
+	gopkg.in/launchdarkly/go-sdk-common.v1 v1.0.0-20200204015611-d48d1b4f4e70
 	gopkg.in/launchdarkly/go-sdk-common.v2 v2.0.0-beta.2
-	gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.1
-	gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.1
+	gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.2
+	gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.2
 	gopkg.in/launchdarkly/go-server-sdk.v5 v5.0.0-beta.2
 )

go.sum

@@ -129,6 +129,8 @@ github.com/launchdarkly-labs/go-coverage-enforcer v1.1.0/go.mod h1:fJXIR7UJYgdXV
 github.com/launchdarkly/eventsource v0.0.0-20190617224151-6b837031c9ca/go.mod h1:y6+/PY1Az/AQyjCMsFt5mOZq1tfM0/KOQRS4f0pVe3M=
 github.com/launchdarkly/eventsource v1.4.2 h1:qLWR3FgHrFpdeHH8lJjBjxdFaZ2U+UG4zBemh/H9xJk=
 github.com/launchdarkly/eventsource v1.4.2/go.mod h1:nX3/XHrcN2ou8HBgjqBTdv22Gl5KoMnzviwrwyGZh+k=
+github.com/launchdarkly/eventsource v1.4.3 h1:G0s/nlctuI6Vz/BKpNLE2L/S4U0lQ/ghYqecPjUuQIA=
+github.com/launchdarkly/eventsource v1.4.3/go.mod h1:LHxSeb4OnqznNZxCSXbFghxS/CjIQfzHovNoAqbO/Wk=
 github.com/launchdarkly/gcfg v0.0.0-20160218190638-83c3f001aeeb h1:a+uuDT3nURKt4laWk3PF7gxUuQfZ3XNE51AiOKjj5GA=
 github.com/launchdarkly/gcfg v0.0.0-20160218190638-83c3f001aeeb/go.mod h1:Y0RjGJ9V/bMra5xAA/Z0bkwdkwi/41b+/Krem9nIYs8=
 github.com/launchdarkly/go-ntlm-proxy-auth v1.0.1 h1:Iz5cg9mB/0vt5llZE+J0iGQ5+O/U8CWuRUUR7Ou8eNM=
@@ -306,8 +308,12 @@ gopkg.in/launchdarkly/go-sdk-common.v2 v2.0.0-beta.2 h1:jrbQ2R4gc3zMz83fXgbURCXA
 gopkg.in/launchdarkly/go-sdk-common.v2 v2.0.0-beta.2/go.mod h1:4l1+/AtknK5Sx6YTO9XDqrCbAXj8FgwpI2U/x6ZBIM4=
 gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.1 h1:yTN6mNmp9gAVAY7W8v1SwvZcGLo/D39SdsWSvgnYkRQ=
 gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.1/go.mod h1:Tc+WQhBX/1rJL4BN3fyHC9TkjJulBXyeA0Q44TIQ3Kw=
+gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.2 h1:1CfbJh5x/FXS9+1d2GeWNrr3k4yhXLg9rk38y04X5f0=
+gopkg.in/launchdarkly/go-sdk-events.v1 v1.0.0-beta.2/go.mod h1:1+nV60Da/7SrWsncuej9pRbGf3D5mXaGy0SECKQAUos=
 gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.1 h1:6rE8MmrF/uDJdAhooRcIJrFRzYVw1k08opyJowfa7eg=
 gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.1/go.mod h1:8zkK/rTeoXnFEiWQ5ZfTm6lRkkqMkeuRy4Gh9QEB87c=
+gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.2 h1:y1sCmPYgboPkuxYZMQ5NjK09EqKjA9s3dbna5IYS+ZM=
+gopkg.in/launchdarkly/go-server-sdk-evaluation.v1 v1.0.0-beta.2/go.mod h1:2sJ78PPDpQQKnLTu6v9+AJwrh1J995YjuNsx1J8NyFI=
 gopkg.in/launchdarkly/go-server-sdk.v5 v5.0.0-beta.2 h1:WxRc4vxgHpTWg8nEKGeJAzf5DVWoWmkx2NYPm4lLZ0c=
 gopkg.in/launchdarkly/go-server-sdk.v5 v5.0.0-beta.2/go.mod h1:ck8Oqf5p5vm703cf+X3G7N3CVkhU7AuQWfiSAqEn83M=
 gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=

internal/metrics/datadog.go

@@ -1,5 +1,3 @@
-// +build go1.10
-
 package metrics
 
 import (
@@ -8,48 +6,53 @@ import (
 	"go.opencensus.io/trace"
 
 	"github.com/launchdarkly/ld-relay/v6/config"
+	"gopkg.in/launchdarkly/go-sdk-common.v2/ldlog"
 	"gopkg.in/launchdarkly/go-sdk-common.v2/ldvalue"
 )
 
-func init() {
-	defineExporter(datadogExporterType, registerDatadogExporter)
-}
+var datadogExporterType exporterType = datadogExporterTypeImpl{}
+
+type datadogExporterTypeImpl struct{}
 
-type DatadogOptions struct {
-	Prefix    string
-	TraceAddr string
-	StatsAddr string
-	Tags      []string
+type datadogExporterImpl struct {
+	exporter *datadog.Exporter
 }
 
-func (d DatadogOptions) getType() ExporterType {
-	return datadogExporterType
+func (d datadogExporterTypeImpl) getName() string {
+	return "Datadog"
 }
 
-type DatadogConfig config.DatadogConfig
+func (d datadogExporterTypeImpl) createExporterIfEnabled(
+	mc config.MetricsConfig,
+	loggers ldlog.Loggers,
+) (exporter, error) {
+	if !mc.Datadog.Enabled {
+		return nil, nil
+	}
 
-func (c DatadogConfig) toOptions() ExporterOptions {
-	// For historical reasons, TraceAddr and StatsAddr were declared as pointers in DatadogConfig. However,
-	// if Datadog is enabled they must have non-nil values, so we have to change them to strings.
-	return DatadogOptions{
-		TraceAddr: ldvalue.NewOptionalStringFromPointer(c.TraceAddr).StringValue(),
-		StatsAddr: ldvalue.NewOptionalStringFromPointer(c.StatsAddr).StringValue(),
-		Tags:      c.Tag,
-		Prefix:    getPrefix(c.CommonMetricsConfig),
+	options := datadog.Options{
+		Namespace: getPrefix(mc.Datadog.CommonMetricsConfig),
+		Service:   getPrefix(mc.Datadog.CommonMetricsConfig),
+		TraceAddr: ldvalue.NewOptionalStringFromPointer(mc.Datadog.TraceAddr).StringValue(),
+		StatsAddr: ldvalue.NewOptionalStringFromPointer(mc.Datadog.StatsAddr).StringValue(),
+		Tags:      mc.Datadog.Tag,
 	}
+	exporter, err := datadog.NewExporter(options)
+	if err != nil {
+		return nil, err
+	}
+	return &datadogExporterImpl{exporter: exporter}, nil
 }
 
-func (c DatadogConfig) enabled() bool {
-	return c.Enabled
+func (d *datadogExporterImpl) register() error {
+	view.RegisterExporter(d.exporter)
+	trace.RegisterExporter(d.exporter)
+	return nil
 }
 
-func registerDatadogExporter(options ExporterOptions) error {
-	o := options.(DatadogOptions)
-	exporter, err := datadog.NewExporter(datadog.Options{Namespace: o.Prefix, Service: o.Prefix, TraceAddr: o.TraceAddr, StatsAddr: o.StatsAddr, Tags: o.Tags})
-	if err != nil {
-		return err
-	}
-	view.RegisterExporter(exporter)
-	trace.RegisterExporter(exporter)
+func (d *datadogExporterImpl) close() error {
+	d.exporter.Stop()
+	view.UnregisterExporter(d.exporter)
+	trace.UnregisterExporter(d.exporter)
 	return nil
 }

internal/metrics/datadog_test.go

@@ -0,0 +1,59 @@
+package metrics
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/launchdarkly/ld-relay/v6/config"
+	"gopkg.in/launchdarkly/go-sdk-common.v1/ldvalue"
+	"gopkg.in/launchdarkly/go-sdk-common.v2/ldlog"
+)
+
+func TestDatadogExporterType(t *testing.T) {
+	exporterType := datadogExporterType
+
+	t.Run("name", func(t *testing.T) {
+		assert.Equal(t, "Datadog", exporterType.getName())
+	})
+
+	t.Run("included in allExporterTypes", func(t *testing.T) {
+		assert.Contains(t, allExporterTypes(), exporterType)
+	})
+
+	t.Run("does not create exporter if Datadog is disabled", func(t *testing.T) {
+		var mc config.MetricsConfig
+		e, err := exporterType.createExporterIfEnabled(mc, ldlog.NewDisabledLoggers())
+		require.NoError(t, err)
+		assert.Nil(t, e)
+	})
+
+	t.Run("creates exporter if Datadog is enabled", func(t *testing.T) {
+		var mc config.MetricsConfig
+		mc.Datadog.Enabled = true
+		e, err := exporterType.createExporterIfEnabled(mc, ldlog.NewDisabledLoggers())
+		require.NoError(t, err)
+		assert.NotNil(t, e)
+		e.close()
+	})
+
+	t.Run("returns error for invalid stats address", func(t *testing.T) {
+		var mc config.MetricsConfig
+		mc.Datadog.Enabled = true
+		mc.Datadog.StatsAddr = ldvalue.NewOptionalString("::").AsPointer()
+		e, err := exporterType.createExporterIfEnabled(mc, ldlog.NewDisabledLoggers())
+		require.Error(t, err)
+		assert.Nil(t, e)
+	})
+
+	t.Run("registers exporter without errors", func(t *testing.T) {
+		var mc config.MetricsConfig
+		mc.Datadog.Enabled = true
+		e, err := exporterType.createExporterIfEnabled(mc, ldlog.NewDisabledLoggers())
+		require.NoError(t, err)
+		assert.NotNil(t, e)
+		defer e.close()
+		assert.NoError(t, e.register())
+	})
+}

internal/metrics/exporters.go

@@ -1,45 +1,72 @@
 package metrics
 
 import (
-	"fmt"
-
-	"go.opencensus.io/stats/view"
-
 	"github.com/launchdarkly/ld-relay/v6/config"
 	"gopkg.in/launchdarkly/go-sdk-common.v2/ldlog"
 )
 
-type ExporterType string
+// exporterType represents one of the kinds of OpenCensus exporters that we support.
+type exporterType interface {
+	// Returns the human-readable name, like "Datadog".
+	getName() string
 
-const (
-	datadogExporterType     ExporterType = "Datadog"
-	stackdriverExporterType ExporterType = "Stackdriver"
-	prometheusExporterType  ExporterType = "Prometheus"
-)
+	// Checks the MetricsConfig and *if* this type of exporter is enabled in it, constructs an
+	// implementation of the exporter interface containing the relevant configuration (but does not
+	// register it yet). If this type of exporter is not enabled, returns (nil, nil).
+	createExporterIfEnabled(config.MetricsConfig, ldlog.Loggers) (exporter, error)
+}
+
+type exporter interface {
+	// Attempts to register this exporter with OpenCensus.
+	register() error
 
-type ExporterOptions interface {
-	getType() ExporterType
+	// Attempts to unregister this exporter with OpenCensus and release any other resources it uses.
+	close() error
 }
 
-type ExporterRegisterer func(options ExporterOptions) error
+type exportersSet map[exporterType]exporter
 
-// ExporterConfig is used internally to hold options for metrics integrations.
-type ExporterConfig interface {
-	toOptions() ExporterOptions
-	enabled() bool
+func allExporterTypes() []exporterType {
+	return []exporterType{datadogExporterType, prometheusExporterType, stackdriverExporterType}
 }
 
-func ExporterOptionsFromConfig(c config.MetricsConfig) (options []ExporterOptions) {
-	exporterConfigs := []ExporterConfig{
-		DatadogConfig(c.Datadog),
-		StackdriverConfig(c.Stackdriver),
-		PrometheusConfig(c.Prometheus)}
-	for _, e := range exporterConfigs {
-		if e.enabled() {
-			options = append(options, e.toOptions())
+// Attempts to create and register all of the types of exporters in exporterTypes that are actually
+// enabled in the configuration. An error in any of them causes the whole operation to fail and
+// unregisters any that have already been registered.
+func registerExporters(
+	exporterTypes []exporterType,
+	c config.MetricsConfig,
+	loggers ldlog.Loggers,
+) (exportersSet, error) {
+	registered := make(exportersSet)
+	for _, t := range exporterTypes {
+		exporter, err := t.createExporterIfEnabled(c, loggers)
+		if err != nil {
+			loggers.Errorf("Error creating %s metrics exporter: %s", t.getName(), err)
+			closeExporters(registered, loggers)
+			return nil, err
+		}
+		if exporter != nil {
+			err := exporter.register()
+			if err != nil {
+				loggers.Errorf("Error registering %s metrics exporter: %s", t.getName(), err)
+				closeExporters(registered, loggers)
+				return nil, err
+			}
+			loggers.Infof("Successfully registered %s metrics exporter", t.getName())
+			registered[t] = exporter
+		}
+	}
+	return registered, nil
+}
+
+// Attempts to unregister and close a set of exporters. Errors are logged but do not stop the operation.
+func closeExporters(exporters exportersSet, loggers ldlog.Loggers) {
+	for t, e := range exporters {
+		if err := e.close(); err != nil {
+			loggers.Errorf("Error closing %s metrics exporter: %s", t.getName(), err)
 		}
 	}
-	return options
 }
 
 func getPrefix(c config.CommonMetricsConfig) string {
@@ -48,30 +75,3 @@ func getPrefix(c config.CommonMetricsConfig) string {
 	}
 	return defaultMetricsPrefix
 }
-
-func defineExporter(exporterType ExporterType, registerer ExporterRegisterer) {
-	exporters[exporterType] = registerer
-}
-
-func RegisterExporters(options []ExporterOptions, loggers ldlog.Loggers) (registrationErr error) {
-	registerPublicExportersOnce.Do(func() {
-		for _, o := range options {
-			exporter := exporters[o.getType()]
-			if exporter == nil {
-				registrationErr = fmt.Errorf("Got unexpected exporter type: %s", o.getType())
-				return
-			} else if err := exporter(o); err != nil {
-				registrationErr = fmt.Errorf("Could not register %s exporter: %s", o.getType(), err)
-				return
-			} else {
-				loggers.Infof("Successfully registered %s exporter.", o.getType())
-			}
-		}
-
-		err := view.Register(getPublicViews()...)
-		if err != nil {
-			registrationErr = fmt.Errorf("Error registering metrics views")
-		}
-	})
-	return registrationErr
-}