fix(otel): Better handling for HTTP span attributes (#610)

getsentry · Mar 30, 2023 · a97bb8b · a97bb8b
1 parent 8fdc323
commit a97bb8b
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 16 deletions.
diff --git a/otel/internal/utils/spanattributes.go b/otel/internal/utils/spanattributes.go
@@ -4,7 +4,7 @@ package utils
 
 import (
 	"fmt"
-	"strings"
+	"net/url"
 
 	"github.com/getsentry/sentry-go"
 	otelSdkTrace "go.opentelemetry.io/otel/sdk/trace"
@@ -77,44 +77,53 @@ func descriptionForDbSystem(s otelSdkTrace.ReadOnlySpan) SpanAttributes {
 }
 
 func descriptionForHttpMethod(s otelSdkTrace.ReadOnlySpan) SpanAttributes {
-	opParts := []string{"http"}
-
+	op := "http"
 	switch s.SpanKind() {
 	case otelTrace.SpanKindClient:
-		opParts = append(opParts, "client")
+		op = "http.client"
 	case otelTrace.SpanKindServer:
-		opParts = append(opParts, "server")
+		op = "http.server"
 	}
 
 	var httpTarget string
 	var httpRoute string
 	var httpMethod string
+	var httpUrl string
 
 	for _, attribute := range s.Attributes() {
-		if attribute.Key == semconv.HTTPTargetKey {
+		switch attribute.Key {
+		case semconv.HTTPTargetKey:
 			httpTarget = attribute.Value.AsString()
-			break
-		}
-		if attribute.Key == semconv.HTTPRouteKey {
+		case semconv.HTTPRouteKey:
 			httpRoute = attribute.Value.AsString()
-			break
-		}
-		if attribute.Key == semconv.HTTPMethodKey {
+		case semconv.HTTPMethodKey:
 			httpMethod = attribute.Value.AsString()
-			break
+		case semconv.HTTPURLKey:
+			httpUrl = attribute.Value.AsString()
 		}
 	}
 
 	var httpPath string
 	if httpTarget != "" {
-		httpPath = httpTarget
+		if parsedUrl, err := url.Parse(httpTarget); err == nil {
+			// Do not include the query and fragment parts
+			httpPath = parsedUrl.Path
+		} else {
+			httpPath = httpTarget
+		}
 	} else if httpRoute != "" {
 		httpPath = httpRoute
+	} else if httpUrl != "" {
+		// This is normally the HTTP-client case
+		if parsedUrl, err := url.Parse(httpUrl); err == nil {
+			// Do not include the query and fragment parts
+			httpPath = fmt.Sprintf("%s://%s%s", parsedUrl.Scheme, parsedUrl.Host, parsedUrl.Path)
+		}
 	}
 
 	if httpPath == "" {
 		return SpanAttributes{
-			Op:          strings.Join(opParts[:], "."),
+			Op:          op,
 			Description: s.Name(),
 			Source:      sentry.SourceCustom,
 		}
@@ -132,7 +141,7 @@ func descriptionForHttpMethod(s otelSdkTrace.ReadOnlySpan) SpanAttributes {
 	}
 
 	return SpanAttributes{
-		Op:          strings.Join(opParts[:], "."),
+		Op:          op,
 		Description: description,
 		Source:      source,
 	}

diff --git a/otel/span_processor_test.go b/otel/span_processor_test.go
@@ -327,3 +327,77 @@ func TestOnEndDoesNotFinishSentryRequests(t *testing.T) {
 	events := sentryTransport.Events()
 	assertEqual(t, len(events), 0)
 }
+
+func TestParseSpanAttributesHttpClient(t *testing.T) {
+	_, _, tracer := setupSpanProcessorTest()
+	ctx, otelRootSpan := tracer.Start(
+		emptyContextWithSentry(),
+		"rootSpan",
+		trace.WithSpanKind(trace.SpanKindClient),
+		trace.WithAttributes(attribute.String("http.method", "GET")),
+		trace.WithAttributes(attribute.String("http.url", "http://localhost:1234/api/checkout1?q1=q2#fragment")),
+	)
+	_, otelChildSpan := tracer.Start(ctx,
+		"childSpan",
+		trace.WithSpanKind(trace.SpanKindClient),
+		trace.WithAttributes(attribute.String("http.method", "POST")),
+		trace.WithAttributes(attribute.String("http.url", "http://localhost:2345/api/checkout2?q1=q2#fragment")),
+	)
+
+	sentryTransaction, _ := sentrySpanMap.Get(otelRootSpan.SpanContext().SpanID())
+	sentrySpan, _ := sentrySpanMap.Get(otelChildSpan.SpanContext().SpanID())
+
+	otelChildSpan.End()
+	otelRootSpan.End()
+
+	// Transaction
+	assertEqual(t, sentryTransaction.Name, "GET http://localhost:1234/api/checkout1")
+	assertEqual(t, sentryTransaction.Description, "")
+	assertEqual(t, sentryTransaction.Op, "http.client")
+	assertEqual(t, sentryTransaction.Source, sentry.TransactionSource("url"))
+
+	// Span
+	assertEqual(t, sentrySpan.Name, "")
+	assertEqual(t, sentrySpan.Description, "POST http://localhost:2345/api/checkout2")
+	assertEqual(t, sentrySpan.Op, "http.client")
+	assertEqual(t, sentrySpan.Source, sentry.TransactionSource(""))
+}
+
+func TestParseSpanAttributesHttpServer(t *testing.T) {
+	_, _, tracer := setupSpanProcessorTest()
+	ctx, otelRootSpan := tracer.Start(
+		emptyContextWithSentry(),
+		"rootSpan",
+		trace.WithSpanKind(trace.SpanKindServer),
+		trace.WithAttributes(attribute.String("http.method", "GET")),
+		trace.WithAttributes(attribute.String("http.target", "/api/checkout1?k=v")),
+		// We ignore "http.url" if "http.target" is present
+		trace.WithAttributes(attribute.String("http.url", "http://localhost:1234/api/checkout?q1=q2#fragment")),
+	)
+	_, otelChildSpan := tracer.Start(
+		ctx,
+		"span name",
+		trace.WithSpanKind(trace.SpanKindServer),
+		trace.WithAttributes(attribute.String("http.method", "POST")),
+		trace.WithAttributes(attribute.String("http.target", "/api/checkout2?k=v")),
+		// We ignore "http.url" if "http.target" is present
+		trace.WithAttributes(attribute.String("http.url", "http://localhost:2345/api/checkout?q1=q2#fragment")),
+	)
+	sentryTransaction, _ := sentrySpanMap.Get(otelRootSpan.SpanContext().SpanID())
+	sentrySpan, _ := sentrySpanMap.Get(otelChildSpan.SpanContext().SpanID())
+
+	otelChildSpan.End()
+	otelRootSpan.End()
+
+	// Transaction
+	assertEqual(t, sentryTransaction.Name, "GET /api/checkout1")
+	assertEqual(t, sentryTransaction.Description, "")
+	assertEqual(t, sentryTransaction.Op, "http.server")
+	assertEqual(t, sentryTransaction.Source, sentry.TransactionSource("url"))
+
+	// Span
+	assertEqual(t, sentrySpan.Name, "")
+	assertEqual(t, sentrySpan.Description, "POST /api/checkout2")
+	assertEqual(t, sentrySpan.Op, "http.server")
+	assertEqual(t, sentrySpan.Source, sentry.TransactionSource(""))
+}