Support OTEL_TRACES_EXPORTER in distro (#300)

* Support OTEL_PROPAGATORS in distro

* Revert upgrade to otel v1.3

* Isolate distro pkg to fix test failure

* Add changes to changelog

* Add distro README

* Formatting and grammar fixes for distro/README.md

* Lazy load default propagator if unspecified

* Fix WithEndpoint and WithAccessToken docs to match

* Revert Validate comment

* Simplify WithEndpoint default value doc

* Support OTEL_PROPAGATORS in distro

* Revert upgrade to otel v1.3

* Support OTEL_TRACES_EXPORTER in distro

* Add exporterConfig validation

* Lazy evaluate TraceExporterFunc

* Add config docs to distro/README.md

* Fix default OTLP endpoint

* Test for goroutine leaks with goleak

* Document WithEndpoint per exporter configuration

* Honor OTel envars before default endpoints

* Use default propagator const

* Document how invalid envar is handled

* Remove duplicate set of global TextMapPropagator

* Clean up docs

* Remove config validation

Config validation only serves to ensure the endpoint is a valid URL
currently. The OTLP exporter accepts host:port endpoints that do not
conform to this validation but are still valid. Remove the validation
and have the exporter setup perform this instead.

* Remove unused Propagator field from exporterConfig

* Test the OTLP exporter setup

* Add WithTLSConfig option

* Add WithTLSConfig docs to distro/README.md

* Move exporter init to own file

* Fix lint

* Add TestInvalidTraceExporter

* Clean up otel_test

* Test Run with defaults per exporter

* Test none exporter

* Ignore test func length linting

* Add TLS exporter tests

* Unify TLS creation

* Refactor otel_test.go

* Fix spelling error

* Add changes to changelog

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <rpajak@splunk.com>

* Log grpc Server serve error

* Do not error on "none" trace exporter value

Co-authored-by: Robert Pająk <rpajak@splunk.com>

CHANGELOG.md

@@ -8,6 +8,25 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+
+- Add the `WithPropagator` option to
+  `github.com/signalfx/splunk-otel-go/distro` along with parsing of the
+  `OTEL_PROPAGATORS` environment variable to set the global OpenTelemetry
+  `TextMapPropagator`. (#295)
+- Add the `WithTraceExporter` and `WithTLSConfig` options to
+  `github.com/signalfx/splunk-otel-go/distro` along with parsing of hte
+  `OTEL_TRACES_EXPORTER` environment variable to set the global OpenTelemetry
+  `SpanExporter` used by the `SDK` to export traces. (#300)
+
+### Changed
+
+- The `SDK` from `github.com/signalfx/splunk-otel-go/distro` now uses an OTLP
+  exporter by default. The previous default Jaeger
+  thrift exporter can still be used by setting the `OTEL_TRACES_EXPORTER`
+  environment variable to `jaeger-thrift-splunk`, or by directly passing the
+  user configured exporter with a `WithTraceExporter` option. (#300)
+
 ## [0.7.0] - 2022-01-13
 
 ### Added
@@ -61,10 +80,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Add the
   `github.com/signalfx/splunk-otel-go/instrumentation/github.com/gomodule/redigo/splunkredigo`
   instrumentation for the `github.com/gomodule/redigo` package. (#288)
-- Add the `WithPropagator` option to
-  `github.com/signalfx/splunk-otel-go/distro` along with parsing of the
-  `OTEL_PROPAGATORS` environment variable to set the global OpenTelemetry
-  `TextMapPropagator`. (#295)
 - Add the
   `github.com/signalfx/splunk-otel-go/instrumentation/gopkg.in/olivere/elastic/splunkelastic`
   instrumentation for the `gopkg.in/olivere/elastic` package. (#311)

distro/README.md

@@ -20,6 +20,8 @@ The [`SDK`] is configured with the following options.
 | `WithAccessToken` | `""` | `SPLUNK_ACCESS_TOKEN` |
 | `WithEndpoint` | (1) | (2) |
 | `WithPropagator` | `tracecontext,baggage` | `OTEL_PROPAGATORS` |
+| `WithTraceExporter` | `otlp` | `OTEL_TRACES_EXPORTER` |
+| `WithTLSConfig` | none | n/a |
 
 (1): The default value depends on the exporter used. See the
 [`WithEndpoint`](#withendpoint) section for more details.
@@ -75,5 +77,28 @@ global `TextMapPropagator`. Setting to `nil` will prevent any global
   Values can be joined with a comma (`","`) to produce a composite
   `TextMapPropagator`.
 
+### `WithTraceExporter`
+
+`WithTraceExporter` configures the OpenTelemetry trace SpanExporter used to
+deliver telemetry. This exporter is registered with the OpenTelemetry SDK using
+a batch span processor.
+
+- Default value: an OTLP gRPC SpanExporter.
+- Environment variable: `OTEL_TRACES_EXPORTER`
+
+  The environment variable values are restricted to the following.
+  - `"otlp"`: OTLP gRPC exporter.
+  - `"jaeger-thrift-splunk"`: Jaeger thrift exporter.
+  - `"none"`: None, explicitly do not set an exporter.
+
+  Any other value will be ignored and the default used instead.
+
+### `WithTLSConfig`
+
+`WithTLSConfig` configures the TLS configuration used by the exporter.
+
+- Default value: none; an non-TLS connection is used.
+- Environment variable: n/a (only configurable in code).
+
 [`Run`]: https://pkg.go.dev/github.com/signalfx/splunk-otel-go/distro#Run
 [`SDK`]: https://pkg.go.dev/github.com/signalfx/splunk-otel-go/distro#SDK

distro/config.go

@@ -15,8 +15,7 @@
 package distro
 
 import (
-	"fmt"
-	"net/url"
+	"crypto/tls"
 	"os"
 	"strings"
 
@@ -25,6 +24,7 @@ import (
 	"go.opentelemetry.io/contrib/propagators/jaeger"
 	"go.opentelemetry.io/contrib/propagators/ot"
 	"go.opentelemetry.io/otel/propagation"
+	"go.opentelemetry.io/otel/sdk/trace"
 )
 
 // Environment variable keys that set values of the configuration.
@@ -35,22 +35,50 @@ const (
 	// OpenTelemetry TextMapPropagator to set as global.
 	otelPropagatorsKey = "OTEL_PROPAGATORS"
 
+	// OpenTelemetry trace exporter to use.
+	otelTracesExporterKey = "OTEL_TRACES_EXPORTER"
+
+	// OpenTelemetry exporter endpoints.
+	otelExporterJaegerEndpointKey     = "OTEL_EXPORTER_JAEGER_ENDPOINT"
+	otelExporterOTLPEndpointKey       = "OTEL_EXPORTER_OTLP_ENDPOINT"
+	otelExporterOTLPTracesEndpointKey = "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"
+
 	// FIXME: support OTEL_SPAN_LINK_COUNT_LIMIT
 	// FIXME: support OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT
-	// FIXME: support OTEL_TRACES_EXPORTER
 )
 
-// config is the configuration used to create and operate an SDK.
-type config struct {
+// Default configuration values.
+const (
+	defaultAccessToken   = ""
+	defaultPropagator    = "tracecontext,baggage"
+	defaultTraceExporter = "otlp"
+
+	defaultOTLPEndpoint   = "localhost:4317"
+	defaultJaegerEndpoint = "http://127.0.0.1:9080/v1/trace"
+)
+
+type exporterConfig struct {
 	AccessToken string
 	Endpoint    string
-	Propagator  propagation.TextMapPropagator
+
+	UseTLS    bool
+	TLSConfig *tls.Config
+}
+
+// config is the configuration used to create and operate an SDK.
+type config struct {
+	Propagator propagation.TextMapPropagator
+
+	ExportConfig      *exporterConfig
+	TraceExporterFunc traceExporterFunc
 }
 
 // newConfig returns a validated config with Splunk defaults.
-func newConfig(opts ...Option) (*config, error) {
+func newConfig(opts ...Option) *config {
 	c := &config{
-		AccessToken: envOr(accessTokenKey, ""),
+		ExportConfig: &exporterConfig{
+			AccessToken: envOr(accessTokenKey, defaultAccessToken),
+		},
 	}
 
 	for _, o := range opts {
@@ -60,30 +88,20 @@ func newConfig(opts ...Option) (*config, error) {
 	// Apply default field values if they were not set.
 	if c.Propagator == nil {
 		c.Propagator = loadPropagator(
-			envOr(otelPropagatorsKey, "tracecontext,baggage"),
+			envOr(otelPropagatorsKey, defaultPropagator),
 		)
 	}
-
-	if err := c.Validate(); err != nil {
-		return nil, err
-	}
-	return c, nil
-}
-
-// Validate ensures c is valid, otherwise returning an appropriate error.
-func (c *config) Validate() error {
-	var errs []string
-
-	if c.Endpoint != "" {
-		if _, err := url.Parse(c.Endpoint); err != nil {
-			errs = append(errs, "invalid endpoint: %s", err.Error())
+	if c.TraceExporterFunc == nil {
+		key := envOr(otelTracesExporterKey, defaultTraceExporter)
+		tef, ok := exporters[key]
+		if !ok {
+			// TODO: log invalid exporter value.
+			tef = exporters[defaultTraceExporter]
 		}
+		c.TraceExporterFunc = tef
 	}
 
-	if len(errs) > 0 {
-		return fmt.Errorf("invalid config: %v", errs)
-	}
-	return nil
+	return c
 }
 
 type nonePropagatorType struct{ propagation.TextMapPropagator }
@@ -188,7 +206,7 @@ func (fn optionFunc) apply(c *config) {
 // string results in the default value being used.
 func WithEndpoint(endpoint string) Option {
 	return optionFunc(func(c *config) {
-		c.Endpoint = endpoint
+		c.ExportConfig.Endpoint = endpoint
 	})
 }
 
@@ -201,7 +219,37 @@ func WithEndpoint(endpoint string) Option {
 // is not provided.
 func WithAccessToken(accessToken string) Option {
 	return optionFunc(func(c *config) {
-		c.AccessToken = accessToken
+		c.ExportConfig.AccessToken = accessToken
+	})
+}
+
+// WithTraceExporter configures the OpenTelemetry trace SpanExporter used to
+// deliver telemetry. This exporter is registered with the OpenTelemetry SDK
+// using a batch span processor.
+//
+// The OTEL_TRACES_EXPORTER environment variable value is used if this Option
+// is not provided. Valid values for this environment variable are "otlp" for
+// an OTLP exporter, and "jaeger-thrift-splunk" for a Splunk specific Jaeger
+// thrift exporter. If this environment variable is set to "none", no exporter
+// is registered.
+//
+// By default, an OTLP exporter is used if this is not provided or the
+// OTEL_TRACES_EXPORTER environment variable is not set.
+func WithTraceExporter(e trace.SpanExporter) Option {
+	return optionFunc(func(c *config) {
+		c.TraceExporterFunc = func(*exporterConfig) (trace.SpanExporter, error) {
+			return e, nil
+		}
+	})
+}
+
+// WithTLSConfig configures the TLS configuration used by the exporter.
+//
+// If this option is not provided, the exporter connection will not use TLS.
+func WithTLSConfig(conf *tls.Config) Option {
+	return optionFunc(func(c *config) {
+		c.ExportConfig.UseTLS = true
+		c.ExportConfig.TLSConfig = conf
 	})
 }
 

distro/config_test.go

@@ -18,7 +18,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"go.opentelemetry.io/contrib/propagators/aws/xray"
 	"go.opentelemetry.io/contrib/propagators/b3"
 	"go.opentelemetry.io/contrib/propagators/jaeger"
@@ -33,7 +32,7 @@ type KeyValue struct {
 type OptionTest struct {
 	Name          string
 	Options       []Option
-	AssertionFunc func(*testing.T, *config, error)
+	AssertionFunc func(*testing.T, *config)
 }
 
 type ConfigFieldTest struct {
@@ -48,7 +47,7 @@ var ConfigurationTests = []*ConfigFieldTest{
 	{
 		Name: "AccessToken",
 		ValueFunc: func(c *config) interface{} {
-			return c.AccessToken
+			return c.ExportConfig.AccessToken
 		},
 		DefaultValue: "",
 		EnvironmentTests: []KeyValue{
@@ -60,17 +59,16 @@ var ConfigurationTests = []*ConfigFieldTest{
 				Options: []Option{
 					WithAccessToken("secret"),
 				},
-				AssertionFunc: func(t *testing.T, c *config, e error) {
-					assert.NoError(t, e)
-					assert.Equal(t, "secret", c.AccessToken)
+				AssertionFunc: func(t *testing.T, c *config) {
+					assert.Equal(t, "secret", c.ExportConfig.AccessToken)
 				},
 			},
 		},
 	},
 	{
 		Name: "Endpoint",
 		ValueFunc: func(c *config) interface{} {
-			return c.Endpoint
+			return c.ExportConfig.Endpoint
 		},
 		DefaultValue: "",
 		OptionTests: []OptionTest{
@@ -79,18 +77,8 @@ var ConfigurationTests = []*ConfigFieldTest{
 				Options: []Option{
 					WithEndpoint("https://localhost/"),
 				},
-				AssertionFunc: func(t *testing.T, c *config, e error) {
-					assert.NoError(t, e)
-					assert.Equal(t, "https://localhost/", c.Endpoint)
-				},
-			},
-			{
-				Name: "invalid URL",
-				Options: []Option{
-					WithEndpoint("not://a valid.URL"),
-				},
-				AssertionFunc: func(t *testing.T, c *config, e error) {
-					assert.Error(t, e)
+				AssertionFunc: func(t *testing.T, c *config) {
+					assert.Equal(t, "https://localhost/", c.ExportConfig.Endpoint)
 				},
 			},
 		},
@@ -113,8 +101,7 @@ var ConfigurationTests = []*ConfigFieldTest{
 				Options: []Option{
 					WithPropagator(nil),
 				},
-				AssertionFunc: func(t *testing.T, c *config, e error) {
-					assert.NoError(t, e)
+				AssertionFunc: func(t *testing.T, c *config) {
 					assert.Equal(t, nonePropagator, c.Propagator)
 				},
 			},
@@ -123,8 +110,7 @@ var ConfigurationTests = []*ConfigFieldTest{
 				Options: []Option{
 					WithPropagator(propagation.TraceContext{}),
 				},
-				AssertionFunc: func(t *testing.T, c *config, e error) {
-					assert.NoError(t, e)
+				AssertionFunc: func(t *testing.T, c *config) {
 					assert.Equal(t, propagation.TraceContext{}, c.Propagator)
 				},
 			},
@@ -137,9 +123,7 @@ func TestConfig(t *testing.T) {
 		func(t *testing.T, tc *ConfigFieldTest) {
 			t.Run(tc.Name, func(t *testing.T) {
 				t.Run("DefaultValue", func(t *testing.T) {
-					c, err := newConfig()
-					require.NoError(t, err)
-					assert.Equal(t, tc.DefaultValue, tc.ValueFunc(c))
+					assert.Equal(t, tc.DefaultValue, tc.ValueFunc(newConfig()))
 				})
 
 				t.Run("EnvironmentVariableOverride", func(t *testing.T) {
@@ -160,15 +144,11 @@ func testEnvironmentOverrides(t *testing.T, tc *ConfigFieldTest) {
 			revert := Setenv(key, val)
 			defer revert()
 
-			c, err := newConfig()
-			if !assert.NoError(t, err) {
-				return
-			}
 			// The expected type is not known, but we can check that the value
 			// has changed to verify the environment variable influenced the
 			// configuration.
 			assert.NotEqual(
-				t, tc.DefaultValue, tc.ValueFunc(c),
+				t, tc.DefaultValue, tc.ValueFunc(newConfig()),
 				"environment variable %s=%q unused", key, val,
 			)
 		}(ev.Key, ev.Value)
@@ -179,8 +159,7 @@ func testOptions(t *testing.T, tc *ConfigFieldTest) {
 	for _, o := range tc.OptionTests {
 		func(t *testing.T, o OptionTest) {
 			t.Run(o.Name, func(t *testing.T) {
-				c, err := newConfig(o.Options...)
-				o.AssertionFunc(t, c, err)
+				o.AssertionFunc(t, newConfig(o.Options...))
 			})
 		}(t, o)
 	}