From e43d9c00bcdf4fcd11d66bb1b16afac60dc0240b Mon Sep 17 00:00:00 2001 From: Sai Nadendla Date: Wed, 21 Apr 2021 12:02:06 -0700 Subject: [PATCH] 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 --- CHANGELOG.md | 8 +++ example/jaeger/main.go | 2 +- exporters/trace/jaeger/env.go | 18 ------- exporters/trace/jaeger/env_test.go | 74 +++++++++++++++++---------- exporters/trace/jaeger/jaeger_test.go | 40 +++------------ exporters/trace/jaeger/uploader.go | 58 ++++++++++++++------- 6 files changed, 101 insertions(+), 99 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 479d8718488..4ac3e9d4a19 100644 --- 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 index f018852db11..de3ea1b5f41 100644 --- 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 index 92d4a817416..96f2d51c2f1 100644 --- 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 index 12c7828345c..2890a4583c5 100644 --- 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,52 +174,43 @@ 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", }, @@ -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() - 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 index 612eecc9066..d2818e4f5e9 100644 --- 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 { diff --git a/exporters/trace/jaeger/uploader.go b/exporters/trace/jaeger/uploader.go index 2d9eefdbbb4..fefc50f0601 100644 --- a/exporters/trace/jaeger/uploader.go +++ b/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. -// For example, http://localhost:14268/api/traces -func WithCollectorEndpoint(collectorEndpoint string, options ...CollectorEndpointOption) EndpointOption { +// WithCollectorEndpoint defines the full url to the Jaeger HTTP Thrift collector. This will +// use the following environment variables for configuration if no explicit option is provided: +// +// - 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