fix: custom attributes are ignored - #5084 (#5129)

OK... i opened a new PR for #5084 issue, as i shot myself in a foot
trying to modify a previous commit that I pushed under another github
user.

Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>

CHANGELOG.md

@@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Fixed
 
 - The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634)
+- Custom attributes targeting metrics recorded by the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are not ignored anymore. (#5129)
 
 ### Deprecated
 

instrumentation/net/http/otelhttp/handler.go

@@ -209,8 +209,10 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http
 		},
 	})
 
-	labeler := &Labeler{}
-	ctx = injectLabeler(ctx, labeler)
+	labeler, found := LabelerFromContext(ctx)
+	if !found {
+		ctx = ContextWithLabeler(ctx, labeler)
+	}
 
 	next.ServeHTTP(w, r.WithContext(ctx))
 

instrumentation/net/http/otelhttp/labeler.go

@@ -37,8 +37,12 @@ type labelerContextKeyType int
 
 const lablelerContextKey labelerContextKeyType = 0
 
-func injectLabeler(ctx context.Context, l *Labeler) context.Context {
-	return context.WithValue(ctx, lablelerContextKey, l)
+// ContextWithLabeler returns a new context with the provided Labeler instance.
+// Attributes added to the specified labeler will be injected into metrics
+// emitted by the instrumentation. Only one labeller can be injected into the
+// context. Injecting it multiple times will override the previous calls.
+func ContextWithLabeler(parent context.Context, l *Labeler) context.Context {
+	return context.WithValue(parent, lablelerContextKey, l)
 }
 
 // LabelerFromContext retrieves a Labeler instance from the provided context if

instrumentation/net/http/otelhttp/test/transport_test.go

@@ -476,3 +476,77 @@ func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs at
 	}
 	metricdatatest.AssertEqual(t, want, sm.Metrics[2], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue())
 }
+
+func TestCustomAttributesHandling(t *testing.T) {
+	var rm metricdata.ResourceMetrics
+	const (
+		clientRequestSize = "http.client.request.size"
+		clientDuration    = "http.client.duration"
+	)
+	ctx := context.TODO()
+	reader := metric.NewManualReader()
+	provider := metric.NewMeterProvider(metric.WithReader(reader))
+	defer func() {
+		err := provider.Shutdown(ctx)
+		if err != nil {
+			t.Errorf("Error shutting down provider: %v", err)
+		}
+	}()
+
+	transport := otelhttp.NewTransport(http.DefaultTransport, otelhttp.WithMeterProvider(provider))
+	client := http.Client{Transport: transport}
+
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer ts.Close()
+
+	r, err := http.NewRequest(http.MethodGet, ts.URL, nil)
+	require.NoError(t, err)
+	labeler := &otelhttp.Labeler{}
+	labeler.Add(attribute.String("foo", "fooValue"))
+	labeler.Add(attribute.String("bar", "barValue"))
+	ctx = otelhttp.ContextWithLabeler(ctx, labeler)
+	r = r.WithContext(ctx)
+
+	// test bonus: intententionally ignoring response to confirm that
+	// http.client.response.size metric is not recorded
+	// by the Transport.RoundTrip logic
+	_, err = client.Do(r)
+	require.NoError(t, err)
+
+	err = reader.Collect(ctx, &rm)
+	assert.NoError(t, err)
+
+	// http.client.response.size is not recorded so the assert.Len
+	// above should be 2 instead of 3(test bonus)
+	assert.Len(t, rm.ScopeMetrics[0].Metrics, 2)
+	for _, m := range rm.ScopeMetrics[0].Metrics {
+		switch m.Name {
+		case clientRequestSize:
+			d, ok := m.Data.(metricdata.Sum[int64])
+			assert.True(t, ok)
+			assert.Len(t, d.DataPoints, 1)
+			attrSet := d.DataPoints[0].Attributes
+			fooAtrr, ok := attrSet.Value(attribute.Key("foo"))
+			assert.True(t, ok)
+			assert.Equal(t, "fooValue", fooAtrr.AsString())
+			barAtrr, ok := attrSet.Value(attribute.Key("bar"))
+			assert.True(t, ok)
+			assert.Equal(t, "barValue", barAtrr.AsString())
+			assert.False(t, attrSet.HasValue(attribute.Key("baz")))
+		case clientDuration:
+			d, ok := m.Data.(metricdata.Histogram[float64])
+			assert.True(t, ok)
+			assert.Len(t, d.DataPoints, 1)
+			attrSet := d.DataPoints[0].Attributes
+			fooAtrr, ok := attrSet.Value(attribute.Key("foo"))
+			assert.True(t, ok)
+			assert.Equal(t, "fooValue", fooAtrr.AsString())
+			barAtrr, ok := attrSet.Value(attribute.Key("bar"))
+			assert.True(t, ok)
+			assert.Equal(t, "barValue", barAtrr.AsString())
+			assert.False(t, attrSet.HasValue(attribute.Key("baz")))
+		}
+	}
+}

instrumentation/net/http/otelhttp/transport.go

@@ -137,8 +137,10 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) {
 		ctx = httptrace.WithClientTrace(ctx, t.clientTrace(ctx))
 	}
 
-	labeler := &Labeler{}
-	ctx = injectLabeler(ctx, labeler)
+	labeler, found := LabelerFromContext(ctx)
+	if !found {
+		ctx = ContextWithLabeler(ctx, labeler)
+	}
 
 	r = r.Clone(ctx) // According to RoundTripper spec, we shouldn't modify the origin request.