Update Default Value for Jaeger Exporter Endpoint (#1824)

* Update Default Value for OTEL_EXPORTER_JAEGER_ENDPOINT Env Var

* update comments

* fix documentation

* update CHANGELOG

* add missing tab

* fix lint issue

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

CHANGELOG.md

@@ -52,6 +52,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Changed
 
+- Updated Jaeger Environment Variable: `OTEL_EXPORTER_JAEGER_ENDPOINT` to have a default value of 
+  `http://localhost:14250` when not set, in compliance with OTel spec. Changed the function `WithCollectorEndpoint`
+  in the Jaeger exporter package to no longer accept an endpoint as an argument. 
+  The endpoint can be passed in as a `CollectorEndpointOption` using the `WithEndpoint` function or 
+  specified through the `OTEL_EXPORTER_JAEGER_ENDPOINT` environment variable. (#1824)
 - Modify Zipkin Exporter default service name, use default resouce's serviceName instead of empty. (#1777)
 - Updated Jaeger Environment Variables: `JAEGER_ENDPOINT`, `JAEGER_USER`, `JAEGER_PASSWORD`
   to `OTEL_EXPORTER_JAEGER_ENDPOINT`, `OTEL_EXPORTER_JAEGER_USER`, `OTEL_EXPORTER_JAEGER_PASSWORD` 
@@ -80,6 +85,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Removed
 
+- Removed the functions `CollectorEndpointFromEnv` and `WithCollectorEndpointOptionFromEnv` from the Jaeger exporter.
+  These functions for retrieving specific environment variable values are redundant of other internal functions and
+  are not intended for end user use. (#1824)
 - Removed Jaeger Environment variables: `JAEGER_SERVICE_NAME`, `JAEGER_DISABLED`, `JAEGER_TAGS`
   These environment variables will no longer be used to override values of the Jaeger exporter (#1752)
 - No longer set the links for a `Span` in `go.opentelemetry.io/otel/sdk/trace` that is configured to be a new root.

example/jaeger/main.go

@@ -42,7 +42,7 @@ const (
 // about the application.
 func tracerProvider(url string) (*tracesdk.TracerProvider, error) {
 	// Create the Jaeger exporter
-	exp, err := jaeger.NewRawExporter(jaeger.WithCollectorEndpoint(url))
+	exp, err := jaeger.NewRawExporter(jaeger.WithCollectorEndpoint(jaeger.WithEndpoint(url)))
 	if err != nil {
 		return nil, err
 	}

exporters/trace/jaeger/env.go

@@ -42,21 +42,3 @@ func envOr(key, defaultValue string) string {
 	}
 	return defaultValue
 }
-
-// CollectorEndpointFromEnv return environment variable value of OTEL_EXPORTER_JAEGER_ENDPOINT
-func CollectorEndpointFromEnv() string {
-	return os.Getenv(envEndpoint)
-}
-
-// WithCollectorEndpointOptionFromEnv uses environment variables to set the username and password
-// if basic auth is required.
-func WithCollectorEndpointOptionFromEnv() CollectorEndpointOption {
-	return func(o *CollectorEndpointOptions) {
-		if e := os.Getenv(envUser); e != "" {
-			o.username = e
-		}
-		if e := os.Getenv(envPassword); e != "" {
-			o.password = e
-		}
-	}
-}

exporters/trace/jaeger/env_test.go

@@ -24,6 +24,27 @@ import (
 	ottest "go.opentelemetry.io/otel/internal/internaltest"
 )
 
+func TestNewRawExporterWithDefault(t *testing.T) {
+	const (
+		collectorEndpoint = "http://localhost:14250"
+		username          = ""
+		password          = ""
+	)
+
+	// Create Jaeger Exporter with default values
+	exp, err := NewRawExporter(
+		WithCollectorEndpoint(),
+	)
+
+	assert.NoError(t, err)
+
+	require.IsType(t, &collectorUploader{}, exp.uploader)
+	uploader := exp.uploader.(*collectorUploader)
+	assert.Equal(t, collectorEndpoint, uploader.endpoint)
+	assert.Equal(t, username, uploader.username)
+	assert.Equal(t, password, uploader.password)
+}
+
 func TestNewRawExporterWithEnv(t *testing.T) {
 	const (
 		collectorEndpoint = "http://localhost"
@@ -43,7 +64,7 @@ func TestNewRawExporterWithEnv(t *testing.T) {
 
 	// Create Jaeger Exporter with environment variables
 	exp, err := NewRawExporter(
-		WithCollectorEndpoint(CollectorEndpointFromEnv(), WithCollectorEndpointOptionFromEnv()),
+		WithCollectorEndpoint(),
 	)
 
 	assert.NoError(t, err)
@@ -55,11 +76,12 @@ func TestNewRawExporterWithEnv(t *testing.T) {
 	assert.Equal(t, password, uploader.password)
 }
 
-func TestNewRawExporterWithEnvImplicitly(t *testing.T) {
+func TestNewRawExporterWithPassedOption(t *testing.T) {
 	const (
 		collectorEndpoint = "http://localhost"
 		username          = "user"
 		password          = "password"
+		optionEndpoint    = "should not be overwritten"
 	)
 
 	envStore, err := ottest.SetEnvVariables(map[string]string{
@@ -72,16 +94,16 @@ func TestNewRawExporterWithEnvImplicitly(t *testing.T) {
 		require.NoError(t, envStore.Restore())
 	}()
 
-	// Create Jaeger Exporter with environment variables
+	// Create Jaeger Exporter with passed endpoint option, should be used over envEndpoint
 	exp, err := NewRawExporter(
-		WithCollectorEndpoint("should be overwritten"),
+		WithCollectorEndpoint(WithEndpoint(optionEndpoint)),
 	)
 
 	assert.NoError(t, err)
 
 	require.IsType(t, &collectorUploader{}, exp.uploader)
 	uploader := exp.uploader.(*collectorUploader)
-	assert.Equal(t, collectorEndpoint, uploader.endpoint)
+	assert.Equal(t, optionEndpoint, uploader.endpoint)
 	assert.Equal(t, username, uploader.username)
 	assert.Equal(t, password, uploader.password)
 }
@@ -152,52 +174,43 @@ func TestEnvOrWithAgentHostPortFromEnv(t *testing.T) {
 	}
 }
 
-func TestCollectorEndpointFromEnv(t *testing.T) {
+func TestEnvOrWithCollectorEndpointOptionsFromEnv(t *testing.T) {
-	const (
-		collectorEndpoint = "http://localhost"
-	)
-
-	envStore, err := ottest.SetEnvVariables(map[string]string{
-		envEndpoint: collectorEndpoint,
-	})
-	require.NoError(t, err)
-	defer func() {
-		require.NoError(t, envStore.Restore())
-	}()
-
-	assert.Equal(t, collectorEndpoint, CollectorEndpointFromEnv())
-}
-
-func TestWithCollectorEndpointOptionFromEnv(t *testing.T) {
 	testCases := []struct {
 		name                             string
+		envEndpoint                      string
 		envUsername                      string
 		envPassword                      string
-		collectorEndpointOptions         CollectorEndpointOptions
+		defaultCollectorEndpointOptions  CollectorEndpointOptions
 		expectedCollectorEndpointOptions CollectorEndpointOptions
 	}{
 		{
 			name:        "overrides value via environment variables",
+			envEndpoint: "http://localhost:14252",
 			envUsername: "username",
 			envPassword: "password",
-			collectorEndpointOptions: CollectorEndpointOptions{
+			defaultCollectorEndpointOptions: CollectorEndpointOptions{
+				endpoint: "endpoint not to be used",
 				username: "foo",
 				password: "bar",
 			},
 			expectedCollectorEndpointOptions: CollectorEndpointOptions{
+				endpoint: "http://localhost:14252",
 				username: "username",
 				password: "password",
 			},
 		},
 		{
 			name:        "environment variables is empty, will not overwrite value",
+			envEndpoint: "",
 			envUsername: "",
 			envPassword: "",
-			collectorEndpointOptions: CollectorEndpointOptions{
+			defaultCollectorEndpointOptions: CollectorEndpointOptions{
+				endpoint: "endpoint to be used",
 				username: "foo",
 				password: "bar",
 			},
 			expectedCollectorEndpointOptions: CollectorEndpointOptions{
+				endpoint: "endpoint to be used",
 				username: "foo",
 				password: "bar",
 			},
@@ -205,6 +218,7 @@ func TestWithCollectorEndpointOptionFromEnv(t *testing.T) {
 	}
 
 	envStore := ottest.NewEnvStore()
+	envStore.Record(envEndpoint)
 	envStore.Record(envUser)
 	envStore.Record(envPassword)
 	defer func() {
@@ -212,13 +226,17 @@ func TestWithCollectorEndpointOptionFromEnv(t *testing.T) {
 	}()
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			require.NoError(t, os.Setenv(envEndpoint, tc.envEndpoint))
 			require.NoError(t, os.Setenv(envUser, tc.envUsername))
 			require.NoError(t, os.Setenv(envPassword, tc.envPassword))
 
-			f := WithCollectorEndpointOptionFromEnv()
+			endpoint := envOr(envEndpoint, tc.defaultCollectorEndpointOptions.endpoint)
-			f(&tc.collectorEndpointOptions)
+			username := envOr(envUser, tc.defaultCollectorEndpointOptions.username)
+			password := envOr(envPassword, tc.defaultCollectorEndpointOptions.password)
 
-			assert.Equal(t, tc.expectedCollectorEndpointOptions, tc.collectorEndpointOptions)
+			assert.Equal(t, tc.expectedCollectorEndpointOptions.endpoint, endpoint)
+			assert.Equal(t, tc.expectedCollectorEndpointOptions.username, username)
+			assert.Equal(t, tc.expectedCollectorEndpointOptions.password, password)
 		})
 	}
 }

exporters/trace/jaeger/jaeger_test.go

@@ -47,7 +47,7 @@ const (
 )
 
 func TestInstallNewPipeline(t *testing.T) {
-	tp, err := InstallNewPipeline(WithCollectorEndpoint(collectorEndpoint))
+	tp, err := InstallNewPipeline(WithCollectorEndpoint(WithEndpoint(collectorEndpoint)))
 	require.NoError(t, err)
 	// Ensure InstallNewPipeline sets the global TracerProvider. By default
 	// the global tracer provider will be a NoOp implementation, this checks
@@ -74,7 +74,7 @@ func TestNewExportPipelinePassthroughError(t *testing.T) {
 		},
 		{
 			name: "with collector endpoint",
-			epo:  WithCollectorEndpoint(collectorEndpoint),
+			epo:  WithCollectorEndpoint(WithEndpoint(collectorEndpoint)),
 		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
@@ -98,7 +98,7 @@ func TestNewRawExporter(t *testing.T) {
 	}{
 		{
 			name:                   "default exporter",
-			endpoint:               WithCollectorEndpoint(collectorEndpoint),
+			endpoint:               WithCollectorEndpoint(),
 			expectedServiceName:    "unknown_service",
 			expectedBufferMaxCount: bundler.DefaultBufferedByteLimit,
 			expectedBatchMaxCount:  bundler.DefaultBundleCountThreshold,
@@ -112,7 +112,7 @@ func TestNewRawExporter(t *testing.T) {
 		},
 		{
 			name:     "with buffer and batch max count",
-			endpoint: WithCollectorEndpoint(collectorEndpoint),
+			endpoint: WithCollectorEndpoint(WithEndpoint(collectorEndpoint)),
 			options: []Option{
 				WithBufferMaxCount(99),
 				WithBatchMaxCount(99),
@@ -139,32 +139,7 @@ func TestNewRawExporter(t *testing.T) {
 	}
 }
 
-func TestNewRawExporterShouldFail(t *testing.T) {
+func TestNewRawExporterUseEnvVarIfOptionUnset(t *testing.T) {
-	testCases := []struct {
-		name           string
-		endpoint       EndpointOption
-		expectedErrMsg string
-	}{
-		{
-			name:           "with empty collector endpoint",
-			endpoint:       WithCollectorEndpoint(""),
-			expectedErrMsg: "collectorEndpoint must not be empty",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			_, err := NewRawExporter(
-				tc.endpoint,
-			)
-
-			assert.Error(t, err)
-			assert.EqualError(t, err, tc.expectedErrMsg)
-		})
-	}
-}
-
-func TestNewRawExporterShouldFailIfCollectorUnset(t *testing.T) {
 	// Record and restore env
 	envStore := ottest.NewEnvStore()
 	envStore.Record(envEndpoint)
@@ -174,12 +149,11 @@ func TestNewRawExporterShouldFailIfCollectorUnset(t *testing.T) {
 
 	// If the user sets the environment variable OTEL_EXPORTER_JAEGER_ENDPOINT, endpoint will always get a value.
 	require.NoError(t, os.Unsetenv(envEndpoint))
-
 	_, err := NewRawExporter(
-		WithCollectorEndpoint(""),
+		WithCollectorEndpoint(),
 	)
 
-	assert.Error(t, err)
+	assert.NoError(t, err)
 }
 
 type testCollectorEndpoint struct {

exporters/trace/jaeger/uploader.go

@@ -17,7 +17,6 @@ package jaeger // import "go.opentelemetry.io/otel/exporters/trace/jaeger"
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -114,30 +113,31 @@ func WithAttemptReconnectingInterval(interval time.Duration) AgentEndpointOption
 	}
 }
 
-// WithCollectorEndpoint defines the full url to the Jaeger HTTP Thrift collector.
+// WithCollectorEndpoint defines the full url to the Jaeger HTTP Thrift collector. This will
-// For example, http://localhost:14268/api/traces
+// use the following environment variables for configuration if no explicit option is provided:
-func WithCollectorEndpoint(collectorEndpoint string, options ...CollectorEndpointOption) EndpointOption {
+//
+// - OTEL_EXPORTER_JAEGER_ENDPOINT is the HTTP endpoint for sending spans directly to a collector.
+// - OTEL_EXPORTER_JAEGER_USER is the username to be sent as authentication to the collector endpoint.
+// - OTEL_EXPORTER_JAEGER_PASSWORD is the password to be sent as authentication to the collector endpoint.
+//
+// The passed options will take precedence over any environment variables.
+// If neither values are provided for the endpoint, the default value of "http://localhost:14250" will be used.
+// If neither values are provided for the username or the password, they will not be set since there is no default.
+func WithCollectorEndpoint(options ...CollectorEndpointOption) EndpointOption {
 	return func() (batchUploader, error) {
-		// Overwrite collector endpoint if environment variables are available.
-		if e := CollectorEndpointFromEnv(); e != "" {
-			collectorEndpoint = e
-		}
-
-		if collectorEndpoint == "" {
-			return nil, errors.New("collectorEndpoint must not be empty")
-		}
-
 		o := &CollectorEndpointOptions{
+			endpoint:   envOr(envEndpoint, "http://localhost:14250"),
+			username:   envOr(envUser, ""),
+			password:   envOr(envPassword, ""),
 			httpClient: http.DefaultClient,
 		}
 
-		options = append(options, WithCollectorEndpointOptionFromEnv())
 		for _, opt := range options {
 			opt(o)
 		}
 
 		return &collectorUploader{
-			endpoint:   collectorEndpoint,
+			endpoint:   o.endpoint,
 			username:   o.username,
 			password:   o.password,
 			httpClient: o.httpClient,
@@ -148,24 +148,44 @@ func WithCollectorEndpoint(collectorEndpoint string, options ...CollectorEndpoin
 type CollectorEndpointOption func(o *CollectorEndpointOptions)
 
 type CollectorEndpointOptions struct {
-	// username to be used if basic auth is required.
+	// endpoint for sending spans directly to a collector.
+	endpoint string
+
+	// username to be used for authentication with the collector endpoint.
 	username string
 
-	// password to be used if basic auth is required.
+	// password to be used for authentication with the collector endpoint.
 	password string
 
 	// httpClient to be used to make requests to the collector endpoint.
 	httpClient *http.Client
 }
 
-// WithUsername sets the username to be used if basic auth is required.
+// WithEndpoint is the URL for the Jaeger collector that spans are sent to.
+// This option overrides any value set for the
+// OTEL_EXPORTER_JAEGER_ENDPOINT environment variable.
+// If this option is not passed and the environment variable is not set,
+// "http://localhost:14250" will be used by default.
+func WithEndpoint(endpoint string) CollectorEndpointOption {
+	return func(o *CollectorEndpointOptions) {
+		o.endpoint = endpoint
+	}
+}
+
+// WithUsername sets the username to be used in the authorization header sent for all requests to the collector.
+// This option overrides any value set for the
+// OTEL_EXPORTER_JAEGER_USER environment variable.
+// If this option is not passed and the environment variable is not set, no username will be set.
 func WithUsername(username string) CollectorEndpointOption {
 	return func(o *CollectorEndpointOptions) {
 		o.username = username
 	}
 }
 
-// WithPassword sets the password to be used if basic auth is required.
+// WithPassword sets the password to be used in the authorization header sent for all requests to the collector.
+// This option overrides any value set for the
+// OTEL_EXPORTER_JAEGER_PASSWORD environment variable.
+// If this option is not passed and the environment variable is not set, no password will be set.
 func WithPassword(password string) CollectorEndpointOption {
 	return func(o *CollectorEndpointOptions) {
 		o.password = password