Fix per-signal endpoint parsing in otlptrace

open-telemetry · Dec 7, 2021 · 0002114 · 0002114
1 parent ee60bcc
commit 0002114
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 50 deletions.
diff --git a/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go b/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go
@@ -20,16 +20,14 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
-	"regexp"
+	"path"
 	"strconv"
 	"strings"
 	"time"
 
 	"go.opentelemetry.io/otel"
 )
 
-var httpSchemeRegexp = regexp.MustCompile(`(?i)^(http://|https://)`)
-
 func ApplyGRPCEnvConfigs(cfg *Config) {
 	e := EnvOptionsReader{
 		GetEnv:   os.Getenv,
@@ -72,22 +70,55 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption {
 
 	// Endpoint
 	if v, ok := e.getEnvValue("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 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)))
 	}
 	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)
+			}))
 		}
-
-		opts = append(opts, WithEndpoint(trimSchema(v)))
 	}
 
 	// Certificate File
@@ -136,12 +167,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/otlptrace/internal/otlpconfig/options_test.go b/exporters/otlp/otlptrace/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.Traces.Endpoint)
+				assert.False(t, c.Traces.Insecure)
+				if grpcOption {
+					assert.Equal(t, "env.endpoint/prefix", c.Traces.Endpoint)
+				} else {
+					assert.Equal(t, "env.endpoint", c.Traces.Endpoint)
+					assert.Equal(t, "/prefix/v1/traces", c.Traces.URLPath)
+				}
 			},
 		},
 		{
 			name: "Test Environment Signal Specific Endpoint",
 			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":        "overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "env_traces_endpoint",
+				"OTEL_EXPORTER_OTLP_ENDPOINT":        "https://overrode.by.signal.specific/env/var",
+				"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env.traces.endpoint",
 			},
 			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint)
+				assert.True(t, c.Traces.Insecure)
+				assert.Equal(t, "env.traces.endpoint", c.Traces.Endpoint)
+				if !grpcOption {
+					assert.Equal(t, "/", c.Traces.URLPath)
+				}
 			},
 		},
 		{
@@ -154,28 +164,6 @@ func TestConfigs(t *testing.T) {
 				assert.Equal(t, false, c.Traces.Insecure)
 			},
 		},
-		{
-			name: "Test Environment Signal Specific Endpoint",
-			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":        "http://overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env_traces_endpoint",
-			},
-			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint)
-				assert.Equal(t, true, c.Traces.Insecure)
-			},
-		},
-		{
-			name: "Test Environment Signal Specific Endpoint #2",
-			env: map[string]string{
-				"OTEL_EXPORTER_OTLP_ENDPOINT":        "http://overrode_by_signal_specific",
-				"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env_traces_endpoint",
-			},
-			asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) {
-				assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint)
-				assert.Equal(t, true, c.Traces.Insecure)
-			},
-		},
 		{
 			name: "Test Environment Signal Specific Endpoint with uppercase scheme",
 			env: map[string]string{

diff --git a/exporters/otlp/otlptrace/otlptracehttp/client.go b/exporters/otlp/otlptrace/otlptracehttp/client.go
@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"net/url"
 	"path"
 	"strconv"
 	"strings"
@@ -201,8 +202,8 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc
 }
 
 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
 	}