fix: adjust span kind for HTTP client (#287) (#289)

All our HTTP requests are outbound to a remote set, so we should set
the span kind to `client`.

tracing/tracing.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"strings"
 
 	"github.com/observeinc/aws-sam-apps/version"
 
@@ -17,42 +18,76 @@ import (
 	"go.opentelemetry.io/otel/sdk/trace"
 )
 
-var serviceVersionKey = attribute.Key("service.version")
+var (
+	serviceVersionKey              = attribute.Key("service.version")
+	allowedResourceAttributeParams = []string{"deployment.environment"}
+)
 
 func SetLogger(logger logr.Logger) {
 	otel.SetLogger(logger)
 }
 
 // The OTEL SDK does not handle basic auth in OTEL_EXPORTER_OTLP_ENDPOINT
 // Extract username and password and set as OTLP Headers.
-func handleOTLPEndpointAuth() error {
-	endpoint := os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")
+func UpdateOTELEnvVars(getenv func(string) string, setenv func(string, string) error) error {
+	endpoint := getenv("OTEL_EXPORTER_OTLP_ENDPOINT")
 	if endpoint == "" {
 		return nil
 	}
 
-	if u, err := url.Parse(endpoint); err != nil {
+	u, err := url.ParseRequestURI(endpoint)
+	if err != nil {
 		return fmt.Errorf("failed to parse OTEL_EXPORTER_OTLP_ENDPOINT: %w", err)
-	} else if userinfo := u.User; userinfo != nil {
+	}
+
+	if userinfo := u.User; userinfo != nil {
 		authHeader := "Bearer " + userinfo.String()
 
-		headers := os.Getenv("OTEL_EXPORTER_OTLP_HEADERS")
+		headers := getenv("OTEL_EXPORTER_OTLP_HEADERS")
 		if headers != "" {
 			headers += ","
 		}
 		headers += "Authorization=" + authHeader
 
 		// remove auth from URL
 		u.User = nil
-		os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", u.String())
-		os.Setenv("OTEL_EXPORTER_OTLP_HEADERS", headers)
+		if err := setenv("OTEL_EXPORTER_OTLP_HEADERS", headers); err != nil {
+			return fmt.Errorf("failed to set OTLP headers: %w", err)
+		}
+	}
+
+	var resourceAttributes []string
+
+	params := u.Query()
+	for _, k := range allowedResourceAttributeParams {
+		if v := params.Get(k); v != "" {
+			resourceAttributes = append(resourceAttributes, fmt.Sprintf("%s=%s", k, v))
+			params.Del(k)
+		}
 	}
+
+	if len(resourceAttributes) > 0 {
+		if existing := getenv("OTEL_RESOURCE_ATTRIBUTES"); existing != "" {
+			resourceAttributes = append(resourceAttributes, getenv("OTEL_RESOURCE_ATTRIBUTES"))
+		}
+		u.RawQuery = params.Encode()
+		if err := setenv("OTEL_RESOURCE_ATTRIBUTES", strings.Join(resourceAttributes, ",")); err != nil {
+			return fmt.Errorf("failed to set OTLP resource attributes: %w", err)
+		}
+	}
+
+	if getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != u.String() {
+		if err := setenv("OTEL_EXPORTER_OTLP_ENDPOINT", u.String()); err != nil {
+			return fmt.Errorf("failed to set OTLP endpoint: %w", err)
+		}
+	}
+
 	return nil
 }
 
 func NewTracerProvider(ctx context.Context) (*trace.TracerProvider, error) {
-	if err := handleOTLPEndpointAuth(); err != nil {
-		return nil, fmt.Errorf("failed to handle OTLP endpoint auth: %w", err)
+	if err := UpdateOTELEnvVars(os.Getenv, os.Setenv); err != nil {
+		return nil, fmt.Errorf("failed to update OTEL environment variables: %w", err)
 	}
 
 	options := []resource.Option{

tracing/tracing_test.go

@@ -0,0 +1,99 @@
+package tracing_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/observeinc/aws-sam-apps/tracing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+)
+
+func TestOTEL(t *testing.T) {
+	t.Parallel()
+
+	testcases := []struct {
+		Input     map[string]string
+		Expect    map[string]string
+		ExpectErr error
+	}{
+		{
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "hahaha",
+			},
+			ExpectErr: cmpopts.AnyError,
+		},
+		{
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test",
+			},
+			// no changes needed
+			Expect: map[string]string{},
+		},
+		{
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://user:password@localhost/test",
+			},
+			Expect: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test",
+				"OTEL_EXPORTER_OTLP_HEADERS":  "Authorization=Bearer user:password",
+			},
+		},
+		{
+			// Extend existing headers
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://user:password@localhost/test",
+				"OTEL_EXPORTER_OTLP_HEADERS":  "X-Canary=true",
+			},
+			Expect: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test",
+				"OTEL_EXPORTER_OTLP_HEADERS":  "X-Canary=true,Authorization=Bearer user:password",
+			},
+		},
+		{
+			// Extract resource attribute in query parameters
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test?deployment.environment=dev",
+			},
+			Expect: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test",
+				"OTEL_RESOURCE_ATTRIBUTES":    "deployment.environment=dev",
+			},
+		},
+		{
+			// Leave unexpected query params
+			Input: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test?timeout=1&deployment.environment=dev",
+				"OTEL_RESOURCE_ATTRIBUTES":    "something=true",
+			},
+			Expect: map[string]string{
+				"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost/test?timeout=1",
+				"OTEL_RESOURCE_ATTRIBUTES":    "deployment.environment=dev,something=true",
+			},
+		},
+	}
+
+	for i, tc := range testcases {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			t.Parallel()
+			modified := make(map[string]string)
+			getter := func(k string) string {
+				return tc.Input[k]
+			}
+			setter := func(k, v string) error {
+				modified[k] = v
+				return nil
+			}
+			err := tracing.UpdateOTELEnvVars(getter, setter)
+
+			if diff := cmp.Diff(err, tc.ExpectErr, cmpopts.EquateErrors()); diff != "" {
+				t.Error("unexpected error", diff)
+			}
+
+			if diff := cmp.Diff(modified, tc.Expect); diff != "" && tc.ExpectErr == nil {
+				t.Error("unexpected result", diff)
+			}
+		})
+	}
+}