Skip to content

Commit b317ebc

Browse files
rytshrytsh
authored
Fix WithTraceRequestHeader to only add attributes to traces, not metrics (#198)
1 parent a4906bd commit b317ebc

2 files changed

Lines changed: 152 additions & 7 deletions

File tree

‎interceptor.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,9 @@ func (i *Interceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
9999
carrier := propagation.HeaderCarrier(request.Header())
100100
spanKind := trace.SpanKindClient
101101
requestSpan, responseSpan := semconv.MessageTypeSent, semconv.MessageTypeReceived
102-
attributes = addHeaderAttributes(attributes, protocol, requestKey, request.Header(), i.config.requestHeaderKeys)
102+
attributesTrace := addHeaderAttributes(attributes, protocol, requestKey, request.Header(), i.config.requestHeaderKeys)
103103
traceOpts := make([]trace.SpanStartOption, 0, 4)
104-
traceOpts = append(traceOpts, trace.WithAttributes(attributes...))
104+
traceOpts = append(traceOpts, trace.WithAttributes(attributesTrace...))
105105
if !isClient {
106106
spanKind = trace.SpanKindServer
107107
requestSpan, responseSpan = semconv.MessageTypeReceived, semconv.MessageTypeSent

‎interceptor_test.go‎

Lines changed: 150 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,9 +1075,13 @@ func TestBasicFilter(t *testing.T) {
10751075
headerKey, headerVal := "Some-Header", "foobar"
10761076
spanRecorder := tracetest.NewSpanRecorder()
10771077
traceProvider := trace.NewTracerProvider(trace.WithSpanProcessor(spanRecorder))
1078-
serverInterceptor, err := NewInterceptor(WithTracerProvider(traceProvider), WithFilter(func(_ context.Context, _ connect.Spec) bool {
1079-
return false
1080-
}))
1078+
metricReader, meterProvider := setupMetrics()
1079+
serverInterceptor, err := NewInterceptor(
1080+
WithTracerProvider(traceProvider),
1081+
WithMeterProvider(meterProvider),
1082+
WithFilter(func(_ context.Context, _ connect.Spec) bool {
1083+
return false
1084+
}))
10811085
require.NoError(t, err)
10821086
pingClient, _, _ := startServer(t, []connect.HandlerOption{
10831087
connect.WithInterceptors(serverInterceptor),
@@ -1091,6 +1095,13 @@ func TestBasicFilter(t *testing.T) {
10911095
t.Error("unexpected spans recorded")
10921096
}
10931097
assertSpans(t, []wantSpans{}, spanRecorder.Ended())
1098+
1099+
// Verify no metrics are recorded when filtered out
1100+
metrics := &metricdata.ResourceMetrics{}
1101+
require.NoError(t, metricReader.Collect(context.Background(), metrics))
1102+
1103+
// Should have no scope metrics when filtered out
1104+
assert.Empty(t, metrics.ScopeMetrics, "No metrics should be recorded when filtered out")
10941105
}
10951106

10961107
func TestHeaderAttribute(t *testing.T) {
@@ -1114,16 +1125,23 @@ func TestHeaderAttribute(t *testing.T) {
11141125
attributeCumsumRes := attribute.StringSlice(cumsumResKey, attributeValue)
11151126
requestHeaderOption := WithTraceRequestHeader(pingReq, cumsumReq)
11161127
responseHeaderOption := WithTraceResponseHeader(pingRes, cumsumRes)
1128+
1129+
// Setup metrics for both server and client
1130+
serverMetricReader, serverMeterProvider := setupMetrics()
1131+
clientMetricReader, clientMeterProvider := setupMetrics()
1132+
11171133
serverInterceptor, err := NewInterceptor(
11181134
WithPropagator(propagator),
11191135
WithTracerProvider(handlerTraceProvider),
1136+
WithMeterProvider(serverMeterProvider),
11201137
requestHeaderOption,
11211138
responseHeaderOption,
11221139
)
11231140
require.NoError(t, err)
11241141
clientInterceptor, err := NewInterceptor(
11251142
WithPropagator(propagator),
11261143
WithTracerProvider(clientTraceProvider),
1144+
WithMeterProvider(clientMeterProvider),
11271145
requestHeaderOption,
11281146
responseHeaderOption,
11291147
)
@@ -1178,19 +1196,48 @@ func TestHeaderAttribute(t *testing.T) {
11781196
// Response spans from client
11791197
require.Contains(t, clientPingSpan.Attributes(), attributePingRes)
11801198
require.Contains(t, clientCumsumSpan.Attributes(), attributeCumsumRes)
1199+
1200+
// Assert server metrics - should NOT contain header metadata
1201+
assertMetrics(t, serverMetricReader, expectedMetrics{
1202+
ServerDuration: true,
1203+
ServerRequestSize: true,
1204+
ServerResponseSize: true,
1205+
NoHeaderMetadata: true,
1206+
RequiredAttrs: map[string]attribute.Value{
1207+
"rpc.system": attribute.StringValue(connectProtocol),
1208+
"rpc.service": attribute.StringValue(pingv1connect.PingServiceName),
1209+
},
1210+
})
1211+
1212+
// Assert client metrics - should NOT contain header metadata
1213+
assertMetrics(t, clientMetricReader, expectedMetrics{
1214+
ClientDuration: true,
1215+
NoHeaderMetadata: true,
1216+
RequiredAttrs: map[string]attribute.Value{
1217+
"rpc.system": attribute.StringValue(connectProtocol),
1218+
"rpc.service": attribute.StringValue(pingv1connect.PingServiceName),
1219+
},
1220+
})
11811221
}
11821222

11831223
func TestInterceptors(t *testing.T) {
11841224
t.Parallel()
11851225
const largeMessageSize = 1000
11861226
spanRecorder := tracetest.NewSpanRecorder()
11871227
traceProvider := trace.NewTracerProvider(trace.WithSpanProcessor(spanRecorder))
1188-
serverInterceptor, err := NewInterceptor(WithTracerProvider(traceProvider))
1228+
metricReader, meterProvider := setupMetrics()
1229+
serverInterceptor, err := NewInterceptor(
1230+
WithTracerProvider(traceProvider),
1231+
WithMeterProvider(meterProvider),
1232+
WithTraceRequestHeader("X-Request-Id"),
1233+
)
11891234
require.NoError(t, err)
11901235
pingClient, host, port := startServer(t, []connect.HandlerOption{
11911236
connect.WithInterceptors(serverInterceptor),
11921237
}, nil, okayPingServer())
1193-
if _, err := pingClient.Ping(context.Background(), requestOfSize(1, 0)); err != nil {
1238+
pingWithHeader := requestOfSize(1, 0)
1239+
pingWithHeader.Header().Set("X-Request-Id", "request-123")
1240+
if _, err := pingClient.Ping(context.Background(), pingWithHeader); err != nil {
11941241
t.Error(err)
11951242
}
11961243
if _, err := pingClient.Ping(context.Background(), requestOfSize(2, largeMessageSize)); err != nil {
@@ -1223,6 +1270,7 @@ func TestInterceptors(t *testing.T) {
12231270
semconv.RPCSystemKey.String(connectProtocol),
12241271
semconv.RPCServiceKey.String(pingv1connect.PingServiceName),
12251272
semconv.RPCMethodKey.String(pingMethod),
1273+
attribute.StringSlice("rpc.connect_rpc.request.metadata.x_request_id", []string{"request-123"}),
12261274
},
12271275
},
12281276
{
@@ -1254,6 +1302,19 @@ func TestInterceptors(t *testing.T) {
12541302
},
12551303
},
12561304
}, spanRecorder.Ended())
1305+
1306+
// Assert metrics - should NOT contain header metadata but should have standard RPC attributes
1307+
assertMetrics(t, metricReader, expectedMetrics{
1308+
ServerDuration: true,
1309+
ServerRequestSize: true,
1310+
ServerResponseSize: true,
1311+
NoHeaderMetadata: true,
1312+
RequiredAttrs: map[string]attribute.Value{
1313+
"rpc.system": attribute.StringValue(connectProtocol),
1314+
"rpc.service": attribute.StringValue(pingv1connect.PingServiceName),
1315+
"rpc.method": attribute.StringValue(pingMethod),
1316+
},
1317+
})
12571318
}
12581319

12591320
func TestUnaryHandlerNoTraceParent(t *testing.T) {
@@ -2306,6 +2367,90 @@ func setupMetrics() (metricsdk.Reader, *metricsdk.MeterProvider) {
23062367
return metricReader, meterProvider
23072368
}
23082369

2370+
type expectedMetrics struct {
2371+
ServerDuration bool
2372+
ServerRequestSize bool
2373+
ServerResponseSize bool
2374+
ClientDuration bool
2375+
NoHeaderMetadata bool
2376+
RequiredAttrs map[string]attribute.Value
2377+
}
2378+
2379+
// assertMetrics verifies that metrics are collected with expected attributes.
2380+
func assertMetrics(t *testing.T, metricReader metricsdk.Reader, expected expectedMetrics) {
2381+
t.Helper()
2382+
metrics := &metricdata.ResourceMetrics{}
2383+
require.NoError(t, metricReader.Collect(context.Background(), metrics))
2384+
2385+
foundMetrics := make(map[string]bool)
2386+
2387+
for _, scopeMetric := range metrics.ScopeMetrics {
2388+
for _, metric := range scopeMetric.Metrics {
2389+
switch {
2390+
case expected.ServerDuration && metric.Name == rpcServerDuration:
2391+
foundMetrics[rpcServerDuration] = true
2392+
assertMetricAttributes(t, metric, expected.NoHeaderMetadata, expected.RequiredAttrs)
2393+
case expected.ServerRequestSize && metric.Name == rpcServerRequestSize:
2394+
foundMetrics[rpcServerRequestSize] = true
2395+
assertMetricAttributes(t, metric, expected.NoHeaderMetadata, expected.RequiredAttrs)
2396+
case expected.ServerResponseSize && metric.Name == rpcServerResponseSize:
2397+
foundMetrics[rpcServerResponseSize] = true
2398+
assertMetricAttributes(t, metric, expected.NoHeaderMetadata, expected.RequiredAttrs)
2399+
case expected.ClientDuration && metric.Name == rpcClientDuration:
2400+
foundMetrics[rpcClientDuration] = true
2401+
assertMetricAttributes(t, metric, expected.NoHeaderMetadata, expected.RequiredAttrs)
2402+
}
2403+
}
2404+
}
2405+
2406+
if expected.ServerDuration {
2407+
assert.True(t, foundMetrics[rpcServerDuration], "Should find server duration metrics")
2408+
}
2409+
if expected.ServerRequestSize {
2410+
assert.True(t, foundMetrics[rpcServerRequestSize], "Should find server request size metrics")
2411+
}
2412+
if expected.ServerResponseSize {
2413+
assert.True(t, foundMetrics[rpcServerResponseSize], "Should find server response size metrics")
2414+
}
2415+
if expected.ClientDuration {
2416+
assert.True(t, foundMetrics[rpcClientDuration], "Should find client duration metrics")
2417+
}
2418+
}
2419+
2420+
func assertMetricAttributes(t *testing.T, metric metricdata.Metrics, noHeaderMetadata bool, requiredAttrs map[string]attribute.Value) {
2421+
t.Helper()
2422+
if histogram, ok := metric.Data.(metricdata.Histogram[int64]); ok {
2423+
for _, dataPoint := range histogram.DataPoints {
2424+
attrs := dataPoint.Attributes.ToSlice()
2425+
2426+
if noHeaderMetadata {
2427+
// Verify that header metadata is NOT present in metrics
2428+
for _, attr := range attrs {
2429+
assert.NotContains(t, string(attr.Key), "rpc.connect_rpc.request.metadata",
2430+
"Metric attributes should not contain request header metadata")
2431+
assert.NotContains(t, string(attr.Key), "rpc.connect_rpc.response.metadata",
2432+
"Metric attributes should not contain response header metadata")
2433+
}
2434+
}
2435+
2436+
// Verify required attributes are present
2437+
if len(requiredAttrs) > 0 {
2438+
attrMap := make(map[string]attribute.Value)
2439+
for _, attr := range attrs {
2440+
attrMap[string(attr.Key)] = attr.Value
2441+
}
2442+
for key, expectedValue := range requiredAttrs {
2443+
actualValue, exists := attrMap[key]
2444+
assert.True(t, exists, "Required attribute %s should be present", key)
2445+
if exists {
2446+
assert.Equal(t, expectedValue, actualValue, "Attribute %s should have expected value", key)
2447+
}
2448+
}
2449+
}
2450+
}
2451+
}
2452+
}
2453+
23092454
func metricResource() *resource.Resource {
23102455
return resource.NewWithAttributes("https://opentelemetry.io/schemas/1.12.0",
23112456
attribute.String("service.name", "test"),

0 commit comments

Comments
 (0)