Skip to content

Commit

Permalink
frontend: add more attributes to the spans
Browse files Browse the repository at this point in the history 
This change modifies the existing middlewares to add new span attributes
and be consistent with logging.

In practice, the following resource attributes are added:
* `cloud.provider` (always `Azure`) [1]
* `cloud.region` [1]

The following span attributes are added:
* `cloud.resource_id` [1]
* `aro.subscription.id`
* `aro.subscription.state`
* `aro.resource_group.name`
* `aro.resource.name`
* `aro.api_version`

The following span attributes have also been renamed to better match the
guidelines on attribute naming (TL;DR: use dot separators to delimit
namespaces and underscores for multi-word names):
* `aro.correlation.id` -> `aro.correlation_id`
* `aro.client.request.id` -> `aro.client.request_id`
* `aro.request.id` -> `aro.request_id`

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

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
  • Loading branch information
@simonpasquier
simonpasquier committed Feb 25, 2025
1 parent c7c4633 commit 769ff74
Show file tree
Hide file tree
Showing 16 changed files with 315 additions and 103 deletions.
3 changes: 2 additions & 1 deletion frontend/cmd/cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/spf13/cobra"
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"go.opentelemetry.io/otel"
semconv "go.opentelemetry.io/otel/semconv/v1.27.0"

"github.com/Azure/ARO-HCP/frontend/pkg/frontend"
"github.com/Azure/ARO-HCP/frontend/pkg/util"
Expand Down Expand Up @@ -112,7 +113,7 @@ func (opts *FrontendOpts) Run() error {
prometheusEmitter := frontend.NewPrometheusEmitter(prometheus.DefaultRegisterer)

// Initialize the global OpenTelemetry tracer.
otelShutdown, err := frontend.ConfigureOpenTelemetryTracer(ctx, logger)
otelShutdown, err := frontend.ConfigureOpenTelemetryTracer(ctx, logger, semconv.CloudRegion(opts.location))
if err != nil {
return fmt.Errorf("could not initialize opentelemetry sdk: %w", err)
}
Expand Down
85 changes: 64 additions & 21 deletions frontend/pkg/frontend/middleware_logging.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ 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"
"github.com/Azure/ARO-HCP/internal/tracing"
)

type LoggingReadCloser struct {
Expand Down Expand Up @@ -77,39 +82,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, tracing.SubscriptionIDKey.String(a.subscriptionID))
}

resourceName := r.PathValue(PathSegmentResourceName)
if resourceName != "" {
attrs = append(attrs, slog.String("resource_name", resourceName))
if a.resourceGroup != "" {
attrs = append(attrs, tracing.ResourceGroupNameKey.String(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, tracing.ResourceNameKey.String(a.resourceName))
}

return attrs
span.SetAttributes(attrs...)
}
39 changes: 28 additions & 11 deletions frontend/pkg/frontend/middleware_logging_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/Azure/ARO-HCP/internal/api"
)
Expand All @@ -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.name": 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.name": 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),
Expand All @@ -77,6 +82,11 @@ func TestMiddlewareLoggingPostMux(t *testing.T) {
api.ClusterResourceType,
fakeResourceName)),
},
wantSpanAttrs: map[string]string{
"aro.subscription.id": fakeSubscriptionId,
"aro.resource_group.name": fakeResourceGroupName,
"aro.resource.name": fakeResourceName,
},
setReqPathValue: func(req *http.Request) {
// assuming the PathSegmentResourceName is present in the Path
req.SetPathValue(PathSegmentResourceName, fakeResourceName)
Expand All @@ -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)
Expand All @@ -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)
})
}
}
6 changes: 5 additions & 1 deletion frontend/pkg/frontend/middleware_resourceid.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
33 changes: 28 additions & 5 deletions frontend/pkg/frontend/middleware_resourceid_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package frontend
// Licensed under the Apache License 2.0.

import (
"context"
"log/slog"
"net/http"
"net/http/httptest"
Expand All @@ -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",
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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())
})
}
}
Loading

0 comments on commit 769ff74

Please sign in to comment.