Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[otelhttp] transport metrics #3769

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2a075fd
otelhttp transport metrics
RangelReale May 3, 2023
9710398
get attributes
RangelReale May 3, 2023
706a58a
changelog
RangelReale May 4, 2023
0c1ecad
add transport test
RangelReale May 4, 2023
6c046de
fix lint
RangelReale May 4, 2023
faed864
try data race fix
RangelReale May 8, 2023
9304781
use atomic instead of mutex
RangelReale May 9, 2023
f66d205
fix grammar
RangelReale May 10, 2023
0d2323b
use semconv for http response
RangelReale May 10, 2023
d073e3c
don't add status code if error
RangelReale May 17, 2023
3602fb7
remove unused import
RangelReale May 17, 2023
50b6074
Merge branch 'main' into otelhttp-transport-metrics
RangelReale Jul 27, 2023
b4716e0
fix conflicts
RangelReale Jul 29, 2023
4e23144
Merge branch 'main' into otelhttp-transport-metrics
pellared Jul 31, 2023
250299e
Update CHANGELOG.md
pellared Jul 31, 2023
f93b5c1
Update CHANGELOG.md
pellared Jul 31, 2023
bf3503f
fix lint
RangelReale Aug 8, 2023
eafcc75
Merge branch 'main' into otelhttp-transport-metrics
RangelReale Aug 8, 2023
9fc2629
fix changelog
RangelReale Aug 8, 2023
359859c
remove check when error
RangelReale Aug 8, 2023
45ceb1d
Update CHANGELOG.md
pellared Aug 10, 2023
165ee4b
Update instrumentation/net/http/otelhttp/common.go
RangelReale Aug 15, 2023
09066a8
review fixes
RangelReale Aug 15, 2023
75c7903
Merge remote-tracking branch 'origin/otelhttp-transport-metrics' into…
RangelReale Aug 15, 2023
4df192e
Merge branch 'main' into otelhttp-transport-metrics
RangelReale Aug 15, 2023
e28520c
Merge branch 'main' into otelhttp-transport-metrics
RangelReale Aug 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
- Add metrics to HTTP transport in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#3769)

### Added

Expand Down
8 changes: 8 additions & 0 deletions instrumentation/net/http/otelhttp/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ const (
ServerLatency = "http.server.duration" // Incoming end to end duration, microseconds
)

// Client HTTP metrics.
const (
ClientRequestCount = "http.client.request_count" // Outgoing request count total

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer defined in the semantic convention. And doesn't look to be actually used in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is not used, we are supposed to get this information from the count of duration. I left it there becase the one for server was also there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the server one is still there, so I'm keeping this one also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientRequestContentLength = "http.client.request_content_length" // Outgoing request bytes total
ClientResponseContentLength = "http.client.response_content_length" // Outgoing response bytes total
RangelReale marked this conversation as resolved.
Show resolved Hide resolved
ClientLatency = "http.client.duration" // Outgoing end to end duration, microseconds
)

// Filter is a predicate used to determine whether a given http.request should
// be traced. A Filter must return true if the request should be traced.
type Filter func(*http.Request) bool
Expand Down
41 changes: 29 additions & 12 deletions instrumentation/net/http/otelhttp/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"net/http/httptrace"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/trace"
Expand All @@ -32,18 +33,20 @@ const (
// config represents the configuration options available for the http.Handler
// and http.Transport types.
type config struct {
ServerName string
Tracer trace.Tracer
Meter metric.Meter
Propagators propagation.TextMapPropagator
SpanStartOptions []trace.SpanStartOption
PublicEndpoint bool
PublicEndpointFn func(*http.Request) bool
ReadEvent bool
WriteEvent bool
Filters []Filter
SpanNameFormatter func(string, *http.Request) string
ClientTrace func(context.Context) *httptrace.ClientTrace
ServerName string
Tracer trace.Tracer
Meter metric.Meter
Propagators propagation.TextMapPropagator
SpanStartOptions []trace.SpanStartOption
PublicEndpoint bool
PublicEndpointFn func(*http.Request) bool
ReadEvent bool
WriteEvent bool
Filters []Filter
SpanNameFormatter func(string, *http.Request) string
ClientTrace func(context.Context) *httptrace.ClientTrace
GetRequestAttributes func(*http.Request) []attribute.KeyValue
GetResponseAttributes func(*http.Response) []attribute.KeyValue

TracerProvider trace.TracerProvider
MeterProvider metric.MeterProvider
Expand Down Expand Up @@ -206,3 +209,17 @@ func WithServerName(server string) Option {
c.ServerName = server
})
}

// WithRequestAttributeGetter extracts additional attributes from the request.
func WithRequestAttributeGetter(fn func(req *http.Request) []attribute.KeyValue) Option {
return optionFunc(func(o *config) {
o.GetRequestAttributes = fn
})
}

// WithResponseAttributeGetter extracts additional attributes from the response.
func WithResponseAttributeGetter(fn func(req *http.Response) []attribute.KeyValue) Option {
return optionFunc(func(o *config) {
o.GetResponseAttributes = fn
})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People may NOT want to add always the same attributes to spans and metrics. While for spans it is not a problem for metrics it leads to high cardinality.

I strongly suggest to remove this functionality from this PR. We can create a different solution in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

4 changes: 2 additions & 2 deletions instrumentation/net/http/otelhttp/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,15 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http

next.ServeHTTP(w, r.WithContext(ctx))

setAfterServeAttributes(span, bw.read, rww.written, rww.statusCode, bw.err, rww.err)
setAfterServeAttributes(span, bw.read.Load(), rww.written, rww.statusCode, bw.err, rww.err)

// Add metrics
attributes := append(labeler.Get(), semconvutil.HTTPServerRequest(h.server, r)...)
if rww.statusCode > 0 {
attributes = append(attributes, semconv.HTTPStatusCode(rww.statusCode))
}
o := metric.WithAttributes(attributes...)
h.counters[RequestContentLength].Add(ctx, bw.read, o)
h.counters[RequestContentLength].Add(ctx, bw.read.Load(), o)
h.counters[ResponseContentLength].Add(ctx, rww.written, o)

// Use floating point division here for higher precision (instead of Millisecond method).
Expand Down
128 changes: 128 additions & 0 deletions instrumentation/net/http/otelhttp/test/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,75 @@ package test

import (
"context"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"net/http/httptrace"
"runtime"
"strconv"
"strings"
"testing"

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

"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/sdk/metric"
"go.opentelemetry.io/otel/sdk/metric/metricdata"
"go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
"go.opentelemetry.io/otel/sdk/trace/tracetest"
semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
"go.opentelemetry.io/otel/trace"
)

func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we usually add helper functions at the bottom of the file (below tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

assert.Equal(t, instrumentation.Scope{
Name: "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",
Version: otelhttp.SemVersion(),
}, sm.Scope)

require.Len(t, sm.Metrics, 3)

want := metricdata.Metrics{
Name: "http.client.request_content_length",
Data: metricdata.Sum[int64]{
DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 0}},
Temporality: metricdata.CumulativeTemporality,
IsMonotonic: true,
},
}
metricdatatest.AssertEqual(t, want, sm.Metrics[0], metricdatatest.IgnoreTimestamp())

want = metricdata.Metrics{
Name: "http.client.response_content_length",
Data: metricdata.Sum[int64]{
DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 13}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is tightly coupled to the test where the function is used. Maybe it would be better just to inline the function into the test as right now it is not really reusable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is a copy/change from the handler test, maybe it can be better to leave both equals for future changes?

Temporality: metricdata.CumulativeTemporality,
IsMonotonic: true,
},
}
metricdatatest.AssertEqual(t, want, sm.Metrics[1], metricdatatest.IgnoreTimestamp())

// Duration value is not predictable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We can check that it is "non-zero" 😉
  2. Would adding a IgnoreValue option to metricdatatest help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a PR in the main repo to add this "IgnoreValue" option.

dur := sm.Metrics[2]
assert.Equal(t, "http.client.duration", dur.Name)
require.IsType(t, dur.Data, metricdata.Histogram[float64]{})
hist := dur.Data.(metricdata.Histogram[float64])
assert.Equal(t, metricdata.CumulativeTemporality, hist.Temporality)
require.Len(t, hist.DataPoints, 1)
dPt := hist.DataPoints[0]
assert.Equal(t, attrs, dPt.Attributes, "attributes")
assert.Equal(t, uint64(1), dPt.Count, "count")
assert.Equal(t, []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, dPt.Bounds, "bounds")
}

func TestTransportUsesFormatter(t *testing.T) {
prop := propagation.TraceContext{}
spanRecorder := tracetest.NewSpanRecorder()
Expand Down Expand Up @@ -84,6 +134,9 @@ func TestTransportErrorStatus(t *testing.T) {
spanRecorder := tracetest.NewSpanRecorder()
provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(spanRecorder))

reader := metric.NewManualReader()
meterProvider := metric.NewMeterProvider(metric.WithReader(reader))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes needed in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, will remove.


// Run a server and stop to make sure nothing is listening and force the error.
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
server.Close()
Expand All @@ -92,6 +145,7 @@ func TestTransportErrorStatus(t *testing.T) {
tr := otelhttp.NewTransport(
http.DefaultTransport,
otelhttp.WithTracerProvider(provider),
otelhttp.WithMeterProvider(meterProvider),
)
c := http.Client{Transport: tr}
r, err := http.NewRequest(http.MethodGet, server.URL, nil)
Expand Down Expand Up @@ -129,6 +183,16 @@ func TestTransportErrorStatus(t *testing.T) {
if got := span.Status().Description; !strings.Contains(got, errSubstr) {
t.Errorf("expected error status message on span; got: %q", got)
}

// check metrics
rm := metricdata.ResourceMetrics{}
err = reader.Collect(context.Background(), &rm)
require.NoError(t, err)
require.Len(t, rm.ScopeMetrics, 1)
require.Len(t, rm.ScopeMetrics[0].Metrics, 2) // response length isn't added on error

metricdatatest.AssertHasAttributes(t, rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64]),
semconv.HTTPStatusCode(http.StatusBadRequest))
}

func TestTransportRequestWithTraceContext(t *testing.T) {
Expand Down Expand Up @@ -238,3 +302,67 @@ func TestWithHTTPTrace(t *testing.T) {
assert.Equal(t, spans[2].SpanContext().SpanID(), spans[0].Parent().SpanID())
assert.Equal(t, spans[1].SpanContext().SpanID(), spans[2].Parent().SpanID())
}

func TestTransportMetrics(t *testing.T) {
reader := metric.NewManualReader()
meterProvider := metric.NewMeterProvider(metric.WithReader(reader))

content := []byte("Hello, world!")

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
if _, err := w.Write(content); err != nil {
t.Fatal(err)
}
}))
defer ts.Close()

r, err := http.NewRequest(http.MethodGet, ts.URL, nil)
if err != nil {
t.Fatal(err)
}

tr := otelhttp.NewTransport(
http.DefaultTransport,
otelhttp.WithMeterProvider(meterProvider),
otelhttp.WithRequestAttributeGetter(func(req *http.Request) []attribute.KeyValue {
return []attribute.KeyValue{attribute.String("test_req", "attribute_req")}
}),
otelhttp.WithResponseAttributeGetter(func(req *http.Response) []attribute.KeyValue {
return []attribute.KeyValue{attribute.String("test_resp", "attribute_resp")}
}),
)

c := http.Client{Transport: tr}
res, err := c.Do(r)
if err != nil {
t.Fatal(err)
}
require.NoError(t, res.Body.Close())

host, portStr, _ := net.SplitHostPort(r.Host)
if host == "" {
host = "127.0.0.1"
}
port, err := strconv.Atoi(portStr)
if err != nil {
port = 0
}

rm := metricdata.ResourceMetrics{}
err = reader.Collect(context.Background(), &rm)
require.NoError(t, err)
require.Len(t, rm.ScopeMetrics, 1)
attrs := attribute.NewSet(
semconv.NetPeerName(host),
semconv.NetPeerPort(port),
semconv.HTTPURL(ts.URL),
semconv.HTTPFlavorKey.String(fmt.Sprintf("1.%d", r.ProtoMinor)),
semconv.HTTPMethod("GET"),
semconv.HTTPResponseContentLength(13),
attribute.String("test_req", "attribute_req"),
attribute.String("test_resp", "attribute_resp"),
semconv.HTTPStatusCode(200),
)
assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs)
}
Loading