Update Default Value for Jaeger Exporter Endpoint (open-telemetry#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 <[email protected]>
atlassian-forks · Apr 21, 2021 · e43d9c0 · e43d9c0
1 parent 0032bd6
commit e43d9c0
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 99 deletions.
diff --git a/CHANGELOG.md b/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.

diff --git a/example/jaeger/main.go b/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
 	}

diff --git a/exporters/trace/jaeger/env.go b/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
-		}
-	}
-}
diff --git a/exporters/trace/jaeger/env_test.go b/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,73 +174,69 @@ func TestEnvOrWithAgentHostPortFromEnv(t *testing.T) {
 	}
 }
 
-func TestCollectorEndpointFromEnv(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) {
+func TestEnvOrWithCollectorEndpointOptionsFromEnv(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",
 			},
 		},
 	}
 
 	envStore := ottest.NewEnvStore()
+	envStore.Record(envEndpoint)
 	envStore.Record(envUser)
 	envStore.Record(envPassword)
 	defer func() {
 		require.NoError(t, envStore.Restore())
 	}()
 	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()
-			f(&tc.collectorEndpointOptions)
+			endpoint := envOr(envEndpoint, tc.defaultCollectorEndpointOptions.endpoint)
+			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)
 		})
 	}
 }
diff --git a/exporters/trace/jaeger/jaeger_test.go b/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) {
-	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) {
+func TestNewRawExporterUseEnvVarIfOptionUnset(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 {