From bd97adf0e657c5f11ae046c651e19964a0ccefbc Mon Sep 17 00:00:00 2001 From: Ionut Ovidiu Zailic Date: Wed, 29 May 2024 18:01:11 +0300 Subject: [PATCH] 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 <42@dmathieu.com> Co-authored-by: Tyler Yahn --- CHANGELOG.md | 1 + instrumentation/net/http/otelhttp/handler.go | 6 +- instrumentation/net/http/otelhttp/labeler.go | 8 +- .../net/http/otelhttp/test/transport_test.go | 74 +++++++++++++++++++ .../net/http/otelhttp/transport.go | 6 +- 5 files changed, 89 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36eff57b763..bbbe422615d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index 810b7dd769d..cdb11dd7a5e 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/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)) diff --git a/instrumentation/net/http/otelhttp/labeler.go b/instrumentation/net/http/otelhttp/labeler.go index 1548b2db636..ea504e396f1 100644 --- a/instrumentation/net/http/otelhttp/labeler.go +++ b/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 diff --git a/instrumentation/net/http/otelhttp/test/transport_test.go b/instrumentation/net/http/otelhttp/test/transport_test.go index b303f10c57f..d96aedb29ba 100644 --- a/instrumentation/net/http/otelhttp/test/transport_test.go +++ b/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"))) + } + } +} diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 8a25e586574..5c803d02dbc 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/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.