Fix per-signal endpoint parsing in OTLP exporters (open-telemetry#2433)

* Fix per-signal endpoint parsing in otlptrace * Add distinguishing non-per-signal env var value * Fix per-signal endpoint parsing in otlpmetric * Add changes to changelog * Use else if to prevent unnecessary work * Add spec link to otlptrace README
mshuler · Dec 9, 2021 · 9312664 · 9312664
1 parent b177541
commit 9312664
Show file tree

Hide file tree

Showing 8 changed files with 168 additions and 124 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - The `"go.opentelemetry.io/otel/exporter/otel/otlpmetric/otlpmetricgrpc".RetrySettings` type is renamed to `RetryConfig`. (#2425)
 - The `go.opentelemetry.io/otel/exporter/otel/*` gRPC exporters now default to using the host's root CA set if none are provided by the user and `WithInsecure` is not specified. (#1584, #2432)
 
+### Fixed
+
+- The `go.opentelemetry.io/otel/exporter/otel/*` exporters are updated to handle per-signal and universal endpoints according to the OpenTelemetry specification.
+  Any per-signal endpoint set via an `OTEL_EXPORTER_OTLP_<signal>_ENDPOINT` environment variable is now used without modification of the path.
+  When `OTEL_EXPORTER_OTLP_ENDPOINT` is set, if it contains a path, that path is used as a base path which per-signal paths are appended to. (#2338 #2433)
+
 ### Deprecated
 
 - Deprecated the `"go.opentelemetry.io/otel/exporter/otel/otlpmetric/otlpmetrichttp".WithMaxAttempts` `Option`, use the new `WithRetry` `Option` instead. (#2425)

diff --git a/exporters/otlp/otlpmetric/internal/otlpconfig/envconfig.go b/exporters/otlp/otlpmetric/internal/otlpconfig/envconfig.go
@@ -20,22 +20,18 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
-	"regexp"
+	"path"
 	"strconv"
 	"strings"
 	"time"
 
 	"go.opentelemetry.io/otel"
 )
 
-var (
-	httpSchemeRegexp = regexp.MustCompile(`(?i)^(http://|https://)`)
-
-	DefaultEnvOptionsReader = EnvOptionsReader{
-		GetEnv:   os.Getenv,
-		ReadFile: ioutil.ReadFile,
-	}
-)
+var DefaultEnvOptionsReader = EnvOptionsReader{
+	GetEnv:   os.Getenv,
+	ReadFile: ioutil.ReadFile,
+}
 
 func ApplyGRPCEnvConfigs(cfg *Config) {
 	DefaultEnvOptionsReader.ApplyGRPCEnvConfigs(cfg)
@@ -68,23 +64,55 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption {
 	var opts []GenericOption
 
 	// Endpoint
-	if v, ok := e.getEnvValue("ENDPOINT"); ok {
-		if isInsecureEndpoint(v) {
-			opts = append(opts, WithInsecure())
-		} else {
-			opts = append(opts, WithSecure())
-		}
-
-		opts = append(opts, WithEndpoint(trimSchema(v)))
-	}
 	if v, ok := e.getEnvValue("METRICS_ENDPOINT"); ok {
-		if isInsecureEndpoint(v) {
-			opts = append(opts, WithInsecure())
-		} else {
-			opts = append(opts, WithSecure())
+		u, err := url.Parse(v)
+		// Ignore invalid values.
+		if err == nil {
+			// This is used to set the scheme for OTLP/HTTP.
+			if insecureSchema(u.Scheme) {
+				opts = append(opts, WithInsecure())
+			} else {
+				opts = append(opts, WithSecure())
+			}
+			opts = append(opts, newSplitOption(func(cfg *Config) {
+				cfg.Metrics.Endpoint = u.Host
+				// For endpoint URLs for OTLP/HTTP per-signal variables, the
+				// URL MUST be used as-is without any modification. The only
+				// exception is that if an URL contains no path part, the root
+				// path / MUST be used.
+				path := u.Path
+				if path == "" {
+					path = "/"
+				}
+				cfg.Metrics.URLPath = path
+			}, func(cfg *Config) {
+				// For OTLP/gRPC endpoints, this is the target to which the
+				// exporter is going to send telemetry.
+				cfg.Metrics.Endpoint = path.Join(u.Host, u.Path)
+			}))
+		}
+	} else if v, ok = e.getEnvValue("ENDPOINT"); ok {
+		u, err := url.Parse(v)
+		// Ignore invalid values.
+		if err == nil {
+			// This is used to set the scheme for OTLP/HTTP.
+			if insecureSchema(u.Scheme) {
+				opts = append(opts, WithInsecure())
+			} else {
+				opts = append(opts, WithSecure())
+			}
+			opts = append(opts, newSplitOption(func(cfg *Config) {
+				cfg.Metrics.Endpoint = u.Host
+				// For OTLP/HTTP endpoint URLs without a per-signal
+				// configuration, the passed endpoint is used as a base URL
+				// and the signals are sent to these paths relative to that.
+				cfg.Metrics.URLPath = path.Join(u.Path, DefaultMetricsPath)
+			}, func(cfg *Config) {
+				// For OTLP/gRPC endpoints, this is the target to which the
+				// exporter is going to send telemetry.
+				cfg.Metrics.Endpoint = path.Join(u.Host, u.Path)
+			}))
 		}
-
-		opts = append(opts, WithEndpoint(trimSchema(v)))
 	}
 
 	// Certificate File
@@ -134,12 +162,13 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption {
 	return opts
 }
 
-func isInsecureEndpoint(endpoint string) bool {
-	return strings.HasPrefix(strings.ToLower(endpoint), "http://") || strings.HasPrefix(strings.ToLower(endpoint), "unix://")
-}
-
-func trimSchema(endpoint string) string {
-	return httpSchemeRegexp.ReplaceAllString(endpoint, "")
+func insecureSchema(schema string) bool {
+	switch strings.ToLower(schema) {
+	case "http", "unix":
+		return true
+	default:
+		return false
+	}
 }
 
 // getEnvValue gets an OTLP environment variable value of the specified key using the GetEnv function.

diff --git a/exporters/otlp/otlpmetric/internal/otlpconfig/options_test.go b/exporters/otlp/otlpmetric/internal/otlpconfig/options_test.go
@@ -96,20 +96,30 @@ func TestConfigs(t *testing.T) {
 		{
 			name: "Test Environment Endpoint",
 			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT": "env_endpoint",
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "https://env.endpoint/prefix",
 			},
 			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_endpoint", c.Metrics.Endpoint)
+				assert.False(t, c.Metrics.Insecure)
+				if grpcOption {
+					assert.Equal(t, "env.endpoint/prefix", c.Metrics.Endpoint)
+				} else {
+					assert.Equal(t, "env.endpoint", c.Metrics.Endpoint)
+					assert.Equal(t, "/prefix/v1/metrics", c.Metrics.URLPath)
+				}
 			},
 		},
 		{
 			name: "Test Environment Signal Specific Endpoint",
 			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":         "overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "env_metrics_endpoint",
+				"OTEL_EXPORTER_OTLP_ENDPOINT":         "https://overrode.by.signal.specific/env/var",
+				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "http://env.metrics.endpoint",
 			},
 			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_metrics_endpoint", c.Metrics.Endpoint)
+				assert.True(t, c.Metrics.Insecure)
+				assert.Equal(t, "env.metrics.endpoint", c.Metrics.Endpoint)
+				if !grpcOption {
+					assert.Equal(t, "/", c.Metrics.URLPath)
+				}
 			},
 		},
 		{
@@ -154,37 +164,15 @@ func TestConfigs(t *testing.T) {
 				assert.Equal(t, false, c.Metrics.Insecure)
 			},
 		},
-		{
-			name: "Test Environment Signal Specific Endpoint",
-			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":         "http://overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "https://env_metrics_endpoint",
-			},
-			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_metrics_endpoint", c.Metrics.Endpoint)
-				assert.Equal(t, false, c.Metrics.Insecure)
-			},
-		},
-		{
-			name: "Test Environment Signal Specific Endpoint #2",
-			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":         "http://overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "env_metrics_endpoint",
-			},
-			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_metrics_endpoint", c.Metrics.Endpoint)
-				assert.Equal(t, false, c.Metrics.Insecure)
-			},
-		},
 		{
 			name: "Test Environment Signal Specific Endpoint with uppercase scheme",
 			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":         "HTTP://overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "env_metrics_endpoint",
+				"OTEL_EXPORTER_OTLP_ENDPOINT":         "HTTPS://overrode_by_signal_specific",
+				"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT": "HtTp://env_metrics_endpoint",
 			},
 			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
 				assert.Equal(t, "env_metrics_endpoint", c.Metrics.Endpoint)
-				assert.Equal(t, false, c.Metrics.Insecure)
+				assert.Equal(t, true, c.Metrics.Insecure)
 			},
 		},
 

diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/client.go b/exporters/otlp/otlpmetric/otlpmetrichttp/client.go
@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"net/url"
 	"path"
 	"strconv"
 	"strings"
@@ -199,8 +200,8 @@ func (d *client) UploadMetrics(ctx context.Context, protoMetrics []*metricpb.Res
 }
 
 func (d *client) newRequest(body []byte) (request, error) {
-	address := fmt.Sprintf("%s://%s%s", d.getScheme(), d.cfg.Endpoint, d.cfg.URLPath)
-	r, err := http.NewRequest(http.MethodPost, address, nil)
+	u := url.URL{Scheme: d.getScheme(), Host: d.cfg.Endpoint, Path: d.cfg.URLPath}
+	r, err := http.NewRequest(http.MethodPost, u.String(), nil)
 	if err != nil {
 		return request{Request: r}, err
 	}

diff --git a/exporters/otlp/otlptrace/README.md b/exporters/otlp/otlptrace/README.md
@@ -33,8 +33,10 @@ The `otlptracehttp` package implements a client for the span exporter that sends
 
 ### Environment Variables
 
-The following environment variables can be used
-(instead of options objects) to override the default configuration.
+The following environment variables can be used (instead of options objects) to
+override the default configuration. For more information about how each of
+these environment variables is interpreted, see [the OpenTelemetry
+specification](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/protocol/exporter.md).
 
 | Environment variable                                                     | Option                        | Default value                       |
 | ------------------------------------------------------------------------ |------------------------------ | ----------------------------------- |

diff --git a/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go b/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go
@@ -20,22 +20,18 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
-	"regexp"
+	"path"
 	"strconv"
 	"strings"
 	"time"
 
 	"go.opentelemetry.io/otel"
 )
 
-var (
-	httpSchemeRegexp = regexp.MustCompile(`(?i)^(http://|https://)`)
-
-	DefaultEnvOptionsReader = EnvOptionsReader{
-		GetEnv:   os.Getenv,
-		ReadFile: ioutil.ReadFile,
-	}
-)
+var DefaultEnvOptionsReader = EnvOptionsReader{
+	GetEnv:   os.Getenv,
+	ReadFile: ioutil.ReadFile,
+}
 
 func ApplyGRPCEnvConfigs(cfg *Config) {
 	DefaultEnvOptionsReader.ApplyGRPCEnvConfigs(cfg)
@@ -68,23 +64,55 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption {
 	var opts []GenericOption
 
 	// Endpoint
-	if v, ok := e.getEnvValue("ENDPOINT"); ok {
-		if isInsecureEndpoint(v) {
-			opts = append(opts, WithInsecure())
-		} else {
-			opts = append(opts, WithSecure())
-		}
-
-		opts = append(opts, WithEndpoint(trimSchema(v)))
-	}
 	if v, ok := e.getEnvValue("TRACES_ENDPOINT"); ok {
-		if isInsecureEndpoint(v) {
-			opts = append(opts, WithInsecure())
-		} else {
-			opts = append(opts, WithSecure())
+		u, err := url.Parse(v)
+		// Ignore invalid values.
+		if err == nil {
+			// This is used to set the scheme for OTLP/HTTP.
+			if insecureSchema(u.Scheme) {
+				opts = append(opts, WithInsecure())
+			} else {
+				opts = append(opts, WithSecure())
+			}
+			opts = append(opts, newSplitOption(func(cfg *Config) {
+				cfg.Traces.Endpoint = u.Host
+				// For endpoint URLs for OTLP/HTTP per-signal variables, the
+				// URL MUST be used as-is without any modification. The only
+				// exception is that if an URL contains no path part, the root
+				// path / MUST be used.
+				path := u.Path
+				if path == "" {
+					path = "/"
+				}
+				cfg.Traces.URLPath = path
+			}, func(cfg *Config) {
+				// For OTLP/gRPC endpoints, this is the target to which the
+				// exporter is going to send telemetry.
+				cfg.Traces.Endpoint = path.Join(u.Host, u.Path)
+			}))
+		}
+	} else if v, ok = e.getEnvValue("ENDPOINT"); ok {
+		u, err := url.Parse(v)
+		// Ignore invalid values.
+		if err == nil {
+			// This is used to set the scheme for OTLP/HTTP.
+			if insecureSchema(u.Scheme) {
+				opts = append(opts, WithInsecure())
+			} else {
+				opts = append(opts, WithSecure())
+			}
+			opts = append(opts, newSplitOption(func(cfg *Config) {
+				cfg.Traces.Endpoint = u.Host
+				// For OTLP/HTTP endpoint URLs without a per-signal
+				// configuration, the passed endpoint is used as a base URL
+				// and the signals are sent to these paths relative to that.
+				cfg.Traces.URLPath = path.Join(u.Path, DefaultTracesPath)
+			}, func(cfg *Config) {
+				// For OTLP/gRPC endpoints, this is the target to which the
+				// exporter is going to send telemetry.
+				cfg.Traces.Endpoint = path.Join(u.Host, u.Path)
+			}))
 		}
-
-		opts = append(opts, WithEndpoint(trimSchema(v)))
 	}
 
 	// Certificate File
@@ -133,12 +161,13 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption {
 	return opts
 }
 
-func isInsecureEndpoint(endpoint string) bool {
-	return strings.HasPrefix(strings.ToLower(endpoint), "http://") || strings.HasPrefix(strings.ToLower(endpoint), "unix://")
-}
-
-func trimSchema(endpoint string) string {
-	return httpSchemeRegexp.ReplaceAllString(endpoint, "")
+func insecureSchema(schema string) bool {
+	switch strings.ToLower(schema) {
+	case "http", "unix":
+		return true
+	default:
+		return false
+	}
 }
 
 // getEnvValue gets an OTLP environment variable value of the specified key using the GetEnv function.