frontend: add more attributes to the spans

This change modifies the existing middlewares to add new span attributes
and be consistent with logging.

In practice, the following attributes are added:
* `cloud.provider` (always `Azure`) [1]
* `cloud.resource_id` [1]
* `aro.subscription_id`
* `aro.resource_group`
* `aro.resource_name`
* `aro.api_version`

[1] https://pkg.go.dev/go.opentelemetry.io/otel@v1.34.0/semconv/v1.27.0

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

frontend/pkg/frontend/middleware_logging.go

@@ -4,12 +4,16 @@ package frontend
 // Licensed under the Apache License 2.0.
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"log/slog"
 	"net/http"
 	"time"
 
+	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/trace"
+
 	"github.com/Azure/ARO-HCP/internal/api"
 )
 
@@ -77,39 +81,77 @@ func MiddlewareLoggingPostMux(w http.ResponseWriter, r *http.Request, next http.
 	ctx := r.Context()
 	logger := LoggerFromContext(ctx)
 
-	attrs := getLogAttrs(r)
-	logger = slog.New(logger.Handler().WithAttrs(attrs))
-	r = r.WithContext(ContextWithLogger(ctx, logger))
+	attrs := &attributes{
+		subscriptionID: r.PathValue(PathSegmentSubscriptionID),
+		resourceGroup:  r.PathValue(PathSegmentResourceGroupName),
+		resourceName:   r.PathValue(PathSegmentResourceName),
+	}
+	attrs.addToCurrentSpan(ctx)
+	ctx = ContextWithLogger(ctx, attrs.extendLogger(logger))
+	r = r.WithContext(ctx)
 
 	next(w, r)
 }
 
-// getLogAttrs returns the additional Logging attributes based on the wildcards
-// from the matched pattern.
-func getLogAttrs(r *http.Request) []slog.Attr {
+type attributes struct {
+	subscriptionID string
+	resourceGroup  string
+	resourceName   string
+}
+
+func (a *attributes) resourceID() string {
+	if a.subscriptionID == "" || a.resourceGroup == "" || a.resourceName == "" {
+		return ""
+	}
+
+	return fmt.Sprintf(
+		"/subscriptions/%s/resourcegroups/%s/providers/%s/%s",
+		a.subscriptionID,
+		a.resourceGroup,
+		api.ClusterResourceType,
+		a.resourceName,
+	)
+}
+
+// extendLogger returns a new logger with additional Logging attributes based
+// on the wildcards from the matched pattern.
+func (a *attributes) extendLogger(logger *slog.Logger) *slog.Logger {
 	var attrs []slog.Attr
 
-	subscriptionID := r.PathValue(PathSegmentSubscriptionID)
-	if subscriptionID != "" {
-		attrs = append(attrs, slog.String("subscription_id", subscriptionID))
+	if a.subscriptionID != "" {
+		attrs = append(attrs, slog.String("subscription_id", a.subscriptionID))
 	}
 
-	resourceGroup := r.PathValue(PathSegmentResourceGroupName)
-	if resourceGroup != "" {
-		attrs = append(attrs, slog.String("resource_group", resourceGroup))
+	if a.resourceGroup != "" {
+		attrs = append(attrs, slog.String("resource_group", a.resourceGroup))
+	}
+
+	if a.resourceName != "" {
+		attrs = append(attrs, slog.String("resource_name", a.resourceName))
+	}
+
+	if resourceID := a.resourceID(); resourceID != "" {
+		attrs = append(attrs, slog.String("resource_id", resourceID))
+	}
+
+	return slog.New(logger.Handler().WithAttrs(attrs))
+}
+
+func (a *attributes) addToCurrentSpan(ctx context.Context) {
+	span := trace.SpanFromContext(ctx)
+
+	var attrs []attribute.KeyValue
+	if a.subscriptionID != "" {
+		attrs = append(attrs, attribute.String("aro.subscription_id", a.subscriptionID))
 	}
 
-	resourceName := r.PathValue(PathSegmentResourceName)
-	if resourceName != "" {
-		attrs = append(attrs, slog.String("resource_name", resourceName))
+	if a.resourceGroup != "" {
+		attrs = append(attrs, attribute.String("aro.resource_group", a.resourceGroup))
 	}
 
-	wholePath := subscriptionID != "" && resourceGroup != "" && resourceName != ""
-	if wholePath {
-		format := "/subscriptions/%s/resourcegroups/%s/providers/%s/%s"
-		resource_id := fmt.Sprintf(format, subscriptionID, resourceGroup, api.ClusterResourceType, resourceName)
-		attrs = append(attrs, slog.String("resource_id", resource_id))
+	if a.resourceName != "" {
+		attrs = append(attrs, attribute.String("aro.resource_name", a.resourceName))
 	}
 
-	return attrs
+	span.SetAttributes(attrs...)
 }

frontend/pkg/frontend/middleware_logging_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/Azure/ARO-HCP/internal/api"
 )
@@ -31,40 +32,44 @@ func TestMiddlewareLoggingPostMux(t *testing.T) {
 
 	type testCase struct {
 		name            string
-		want            []slog.Attr
+		wantLogAttrs    []slog.Attr
+		wantSpanAttrs   map[string]string
 		setReqPathValue ReqPathModifier
 	}
 
 	tests := []testCase{
 		{
 			name:            "handles the common logging attributes",
-			want:            []slog.Attr{},
+			wantLogAttrs:    []slog.Attr{},
 			setReqPathValue: noModifyReqfunc,
 		},
 		{
-			name: "handles the common attributes and the attributes for the subscription_id segment path",
-			want: []slog.Attr{slog.String("subscription_id", fakeSubscriptionId)},
+			name:          "handles the common attributes and the attributes for the subscription_id segment path",
+			wantLogAttrs:  []slog.Attr{slog.String("subscription_id", fakeSubscriptionId)},
+			wantSpanAttrs: map[string]string{"aro.subscription_id": fakeSubscriptionId},
 			setReqPathValue: func(req *http.Request) {
 				req.SetPathValue(PathSegmentSubscriptionID, fakeSubscriptionId)
 			},
 		},
 		{
-			name: "handles the common attributes and the attributes for the resourcegroupname path",
-			want: []slog.Attr{slog.String("resource_group", fakeResourceGroupName)},
+			name:          "handles the common attributes and the attributes for the resourcegroupname path",
+			wantLogAttrs:  []slog.Attr{slog.String("resource_group", fakeResourceGroupName)},
+			wantSpanAttrs: map[string]string{"aro.resource_group": fakeResourceGroupName},
 			setReqPathValue: func(req *http.Request) {
 				req.SetPathValue(PathSegmentResourceGroupName, fakeResourceGroupName)
 			},
 		},
 		{
-			name: "handles the common attributes and the attributes for the resourcegroupname path",
-			want: []slog.Attr{slog.String("resource_group", fakeResourceGroupName)},
+			name:          "handles the common attributes and the attributes for the resourcegroupname path",
+			wantLogAttrs:  []slog.Attr{slog.String("resource_group", fakeResourceGroupName)},
+			wantSpanAttrs: map[string]string{"aro.resource_group": fakeResourceGroupName},
 			setReqPathValue: func(req *http.Request) {
 				req.SetPathValue(PathSegmentResourceGroupName, fakeResourceGroupName)
 			},
 		},
 		{
 			name: "handles the common attributes and the attributes for the resourcename path, and produces the correct resourceID attribute",
-			want: []slog.Attr{
+			wantLogAttrs: []slog.Attr{
 				slog.String("subscription_id", fakeSubscriptionId),
 				slog.String("resource_group", fakeResourceGroupName),
 				slog.String("resource_name", fakeResourceName),
@@ -77,6 +82,11 @@ func TestMiddlewareLoggingPostMux(t *testing.T) {
 						api.ClusterResourceType,
 						fakeResourceName)),
 			},
+			wantSpanAttrs: map[string]string{
+				"aro.subscription_id": fakeSubscriptionId,
+				"aro.resource_group":  fakeResourceGroupName,
+				"aro.resource_name":   fakeResourceName,
+			},
 			setReqPathValue: func(req *http.Request) {
 				// assuming the PathSegmentResourceName is present in the Path
 				req.SetPathValue(PathSegmentResourceName, fakeResourceName)
@@ -99,6 +109,7 @@ func TestMiddlewareLoggingPostMux(t *testing.T) {
 			)
 
 			ctx := ContextWithLogger(context.Background(), logger)
+			ctx, sr := initSpanRecorder(ctx)
 			req, err := http.NewRequestWithContext(ctx, "GET", "http://example.com", nil)
 			assert.NoError(t, err)
 			tt.setReqPathValue(req)
@@ -114,12 +125,18 @@ func TestMiddlewareLoggingPostMux(t *testing.T) {
 
 			// Check that the contextual logger has the expected attributes.
 			lines := strings.Split(strings.TrimSuffix(buf.String(), "\n"), "\n")
-			assert.Equal(t, 1, len(lines))
+			require.Equal(t, 1, len(lines))
 
 			line := string(lines[0])
-			for _, attr := range tt.want {
+			for _, attr := range tt.wantLogAttrs {
 				assert.Contains(t, line, attr.String())
 			}
+
+			// Check that the attributes have been added to the span too.
+			ss := sr.collect()
+			require.Len(t, ss, 1)
+			span := ss[0]
+			equalSpanAttributes(t, span, tt.wantSpanAttrs)
 		})
 	}
 }

frontend/pkg/frontend/middleware_resourceid.go

@@ -8,11 +8,12 @@ import (
 	"net/http"
 
 	azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+	semconv "go.opentelemetry.io/otel/semconv/v1.27.0"
+	"go.opentelemetry.io/otel/trace"
 )
 
 // This middleware only applies to endpoints whose path form a valid Azure
 // resource ID. It should follow the MiddlewareLowercase function.
-
 func MiddlewareResourceID(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
 	ctx := r.Context()
 	logger := LoggerFromContext(ctx)
@@ -26,6 +27,9 @@ func MiddlewareResourceID(w http.ResponseWriter, r *http.Request, next http.Hand
 
 	resourceID, err := azcorearm.ParseResourceID(originalPath)
 	if err == nil {
+		span := trace.SpanFromContext(ctx)
+		span.SetAttributes(semconv.CloudResourceID(resourceID.String()))
+
 		ctx = ContextWithResourceID(ctx, resourceID)
 		r = r.WithContext(ctx)
 	} else {

frontend/pkg/frontend/middleware_resourceid_test.go

@@ -4,6 +4,7 @@ package frontend
 // Licensed under the Apache License 2.0.
 
 import (
+	"context"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
@@ -12,13 +13,16 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
+	semconv "go.opentelemetry.io/otel/semconv/v1.27.0"
 )
 
 func TestMiddlewareResourceID(t *testing.T) {
 	tests := []struct {
 		name          string
 		path          string
 		resourceTypes []string
+		expectedErr   bool
 	}{
 		{
 			name: "subscription resource",
@@ -92,6 +96,12 @@ func TestMiddlewareResourceID(t *testing.T) {
 				"Microsoft.Resources/tenants",
 			},
 		},
+		{
+			name:          "invalid path",
+			path:          "/healthz",
+			resourceTypes: []string{},
+			expectedErr:   true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -101,9 +111,13 @@ func TestMiddlewareResourceID(t *testing.T) {
 			// Convert path to simulate MiddlewareLowercase
 			url := "http://example.com" + strings.ToLower(tt.path)
 
-			request := httptest.NewRequest("GET", url, nil)
-			request = request.WithContext(ContextWithLogger(request.Context(), slog.Default()))
-			request = request.WithContext(ContextWithOriginalPath(request.Context(), tt.path))
+			ctx := context.Background()
+			ctx = ContextWithLogger(ctx, slog.Default())
+			ctx = ContextWithOriginalPath(ctx, tt.path)
+
+			ctx, sr := initSpanRecorder(ctx)
+
+			request := httptest.NewRequestWithContext(ctx, "GET", url, nil)
 
 			next := func(w http.ResponseWriter, r *http.Request) {
 				request = r // capture modified request
@@ -113,9 +127,11 @@ func TestMiddlewareResourceID(t *testing.T) {
 			MiddlewareResourceID(writer, request, next)
 
 			resourceID, err := ResourceIDFromContext(request.Context())
-			if err != nil {
-				t.Error(err)
+			if tt.expectedErr {
+				require.Error(t, err)
+				return
 			}
+			require.NoError(t, err)
 
 			resourceTypes := []string{}
 			for resourceID != nil {
@@ -126,6 +142,13 @@ func TestMiddlewareResourceID(t *testing.T) {
 			if !reflect.DeepEqual(resourceTypes, tt.resourceTypes) {
 				t.Error(cmp.Diff(resourceTypes, tt.resourceTypes))
 			}
+
+			// Check that the "cloud.resource_id" attribute has been added and isn't empty.
+			ss := sr.collect()
+			require.Len(t, ss, 1)
+			require.Len(t, ss[0].Attributes(), 1)
+			require.Equal(t, semconv.CloudResourceIDKey, ss[0].Attributes()[0].Key)
+			require.NotEmpty(t, ss[0].Attributes()[0].Value.AsString())
 		})
 	}
 }

frontend/pkg/frontend/middleware_tracing.go

@@ -22,27 +22,36 @@ import (
 // The middleware expects that the trace provider is initialized and configured
 // in advance.
 func MiddlewareTracing(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
-	otelhttp.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		var (
-			ctx    = r.Context()
-			logger = LoggerFromContext(ctx)
-		)
+	middlewareTracing(w, r, next)
+}
 
-		data, err := CorrelationDataFromContext(ctx)
-		if err != nil {
-			span := trace.SpanFromContext(ctx)
-			span.RecordError(err)
-			logger.ErrorContext(ctx, "failed to find correlation data in context", "error", err)
-			next(w, r)
-			return
-		}
+// middlewareTracing allows to modify the default otelhttp handler for the tests.
+func middlewareTracing(w http.ResponseWriter, r *http.Request, next http.HandlerFunc, opts ...otelhttp.Option) {
+	otelhttp.NewHandler(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			var (
+				ctx    = r.Context()
+				logger = LoggerFromContext(ctx)
+			)
+
+			data, err := CorrelationDataFromContext(ctx)
+			if err != nil {
+				span := trace.SpanFromContext(ctx)
+				span.RecordError(err)
+				logger.ErrorContext(ctx, "failed to find correlation data in context", "error", err)
+				next(w, r)
+				return
+			}
 
-		r = r.WithContext(
-			addCorrelationDataToSpanContext(ctx, data),
-		)
+			r = r.WithContext(
+				addCorrelationDataToSpanContext(ctx, data),
+			)
 
-		next(w, r)
-	}), fmt.Sprintf("HTTP %s", r.Method)).ServeHTTP(w, r)
+			next(w, r)
+		}),
+		fmt.Sprintf("HTTP %s", r.Method),
+		opts...,
+	).ServeHTTP(w, r)
 }
 
 // addCorrelationDataToSpanContext adds the correlation data as attributes to