From a77b9d08098dba88e8ad1c2354657ba6f43755b3 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 08:39:48 +0800 Subject: [PATCH 01/18] feat(otelhttp): Added 1.26 metrics support for HTTP server instrumentation --- .../net/http/otelhttp/internal/semconv/env.go | 37 +++++--- .../otelhttp/internal/semconv/httpconv.go | 94 +++++++++++++++++++ 2 files changed, 118 insertions(+), 13 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index fec528536a5..58c0e656628 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -30,6 +30,11 @@ type HTTPServer struct { requestBytesCounter metric.Int64Counter responseBytesCounter metric.Int64Counter serverLatencyMeasure metric.Float64Histogram + + // New metrics + requestDurationHistogram metric.Float64Histogram + requestBodySizeHistogram metric.Int64Histogram + responseBodySizeHistogram metric.Int64Histogram } // RequestTraceAttrs returns trace attributes for an HTTP request received by a @@ -103,19 +108,22 @@ type MetricData struct { } func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { - if s.requestBytesCounter == nil || s.responseBytesCounter == nil || s.serverLatencyMeasure == nil { - // This will happen if an HTTPServer{} is used instead of NewHTTPServer. - return + if s.requestBytesCounter != nil && s.responseBytesCounter != nil && s.serverLatencyMeasure != nil { + attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) + o := metric.WithAttributeSet(attribute.NewSet(attributes...)) + addOpts := []metric.AddOption{o} + s.requestBytesCounter.Add(ctx, md.RequestSize, addOpts...) + s.responseBytesCounter.Add(ctx, md.ResponseSize, addOpts...) + s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) } - attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) - o := metric.WithAttributeSet(attribute.NewSet(attributes...)) - addOpts := []metric.AddOption{o} - s.requestBytesCounter.Add(ctx, md.RequestSize, addOpts...) - s.responseBytesCounter.Add(ctx, md.ResponseSize, addOpts...) - s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) - - // TODO: Duplicate Metrics + if s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { + attributes := newHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) + o := metric.WithAttributeSet(attribute.NewSet(attributes...)) + s.requestDurationHistogram.Record(ctx, md.ElapsedTime, o) + s.requestBodySizeHistogram.Record(ctx, md.RequestSize, o) + s.responseBodySizeHistogram.Record(ctx, md.ResponseSize, o) + } } func NewHTTPServer(meter metric.Meter) HTTPServer { @@ -125,6 +133,9 @@ func NewHTTPServer(meter metric.Meter) HTTPServer { duplicate: duplicate, } server.requestBytesCounter, server.responseBytesCounter, server.serverLatencyMeasure = oldHTTPServer{}.createMeasures(meter) + if duplicate { + server.requestDurationHistogram, server.requestBodySizeHistogram, server.responseBodySizeHistogram = newHTTPServer{}.createMeasures(meter) + } return server } @@ -206,7 +217,7 @@ func (c HTTPClient) MetricOptions(ma MetricAttributes) MetricOpts { func (s HTTPClient) RecordMetrics(ctx context.Context, md MetricData, opts MetricOpts) { if s.requestBytesCounter == nil || s.latencyMeasure == nil { - // This will happen if an HTTPClient{} is used instead of NewHTTPClient(). + // This will happen if an HTTPClient{} is used insted of NewHTTPClient(). return } @@ -218,7 +229,7 @@ func (s HTTPClient) RecordMetrics(ctx context.Context, md MetricData, opts Metri func (s HTTPClient) RecordResponseSize(ctx context.Context, responseData int64, opts metric.AddOption) { if s.responseBytesCounter == nil { - // This will happen if an HTTPClient{} is used instead of NewHTTPClient(). + // This will happen if an HTTPClient{} is used insted of NewHTTPClient(). return } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go index 745b8c67bc4..cb729b96c0a 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go @@ -7,10 +7,13 @@ import ( "fmt" "net/http" "reflect" + "slices" "strconv" "strings" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/noop" semconvNew "go.opentelemetry.io/otel/semconv/v1.26.0" ) @@ -199,6 +202,87 @@ func (n newHTTPServer) Route(route string) attribute.KeyValue { return semconvNew.HTTPRoute(route) } +func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Float64Histogram, metric.Int64Histogram, metric.Int64Histogram) { + if meter == nil { + return noop.Float64Histogram{}, noop.Int64Histogram{}, noop.Int64Histogram{} + } + + var err error + requestDurationHistogram, err := meter.Float64Histogram( + semconvNew.HTTPServerRequestDurationName, + metric.WithUnit(semconvNew.HTTPServerRequestDurationUnit), + metric.WithDescription(semconvNew.HTTPServerRequestDurationDescription), + ) + handleErr(err) + + requestBodySizeHistogram, err := meter.Int64Histogram( + semconvNew.HTTPServerRequestBodySizeName, + metric.WithUnit(semconvNew.HTTPServerRequestBodySizeUnit), + metric.WithDescription(semconvNew.HTTPServerRequestBodySizeDescription), + ) + handleErr(err) + + responseBodySizeHistogram, err := meter.Int64Histogram( + semconvNew.HTTPServerResponseBodySizeName, + metric.WithUnit(semconvNew.HTTPServerResponseBodySizeUnit), + metric.WithDescription(semconvNew.HTTPServerResponseBodySizeDescription), + ) + handleErr(err) + + return requestDurationHistogram, requestBodySizeHistogram, responseBodySizeHistogram +} + +func (n newHTTPServer) MetricAttributes(server string, req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue { + num := len(additionalAttributes) + 3 + var host string + var p int + if server == "" { + host, p = splitHostPort(req.Host) + } else { + // Prioritize the primary server name. + host, p = splitHostPort(server) + if p < 0 { + _, p = splitHostPort(req.Host) + } + } + hostPort := requiredHTTPPort(req.TLS != nil, p) + if hostPort > 0 { + num++ + } + protoName, protoVersion := netProtocol(req.Proto) + if protoName != "" { + num++ + } + if protoVersion != "" { + num++ + } + + if statusCode > 0 { + num++ + } + + attributes := slices.Grow(additionalAttributes, num) + attributes = append(attributes, + standardizeNewHTTPMethodMetric(req.Method), + n.scheme(req.TLS != nil), + semconvNew.ServerAddress(host)) + + if hostPort > 0 { + attributes = append(attributes, semconvNew.ServerPort(hostPort)) + } + if protoName != "" { + attributes = append(attributes, semconvNew.NetworkProtocolName(protoName)) + } + if protoVersion != "" { + attributes = append(attributes, semconvNew.NetworkProtocolVersion(protoVersion)) + } + + if statusCode > 0 { + attributes = append(attributes, semconvNew.HTTPResponseStatusCode(statusCode)) + } + return attributes +} + type newHTTPClient struct{} // RequestTraceAttrs returns trace attributes for an HTTP request made by a client. @@ -346,3 +430,13 @@ func (n newHTTPClient) method(method string) (attribute.KeyValue, attribute.KeyV func isErrorStatusCode(code int) bool { return code >= 400 || code < 100 } + +func standardizeNewHTTPMethodMetric(method string) attribute.KeyValue { + method = strings.ToUpper(method) + switch method { + case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace: + default: + method = "_OTHER" + } + return semconvNew.HTTPRequestMethodKey.String(method) +} From c6da5fa1bf4003bacabda398741725d8bcc98e07 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 08:59:48 +0800 Subject: [PATCH 02/18] fix typos --- instrumentation/net/http/otelhttp/internal/semconv/env.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 58c0e656628..3229edbfa48 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -117,7 +117,7 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) } - if s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { + if s.duplicate && s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { attributes := newHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) s.requestDurationHistogram.Record(ctx, md.ElapsedTime, o) @@ -217,7 +217,7 @@ func (c HTTPClient) MetricOptions(ma MetricAttributes) MetricOpts { func (s HTTPClient) RecordMetrics(ctx context.Context, md MetricData, opts MetricOpts) { if s.requestBytesCounter == nil || s.latencyMeasure == nil { - // This will happen if an HTTPClient{} is used insted of NewHTTPClient(). + // This will happen if an HTTPClient{} is used instead of NewHTTPClient(). return } @@ -229,7 +229,7 @@ func (s HTTPClient) RecordMetrics(ctx context.Context, md MetricData, opts Metri func (s HTTPClient) RecordResponseSize(ctx context.Context, responseData int64, opts metric.AddOption) { if s.responseBytesCounter == nil { - // This will happen if an HTTPClient{} is used insted of NewHTTPClient(). + // This will happen if an HTTPClient{} is used instead of NewHTTPClient(). return } From f62983d36b3d672d17fdeb498824d3259c858f34 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 19:51:50 +0800 Subject: [PATCH 03/18] backup --- .../net/http/otelhttp/internal/semconv/env.go | 14 ++++-- .../otelhttp/internal/semconv/env_test.go | 36 ++++++++----- .../otelhttp/internal/semconv/httpconv.go | 19 ++++--- .../internal/semconv/httpconv_test.go | 50 +++++++++++++++++-- .../otelhttp/internal/semconv/v1.20.0_test.go | 26 +++++----- 5 files changed, 101 insertions(+), 44 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 3229edbfa48..469821e2620 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -15,6 +15,10 @@ import ( "go.opentelemetry.io/otel/metric" ) +// OTelSemConvStabilityOptIn is an environment variable. +// That can be set to "old" or "http/dup" to opt into the new HTTP semantic conventions. +const OTelSemConvStabilityOptIn = "OTEL_SEMCONV_STABILITY_OPT_IN" + type ResponseTelemetry struct { StatusCode int ReadBytes int64 @@ -32,9 +36,9 @@ type HTTPServer struct { serverLatencyMeasure metric.Float64Histogram // New metrics - requestDurationHistogram metric.Float64Histogram requestBodySizeHistogram metric.Int64Histogram responseBodySizeHistogram metric.Int64Histogram + requestDurationHistogram metric.Float64Histogram } // RequestTraceAttrs returns trace attributes for an HTTP request received by a @@ -120,21 +124,21 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { if s.duplicate && s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { attributes := newHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) - s.requestDurationHistogram.Record(ctx, md.ElapsedTime, o) s.requestBodySizeHistogram.Record(ctx, md.RequestSize, o) s.responseBodySizeHistogram.Record(ctx, md.ResponseSize, o) + s.requestDurationHistogram.Record(ctx, md.ElapsedTime, o) } } func NewHTTPServer(meter metric.Meter) HTTPServer { - env := strings.ToLower(os.Getenv("OTEL_SEMCONV_STABILITY_OPT_IN")) + env := strings.ToLower(os.Getenv(OTelSemConvStabilityOptIn)) duplicate := env == "http/dup" server := HTTPServer{ duplicate: duplicate, } server.requestBytesCounter, server.responseBytesCounter, server.serverLatencyMeasure = oldHTTPServer{}.createMeasures(meter) if duplicate { - server.requestDurationHistogram, server.requestBodySizeHistogram, server.responseBodySizeHistogram = newHTTPServer{}.createMeasures(meter) + server.requestBodySizeHistogram, server.responseBodySizeHistogram, server.requestDurationHistogram = newHTTPServer{}.createMeasures(meter) } return server } @@ -149,7 +153,7 @@ type HTTPClient struct { } func NewHTTPClient(meter metric.Meter) HTTPClient { - env := strings.ToLower(os.Getenv("OTEL_SEMCONV_STABILITY_OPT_IN")) + env := strings.ToLower(os.Getenv(OTelSemConvStabilityOptIn)) client := HTTPClient{ duplicate: env == "http/dup", } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 3a02a777373..9fda8fe80e4 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -94,24 +94,31 @@ func TestHTTPClientDoesNotPanic(t *testing.T) { } } -type testInst struct { +type testRecorder[T any] struct { embedded.Int64Counter + embedded.Int64Histogram embedded.Float64Histogram - intValue int64 - floatValue float64 + value T attributes []attribute.KeyValue } -func (t *testInst) Add(ctx context.Context, incr int64, options ...metric.AddOption) { - t.intValue = incr +var ( + _ metric.Int64Counter = (*testRecorder[int64])(nil) + _ metric.Float64Histogram = (*testRecorder[float64])(nil) + _ metric.Int64Histogram = (*testRecorder[int64])(nil) + _ metric.Float64Histogram = (*testRecorder[float64])(nil) +) + +func (t *testRecorder[T]) Add(_ context.Context, incr T, options ...metric.AddOption) { + t.value = incr cfg := metric.NewAddConfig(options) attr := cfg.Attributes() t.attributes = attr.ToSlice() } -func (t *testInst) Record(ctx context.Context, value float64, options ...metric.RecordOption) { - t.floatValue = value +func (t *testRecorder[T]) Record(_ context.Context, value T, options ...metric.RecordOption) { + t.value = value cfg := metric.NewRecordConfig(options) attr := cfg.Attributes() t.attributes = attr.ToSlice() @@ -119,16 +126,19 @@ func (t *testInst) Record(ctx context.Context, value float64, options ...metric. func NewTestHTTPServer() HTTPServer { return HTTPServer{ - requestBytesCounter: &testInst{}, - responseBytesCounter: &testInst{}, - serverLatencyMeasure: &testInst{}, + requestBytesCounter: &testRecorder[int64]{}, + responseBytesCounter: &testRecorder[int64]{}, + serverLatencyMeasure: &testRecorder[float64]{}, + requestBodySizeHistogram: &testRecorder[int64]{}, + responseBodySizeHistogram: &testRecorder[int64]{}, + requestDurationHistogram: &testRecorder[float64]{}, } } func NewTestHTTPClient() HTTPClient { return HTTPClient{ - requestBytesCounter: &testInst{}, - responseBytesCounter: &testInst{}, - latencyMeasure: &testInst{}, + requestBytesCounter: &testRecorder[int64]{}, + responseBytesCounter: &testRecorder[int64]{}, + latencyMeasure: &testRecorder[float64]{}, } } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go index cb729b96c0a..3f2417b52b1 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go @@ -202,19 +202,12 @@ func (n newHTTPServer) Route(route string) attribute.KeyValue { return semconvNew.HTTPRoute(route) } -func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Float64Histogram, metric.Int64Histogram, metric.Int64Histogram) { +func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Int64Histogram, metric.Int64Histogram, metric.Float64Histogram) { if meter == nil { - return noop.Float64Histogram{}, noop.Int64Histogram{}, noop.Int64Histogram{} + return noop.Int64Histogram{}, noop.Int64Histogram{}, noop.Float64Histogram{} } var err error - requestDurationHistogram, err := meter.Float64Histogram( - semconvNew.HTTPServerRequestDurationName, - metric.WithUnit(semconvNew.HTTPServerRequestDurationUnit), - metric.WithDescription(semconvNew.HTTPServerRequestDurationDescription), - ) - handleErr(err) - requestBodySizeHistogram, err := meter.Int64Histogram( semconvNew.HTTPServerRequestBodySizeName, metric.WithUnit(semconvNew.HTTPServerRequestBodySizeUnit), @@ -228,8 +221,14 @@ func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Float64Histogr metric.WithDescription(semconvNew.HTTPServerResponseBodySizeDescription), ) handleErr(err) + requestDurationHistogram, err := meter.Float64Histogram( + semconvNew.HTTPServerRequestDurationName, + metric.WithUnit(semconvNew.HTTPServerRequestDurationUnit), + metric.WithDescription(semconvNew.HTTPServerRequestDurationDescription), + ) + handleErr(err) - return requestDurationHistogram, requestBodySizeHistogram, responseBodySizeHistogram + return requestBodySizeHistogram, responseBodySizeHistogram, requestDurationHistogram } func (n newHTTPServer) MetricAttributes(server string, req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue { diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 6a3f6c09a4f..5394251cbc4 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -4,6 +4,7 @@ package semconv import ( + "context" "errors" "fmt" "net/http" @@ -17,7 +18,7 @@ import ( ) func TestNewTraceRequest(t *testing.T) { - t.Setenv("OTEL_SEMCONV_STABILITY_OPT_IN", "http/dup") + t.Setenv(OTelSemConvStabilityOptIn, "http/dup") serv := NewHTTPServer(nil) want := func(req testServerReq) []attribute.KeyValue { return []attribute.KeyValue{ @@ -95,6 +96,49 @@ func TestNewTraceResponse(t *testing.T) { } } +func TestNewRecordMetrics(t *testing.T) { + t.Setenv(OTelSemConvStabilityOptIn, "http/dup") + server := NewTestHTTPServer() + server.duplicate = true + req, err := http.NewRequest("POST", "http://example.com", nil) + assert.NoError(t, err) + + server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", + ResponseSize: 200, + MetricAttributes: MetricAttributes{ + Req: req, + StatusCode: 301, + AdditionalAttributes: []attribute.KeyValue{ + attribute.String("key", "value"), + }, + }, + MetricData: MetricData{ + RequestSize: 100, + ElapsedTime: 300, + }, + }) + + assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) + + want := []attribute.KeyValue{ + attribute.String("http.scheme", "http"), + attribute.String("http.method", "POST"), + attribute.Int64("http.status_code", 301), + attribute.String("key", "value"), + attribute.String("net.host.name", "stuff"), + attribute.String("net.protocol.name", "http"), + attribute.String("net.protocol.version", "1.1"), + } + _ = want + + // assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) + // assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) + // assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) +} + func TestNewMethod(t *testing.T) { testCases := []struct { method string @@ -131,7 +175,7 @@ func TestNewMethod(t *testing.T) { } func TestNewTraceRequest_Client(t *testing.T) { - t.Setenv("OTEL_SEMCONV_STABILITY_OPT_IN", "http/dup") + t.Setenv(OTelSemConvStabilityOptIn, "http/dup") body := strings.NewReader("Hello, world!") url := "https://example.com:8888/foo/bar?stuff=morestuff" req := httptest.NewRequest("pOST", url, body) @@ -156,7 +200,7 @@ func TestNewTraceRequest_Client(t *testing.T) { } func TestNewTraceResponse_Client(t *testing.T) { - t.Setenv("OTEL_SEMCONV_STABILITY_OPT_IN", "http/dup") + t.Setenv(OTelSemConvStabilityOptIn, "http/dup") testcases := []struct { resp http.Response want []attribute.KeyValue diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go index eef382a5047..ef5a4568965 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go @@ -17,7 +17,7 @@ import ( func TestV120TraceRequest(t *testing.T) { // Anything but "http" or "http/dup" works. - t.Setenv("OTEL_SEMCONV_STABILITY_OPT_IN", "old") + t.Setenv(OTelSemConvStabilityOptIn, "old") serv := NewHTTPServer(nil) want := func(req testServerReq) []attribute.KeyValue { return []attribute.KeyValue{ @@ -108,9 +108,9 @@ func TestV120RecordMetrics(t *testing.T) { }, }) - assert.Equal(t, int64(100), server.requestBytesCounter.(*testInst).intValue) - assert.Equal(t, int64(200), server.responseBytesCounter.(*testInst).intValue) - assert.Equal(t, float64(300), server.serverLatencyMeasure.(*testInst).floatValue) + assert.Equal(t, int64(100), server.requestBytesCounter.(*testRecorder[int64]).value) + assert.Equal(t, int64(200), server.responseBytesCounter.(*testRecorder[int64]).value) + assert.Equal(t, float64(300), server.serverLatencyMeasure.(*testRecorder[float64]).value) want := []attribute.KeyValue{ attribute.String("http.scheme", "http"), @@ -122,9 +122,9 @@ func TestV120RecordMetrics(t *testing.T) { attribute.String("net.protocol.version", "1.1"), } - assert.ElementsMatch(t, want, server.requestBytesCounter.(*testInst).attributes) - assert.ElementsMatch(t, want, server.responseBytesCounter.(*testInst).attributes) - assert.ElementsMatch(t, want, server.serverLatencyMeasure.(*testInst).attributes) + assert.ElementsMatch(t, want, server.requestBytesCounter.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, server.responseBytesCounter.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, server.serverLatencyMeasure.(*testRecorder[float64]).attributes) } func TestV120ClientRequest(t *testing.T) { @@ -183,9 +183,9 @@ func TestV120ClientMetrics(t *testing.T) { ElapsedTime: 300, }, opts) - assert.Equal(t, int64(100), client.requestBytesCounter.(*testInst).intValue) - assert.Equal(t, int64(200), client.responseBytesCounter.(*testInst).intValue) - assert.Equal(t, float64(300), client.latencyMeasure.(*testInst).floatValue) + assert.Equal(t, int64(100), client.requestBytesCounter.(*testRecorder[int64]).value) + assert.Equal(t, int64(200), client.responseBytesCounter.(*testRecorder[int64]).value) + assert.Equal(t, float64(300), client.latencyMeasure.(*testRecorder[float64]).value) want := []attribute.KeyValue{ attribute.String("http.method", "POST"), @@ -194,9 +194,9 @@ func TestV120ClientMetrics(t *testing.T) { attribute.String("net.peer.name", "example.com"), } - assert.ElementsMatch(t, want, client.requestBytesCounter.(*testInst).attributes) - assert.ElementsMatch(t, want, client.responseBytesCounter.(*testInst).attributes) - assert.ElementsMatch(t, want, client.latencyMeasure.(*testInst).attributes) + assert.ElementsMatch(t, want, client.requestBytesCounter.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, client.responseBytesCounter.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, client.latencyMeasure.(*testRecorder[float64]).attributes) } func TestStandardizeHTTPMethodMetric(t *testing.T) { From 35c04606ef87ff24f0de4b4c42b1d8c6c7e2f55d Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 22:35:22 +0800 Subject: [PATCH 04/18] feat(otelhttp): generate New Server Metrics in `otelhttp` --- .../otelhttp/internal/semconv/env_test.go | 16 +-- .../internal/semconv/httpconv_test.go | 98 ++++++++++++------- 2 files changed, 73 insertions(+), 41 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 9fda8fe80e4..874966b3e15 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -125,14 +125,14 @@ func (t *testRecorder[T]) Record(_ context.Context, value T, options ...metric.R } func NewTestHTTPServer() HTTPServer { - return HTTPServer{ - requestBytesCounter: &testRecorder[int64]{}, - responseBytesCounter: &testRecorder[int64]{}, - serverLatencyMeasure: &testRecorder[float64]{}, - requestBodySizeHistogram: &testRecorder[int64]{}, - responseBodySizeHistogram: &testRecorder[int64]{}, - requestDurationHistogram: &testRecorder[float64]{}, - } + httpServer := NewHTTPServer(nil) + httpServer.requestBytesCounter = &testRecorder[int64]{} + httpServer.responseBytesCounter = &testRecorder[int64]{} + httpServer.serverLatencyMeasure = &testRecorder[float64]{} + httpServer.requestBodySizeHistogram = &testRecorder[int64]{} + httpServer.responseBodySizeHistogram = &testRecorder[int64]{} + httpServer.requestDurationHistogram = &testRecorder[float64]{} + return httpServer } func NewTestHTTPClient() HTTPClient { diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 5394251cbc4..a9da5584827 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -97,46 +97,78 @@ func TestNewTraceResponse(t *testing.T) { } func TestNewRecordMetrics(t *testing.T) { - t.Setenv(OTelSemConvStabilityOptIn, "http/dup") - server := NewTestHTTPServer() - server.duplicate = true - req, err := http.NewRequest("POST", "http://example.com", nil) - assert.NoError(t, err) + tests := []struct { + name string + setEnv bool + expectedFunc func(server HTTPServer, t *testing.T) + }{ + { + name: "set env", + setEnv: true, + expectedFunc: func(server HTTPServer, t *testing.T) { + assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) + + want := []attribute.KeyValue{ + attribute.String("url.scheme", "http"), + attribute.String("http.request.method", "POST"), + attribute.Int64("http.response.status_code", 301), + attribute.String("key", "value"), + attribute.String("server.address", "stuff"), + attribute.String("network.protocol.name", "http"), + attribute.String("network.protocol.version", "1.1"), + } - server.RecordMetrics(context.Background(), ServerMetricData{ - ServerName: "stuff", - ResponseSize: 200, - MetricAttributes: MetricAttributes{ - Req: req, - StatusCode: 301, - AdditionalAttributes: []attribute.KeyValue{ - attribute.String("key", "value"), + assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) + assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) }, }, - MetricData: MetricData{ - RequestSize: 100, - ElapsedTime: 300, + { + name: "not set env", + setEnv: false, + expectedFunc: func(server HTTPServer, t *testing.T) { + assert.Equal(t, int64(0), server.requestBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, int64(0), server.responseBodySizeHistogram.(*testRecorder[int64]).value) + assert.Equal(t, float64(0), server.requestDurationHistogram.(*testRecorder[float64]).value) + + assert.Len(t, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes, 0) + assert.Len(t, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes, 0) + assert.Len(t, server.requestDurationHistogram.(*testRecorder[float64]).attributes, 0) + }, }, - }) + } - assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setEnv { + t.Setenv(OTelSemConvStabilityOptIn, "http/dup") + } - want := []attribute.KeyValue{ - attribute.String("http.scheme", "http"), - attribute.String("http.method", "POST"), - attribute.Int64("http.status_code", 301), - attribute.String("key", "value"), - attribute.String("net.host.name", "stuff"), - attribute.String("net.protocol.name", "http"), - attribute.String("net.protocol.version", "1.1"), - } - _ = want + server := NewTestHTTPServer() + req, err := http.NewRequest("POST", "http://example.com", nil) + assert.NoError(t, err) - // assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) - // assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) - // assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) + server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", + ResponseSize: 200, + MetricAttributes: MetricAttributes{ + Req: req, + StatusCode: 301, + AdditionalAttributes: []attribute.KeyValue{ + attribute.String("key", "value"), + }, + }, + MetricData: MetricData{ + RequestSize: 100, + ElapsedTime: 300, + }, + }) + + tt.expectedFunc(server, t) + }) + } } func TestNewMethod(t *testing.T) { From c2c68641ea6ef9bc44015b3f2c8847eab793bd39 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 22:38:09 +0800 Subject: [PATCH 05/18] docs(CHANGELOG): update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61f0af8f6b..a26d08f77cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added support exporting logs via OTLP over gRPC in `go.opentelemetry.io/contrib/config`. (#6340) - The `go.opentelemetry.io/contrib/bridges/otellogr` module. This module provides an OpenTelemetry logging bridge for `github.com/go-logr/logr`. (#6386) +- Generate New Server Metrics when set env `OTEL_SEMCONV_STABILITY_OPT_IN=http/dup` in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) + ### Changed From cb5f43b5fed83c6571b220d3ced254ba4ab094b8 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 22:39:06 +0800 Subject: [PATCH 06/18] docs(CHANGELOG): update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a26d08f77cf..62709db3909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added support exporting logs via OTLP over gRPC in `go.opentelemetry.io/contrib/config`. (#6340) - The `go.opentelemetry.io/contrib/bridges/otellogr` module. This module provides an OpenTelemetry logging bridge for `github.com/go-logr/logr`. (#6386) -- Generate New Server Metrics when set env `OTEL_SEMCONV_STABILITY_OPT_IN=http/dup` in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) +- Generate new server metrics upon setting the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN=http/dup` within `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) ### Changed From 39b2c894a460b34c11bdc68decf98bea756c3d7c Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 9 Dec 2024 22:46:12 +0800 Subject: [PATCH 07/18] fix lint --- .../net/http/otelhttp/internal/semconv/httpconv_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index a9da5584827..908755f8cd1 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -133,9 +133,9 @@ func TestNewRecordMetrics(t *testing.T) { assert.Equal(t, int64(0), server.responseBodySizeHistogram.(*testRecorder[int64]).value) assert.Equal(t, float64(0), server.requestDurationHistogram.(*testRecorder[float64]).value) - assert.Len(t, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes, 0) - assert.Len(t, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes, 0) - assert.Len(t, server.requestDurationHistogram.(*testRecorder[float64]).attributes, 0) + assert.Empty(t, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) + assert.Empty(t, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) + assert.Empty(t, server.requestDurationHistogram.(*testRecorder[float64]).attributes) }, }, } From 3a7583798c5d042f22af237bfe153ef37c81c48b Mon Sep 17 00:00:00 2001 From: Flc Date: Thu, 12 Dec 2024 22:33:54 +0800 Subject: [PATCH 08/18] resolve(conflict): resolve merge conflicts --- .../net/http/otelhttp/internal/semconv/env.go | 4 ++-- .../http/otelhttp/internal/semconv/env_test.go | 3 +++ .../http/otelhttp/internal/semconv/httpconv.go | 18 ++++++++++-------- .../otelhttp/internal/semconv/httpconv_test.go | 2 -- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 00edf2639c4..aa835607d46 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -122,7 +122,7 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { } if s.duplicate && s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { - attributes := newHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) + attributes := CurrentHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) s.requestBodySizeHistogram.Record(ctx, md.RequestSize, o) s.responseBodySizeHistogram.Record(ctx, md.ResponseSize, o) @@ -138,7 +138,7 @@ func NewHTTPServer(meter metric.Meter) HTTPServer { } server.requestBytesCounter, server.responseBytesCounter, server.serverLatencyMeasure = OldHTTPServer{}.createMeasures(meter) if duplicate { - server.requestBodySizeHistogram, server.responseBodySizeHistogram, server.requestDurationHistogram = newHTTPServer{}.createMeasures(meter) + server.requestBodySizeHistogram, server.responseBodySizeHistogram, server.requestDurationHistogram = CurrentHTTPServer{}.createMeasures(meter) } return server } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index a80947cdd8c..01b17080100 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -9,6 +9,9 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/embedded" "go.opentelemetry.io/otel/metric/noop" ) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go index 08199bf09fc..10b597bff1b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go @@ -7,14 +7,17 @@ import ( "fmt" "net/http" "reflect" + "slices" "strconv" "strings" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/noop" semconvNew "go.opentelemetry.io/otel/semconv/v1.26.0" ) -type newHTTPServer struct{} +type CurrentHTTPServer struct{} // TraceRequest returns trace attributes for an HTTP request received by a // server. @@ -199,8 +202,7 @@ func (n CurrentHTTPServer) Route(route string) attribute.KeyValue { return semconvNew.HTTPRoute(route) } -type CurrentHTTPClient struct{} -func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Int64Histogram, metric.Int64Histogram, metric.Float64Histogram) { +func (n CurrentHTTPServer) createMeasures(meter metric.Meter) (metric.Int64Histogram, metric.Int64Histogram, metric.Float64Histogram) { if meter == nil { return noop.Int64Histogram{}, noop.Int64Histogram{}, noop.Float64Histogram{} } @@ -229,17 +231,17 @@ func (n newHTTPServer) createMeasures(meter metric.Meter) (metric.Int64Histogram return requestBodySizeHistogram, responseBodySizeHistogram, requestDurationHistogram } -func (n newHTTPServer) MetricAttributes(server string, req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue { +func (n CurrentHTTPServer) MetricAttributes(server string, req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue { num := len(additionalAttributes) + 3 var host string var p int if server == "" { - host, p = splitHostPort(req.Host) + host, p = SplitHostPort(req.Host) } else { // Prioritize the primary server name. - host, p = splitHostPort(server) + host, p = SplitHostPort(server) if p < 0 { - _, p = splitHostPort(req.Host) + _, p = SplitHostPort(req.Host) } } hostPort := requiredHTTPPort(req.TLS != nil, p) @@ -280,7 +282,7 @@ func (n newHTTPServer) MetricAttributes(server string, req *http.Request, status return attributes } -type newHTTPClient struct{} +type CurrentHTTPClient struct{} // RequestTraceAttrs returns trace attributes for an HTTP request made by a client. func (n CurrentHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue { diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 794b0e75bf8..56bf5b2dc7d 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -5,8 +5,6 @@ package semconv import ( "context" - "errors" - "fmt" "net/http" "strconv" "testing" From 16e399e19294d7362175aa3afd74b4290ffaef6e Mon Sep 17 00:00:00 2001 From: Flc Date: Thu, 12 Dec 2024 22:44:42 +0800 Subject: [PATCH 09/18] resolve(conflict): resolve merge conflicts --- .../net/http/otelhttp/internal/semconv/env_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 01b17080100..9eddb65b720 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -134,11 +134,3 @@ func NewTestHTTPServer() HTTPServer { httpServer.requestDurationHistogram = &testRecorder[float64]{} return httpServer } - -func NewTestHTTPClient() HTTPClient { - return HTTPClient{ - requestBytesCounter: &testRecorder[int64]{}, - responseBytesCounter: &testRecorder[int64]{}, - latencyMeasure: &testRecorder[float64]{}, - } -} From 016b2578f645b168aa6612ec4e43e433cca50e02 Mon Sep 17 00:00:00 2001 From: Flc Date: Thu, 12 Dec 2024 22:46:44 +0800 Subject: [PATCH 10/18] resolve(conflict): resolve merge conflicts --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fda6d36f799..9822a95aff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added SNS instrumentation in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#6388) - Generate new server metrics upon setting the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN=http/dup` within `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) - ### Changed - Change the span name to be `GET /path` so it complies with the OTel HTTP semantic conventions in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) From 64588cef0b1e5a9c917153365fcd9a495f172f8f Mon Sep 17 00:00:00 2001 From: Flc Date: Thu, 12 Dec 2024 23:50:16 +0800 Subject: [PATCH 11/18] make a temporary backup --- .../internal/semconv/httpconv_test.go | 76 ------------ .../internal/semconv/test/httpconv_test.go | 110 +++++++++++++++++- 2 files changed, 109 insertions(+), 77 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 56bf5b2dc7d..47b4db548ce 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -4,7 +4,6 @@ package semconv import ( - "context" "net/http" "strconv" "testing" @@ -15,81 +14,6 @@ import ( "go.opentelemetry.io/otel/codes" ) -func TestNewRecordMetrics(t *testing.T) { - tests := []struct { - name string - setEnv bool - expectedFunc func(server HTTPServer, t *testing.T) - }{ - { - name: "set env", - setEnv: true, - expectedFunc: func(server HTTPServer, t *testing.T) { - assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) - - want := []attribute.KeyValue{ - attribute.String("url.scheme", "http"), - attribute.String("http.request.method", "POST"), - attribute.Int64("http.response.status_code", 301), - attribute.String("key", "value"), - attribute.String("server.address", "stuff"), - attribute.String("network.protocol.name", "http"), - attribute.String("network.protocol.version", "1.1"), - } - - assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) - assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) - assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) - }, - }, - { - name: "not set env", - setEnv: false, - expectedFunc: func(server HTTPServer, t *testing.T) { - assert.Equal(t, int64(0), server.requestBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, int64(0), server.responseBodySizeHistogram.(*testRecorder[int64]).value) - assert.Equal(t, float64(0), server.requestDurationHistogram.(*testRecorder[float64]).value) - - assert.Empty(t, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) - assert.Empty(t, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) - assert.Empty(t, server.requestDurationHistogram.(*testRecorder[float64]).attributes) - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.setEnv { - t.Setenv(OTelSemConvStabilityOptIn, "http/dup") - } - - server := NewTestHTTPServer() - req, err := http.NewRequest("POST", "http://example.com", nil) - assert.NoError(t, err) - - server.RecordMetrics(context.Background(), ServerMetricData{ - ServerName: "stuff", - ResponseSize: 200, - MetricAttributes: MetricAttributes{ - Req: req, - StatusCode: 301, - AdditionalAttributes: []attribute.KeyValue{ - attribute.String("key", "value"), - }, - }, - MetricData: MetricData{ - RequestSize: 100, - ElapsedTime: 300, - }, - }) - - tt.expectedFunc(server, t) - }) - } -} - func TestNewMethod(t *testing.T) { testCases := []struct { method string diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go index da37f9765b3..04082571901 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go @@ -4,6 +4,7 @@ package test import ( + "context" "errors" "fmt" "net/http" @@ -11,10 +12,13 @@ import ( "strings" "testing" + "github.com/davecgh/go-spew/spew" "github.com/stretchr/testify/assert" - + "github.com/stretchr/testify/require" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" "go.opentelemetry.io/otel/attribute" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" ) func TestNewTraceRequest(t *testing.T) { @@ -47,6 +51,110 @@ func TestNewTraceRequest(t *testing.T) { testTraceRequest(t, serv, want) } +func TestNewRecordMetrics(t *testing.T) { + reader := sdkmetric.NewManualReader() + mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + + tests := []struct { + name string + setEnv bool + serverFunc func() semconv.HTTPServer + expectedFunc func(t *testing.T, rm metricdata.ResourceMetrics) + }{ + { + name: "not set env and the nil meter", + setEnv: false, + serverFunc: func() semconv.HTTPServer { + return semconv.NewHTTPServer(nil) + }, + expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + assert.Empty(t, rm.ScopeMetrics) + }, + }, + { + name: "not set env and the meter", + setEnv: false, + serverFunc: func() semconv.HTTPServer { + return semconv.NewHTTPServer(mp.Meter("test")) + }, + expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + assert.Len(t, rm.ScopeMetrics, 1) + }, + }, + { + name: "set env and the nil meter", + setEnv: true, + serverFunc: func() semconv.HTTPServer { + return semconv.NewHTTPServer(nil) + }, + expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + assert.Empty(t, rm.ScopeMetrics) + }, + }, + { + name: "set env and the nil meter", + setEnv: true, + serverFunc: func() semconv.HTTPServer { + return semconv.NewHTTPServer(mp.Meter("test")) + }, + expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + // assert.Len(t, rm.ScopeMetrics, 1) + // assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) + // assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) + // assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) + // + // want := []attribute.KeyValue{ + // attribute.String("url.scheme", "http"), + // attribute.String("http.request.method", "POST"), + // attribute.Int64("http.response.status_code", 301), + // attribute.String("key", "value"), + // attribute.String("server.address", "stuff"), + // attribute.String("network.protocol.name", "http"), + // attribute.String("network.protocol.version", "1.1"), + // } + // + // assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) + // assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) + // assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setEnv { + t.Setenv(semconv.OTelSemConvStabilityOptIn, "http/dup") + } + + server := tt.serverFunc() + req, err := http.NewRequest("POST", "http://example.com", nil) + assert.NoError(t, err) + + server.RecordMetrics(context.Background(), semconv.ServerMetricData{ + ServerName: "stuff", + ResponseSize: 200, + MetricAttributes: semconv.MetricAttributes{ + Req: req, + StatusCode: 301, + AdditionalAttributes: []attribute.KeyValue{ + attribute.String("key", "value"), + }, + }, + MetricData: semconv.MetricData{ + RequestSize: 100, + ElapsedTime: 300, + }, + }) + + rm := metricdata.ResourceMetrics{} + require.NoError(t, reader.Collect(context.Background(), &rm)) + spew.Dump(rm) + + tt.expectedFunc(t, rm) + }) + } +} + func TestNewTraceResponse(t *testing.T) { testCases := []struct { name string From 31f87e86e3ff98ab9f715b2c02642b3f37a3503f Mon Sep 17 00:00:00 2001 From: Flc Date: Fri, 13 Dec 2024 15:46:46 +0800 Subject: [PATCH 12/18] test(otelhttp): optimize tests --- .../otelhttp/internal/semconv/env_test.go | 45 ----- .../otelhttp/internal/semconv/test/go.mod | 2 +- .../internal/semconv/test/httpconv_test.go | 186 ++++++++++++++---- 3 files changed, 146 insertions(+), 87 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 9eddb65b720..ac60178f8a5 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -9,10 +9,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/metric" - "go.opentelemetry.io/otel/metric/embedded" - "go.opentelemetry.io/otel/metric/noop" ) @@ -93,44 +89,3 @@ func TestHTTPClientDoesNotPanic(t *testing.T) { }) } } - -type testRecorder[T any] struct { - embedded.Int64Counter - embedded.Int64Histogram - embedded.Float64Histogram - - value T - attributes []attribute.KeyValue -} - -var ( - _ metric.Int64Counter = (*testRecorder[int64])(nil) - _ metric.Float64Histogram = (*testRecorder[float64])(nil) - _ metric.Int64Histogram = (*testRecorder[int64])(nil) - _ metric.Float64Histogram = (*testRecorder[float64])(nil) -) - -func (t *testRecorder[T]) Add(_ context.Context, incr T, options ...metric.AddOption) { - t.value = incr - cfg := metric.NewAddConfig(options) - attr := cfg.Attributes() - t.attributes = attr.ToSlice() -} - -func (t *testRecorder[T]) Record(_ context.Context, value T, options ...metric.RecordOption) { - t.value = value - cfg := metric.NewRecordConfig(options) - attr := cfg.Attributes() - t.attributes = attr.ToSlice() -} - -func NewTestHTTPServer() HTTPServer { - httpServer := NewHTTPServer(nil) - httpServer.requestBytesCounter = &testRecorder[int64]{} - httpServer.responseBytesCounter = &testRecorder[int64]{} - httpServer.serverLatencyMeasure = &testRecorder[float64]{} - httpServer.requestBodySizeHistogram = &testRecorder[int64]{} - httpServer.responseBodySizeHistogram = &testRecorder[int64]{} - httpServer.requestDurationHistogram = &testRecorder[float64]{} - return httpServer -} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod b/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod index 99c176e1117..c77565c6f2d 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod @@ -6,6 +6,7 @@ require ( github.com/stretchr/testify v1.10.0 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.0.0-00010101000000-000000000000 go.opentelemetry.io/otel v1.32.0 + go.opentelemetry.io/otel/metric v1.32.0 go.opentelemetry.io/otel/sdk v1.32.0 go.opentelemetry.io/otel/sdk/metric v1.32.0 ) @@ -16,7 +17,6 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/google/uuid v1.6.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - go.opentelemetry.io/otel/metric v1.32.0 // indirect go.opentelemetry.io/otel/trace v1.32.0 // indirect golang.org/x/sys v0.28.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go index 04082571901..137c1717696 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go @@ -12,13 +12,15 @@ import ( "strings" "testing" - "github.com/davecgh/go-spew/spew" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" sdkmetric "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/metric/metricdata" + "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" ) func TestNewTraceRequest(t *testing.T) { @@ -52,70 +54,171 @@ func TestNewTraceRequest(t *testing.T) { } func TestNewRecordMetrics(t *testing.T) { - reader := sdkmetric.NewManualReader() - mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + oldAttrs := attribute.NewSet( + attribute.String("http.scheme", "http"), + attribute.String("http.method", "POST"), + attribute.Int64("http.status_code", 301), + attribute.String("key", "value"), + attribute.String("net.host.name", "stuff"), + attribute.String("net.protocol.name", "http"), + attribute.String("net.protocol.version", "1.1"), + ) + + currAttrs := attribute.NewSet( + attribute.String("http.request.method", "POST"), + attribute.Int64("http.response.status_code", 301), + attribute.String("key", "value"), + attribute.String("network.protocol.name", "http"), + attribute.String("network.protocol.version", "1.1"), + attribute.String("server.address", "stuff"), + attribute.String("url.scheme", "http"), + ) + + // The OldHTTPServer version + expectedOldScopeMetric := metricdata.ScopeMetrics{ + Scope: instrumentation.Scope{ + Name: "test", + }, + Metrics: []metricdata.Metrics{ + { + Name: "http.server.request.size", + Description: "Measures the size of HTTP request messages.", + Unit: "By", + Data: metricdata.Sum[int64]{ + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: true, + DataPoints: []metricdata.DataPoint[int64]{ + { + Attributes: oldAttrs, + }, + }, + }, + }, + { + Name: "http.server.response.size", + Description: "Measures the size of HTTP response messages.", + Unit: "By", + Data: metricdata.Sum[int64]{ + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: true, + DataPoints: []metricdata.DataPoint[int64]{ + { + Attributes: oldAttrs, + }, + }, + }, + }, + { + Name: "http.server.duration", + Description: "Measures the duration of inbound HTTP requests.", + Unit: "ms", + Data: metricdata.Histogram[float64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[float64]{ + { + Attributes: oldAttrs, + }, + }, + }, + }, + }, + } + + // the CurrentHTTPServer version + expectedCurrentScopeMetric := expectedOldScopeMetric + expectedCurrentScopeMetric.Metrics = append(expectedCurrentScopeMetric.Metrics, []metricdata.Metrics{ + { + + Name: "http.server.request.body.size", + Description: "Size of HTTP server request bodies.", + Unit: "By", + Data: metricdata.Histogram[int64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[int64]{ + { + Attributes: currAttrs, + }, + }, + }, + }, + { + Name: "http.server.response.body.size", + Description: "Size of HTTP server response bodies.", + Unit: "By", + Data: metricdata.Histogram[int64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[int64]{ + { + Attributes: currAttrs, + }, + }, + }, + }, + { + Name: "http.server.request.duration", + Description: "Duration of HTTP server requests.", + Unit: "s", + Data: metricdata.Histogram[float64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[float64]{ + { + Attributes: currAttrs, + }, + }, + }, + }, + }...) tests := []struct { - name string - setEnv bool - serverFunc func() semconv.HTTPServer - expectedFunc func(t *testing.T, rm metricdata.ResourceMetrics) + name string + setEnv bool + serverFunc func(metric.MeterProvider) semconv.HTTPServer + wantFunc func(t *testing.T, rm metricdata.ResourceMetrics) }{ { - name: "not set env and the nil meter", + name: "No environment variable set, and no Meter", setEnv: false, - serverFunc: func() semconv.HTTPServer { + serverFunc: func(metric.MeterProvider) semconv.HTTPServer { return semconv.NewHTTPServer(nil) }, - expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { assert.Empty(t, rm.ScopeMetrics) }, }, { - name: "not set env and the meter", + name: "No environment variable set, but with Meter", setEnv: false, - serverFunc: func() semconv.HTTPServer { + serverFunc: func(mp metric.MeterProvider) semconv.HTTPServer { return semconv.NewHTTPServer(mp.Meter("test")) }, - expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { assert.Len(t, rm.ScopeMetrics, 1) + + // because of OldHTTPServer + require.Len(t, rm.ScopeMetrics[0].Metrics, 3) + metricdatatest.AssertEqual(t, expectedOldScopeMetric, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) }, }, { - name: "set env and the nil meter", + name: "Set environment variable, but no Meter", setEnv: true, - serverFunc: func() semconv.HTTPServer { + serverFunc: func(metric.MeterProvider) semconv.HTTPServer { return semconv.NewHTTPServer(nil) }, - expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { assert.Empty(t, rm.ScopeMetrics) }, }, { - name: "set env and the nil meter", + name: "Set environment variable and Meter", setEnv: true, - serverFunc: func() semconv.HTTPServer { + serverFunc: func(mp metric.MeterProvider) semconv.HTTPServer { return semconv.NewHTTPServer(mp.Meter("test")) }, - expectedFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { - // assert.Len(t, rm.ScopeMetrics, 1) - // assert.Equal(t, int64(100), server.requestBodySizeHistogram.(*testRecorder[int64]).value) - // assert.Equal(t, int64(200), server.responseBodySizeHistogram.(*testRecorder[int64]).value) - // assert.Equal(t, float64(300), server.requestDurationHistogram.(*testRecorder[float64]).value) - // - // want := []attribute.KeyValue{ - // attribute.String("url.scheme", "http"), - // attribute.String("http.request.method", "POST"), - // attribute.Int64("http.response.status_code", 301), - // attribute.String("key", "value"), - // attribute.String("server.address", "stuff"), - // attribute.String("network.protocol.name", "http"), - // attribute.String("network.protocol.version", "1.1"), - // } - // - // assert.ElementsMatch(t, want, server.requestBodySizeHistogram.(*testRecorder[int64]).attributes) - // assert.ElementsMatch(t, want, server.responseBodySizeHistogram.(*testRecorder[int64]).attributes) - // assert.ElementsMatch(t, want, server.requestDurationHistogram.(*testRecorder[float64]).attributes) + wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { + assert.Len(t, rm.ScopeMetrics, 1) + require.Len(t, rm.ScopeMetrics[0].Metrics, 6) + metricdatatest.AssertEqual(t, expectedCurrentScopeMetric, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) }, }, } @@ -126,7 +229,10 @@ func TestNewRecordMetrics(t *testing.T) { t.Setenv(semconv.OTelSemConvStabilityOptIn, "http/dup") } - server := tt.serverFunc() + reader := sdkmetric.NewManualReader() + mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + + server := tt.serverFunc(mp) req, err := http.NewRequest("POST", "http://example.com", nil) assert.NoError(t, err) @@ -148,9 +254,7 @@ func TestNewRecordMetrics(t *testing.T) { rm := metricdata.ResourceMetrics{} require.NoError(t, reader.Collect(context.Background(), &rm)) - spew.Dump(rm) - - tt.expectedFunc(t, rm) + tt.wantFunc(t, rm) }) } } From e1cf824d6687ecb67ee75c483b8273b4ca259e08 Mon Sep 17 00:00:00 2001 From: Flc Date: Fri, 13 Dec 2024 16:01:12 +0800 Subject: [PATCH 13/18] Fix lint and test --- .../net/http/otelhttp/internal/semconv/env.go | 27 +++++++++++++------ .../otelhttp/internal/semconv/env_test.go | 1 + .../otelhttp/internal/semconv/test/go.mod | 2 +- .../internal/semconv/test/httpconv_test.go | 2 +- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 8467c99b17b..cab3cf812ab 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -112,15 +112,22 @@ type MetricData struct { ElapsedTime float64 } -var metricAddOptionPool = &sync.Pool{ - New: func() interface{} { - return &[]metric.AddOption{} - }, -} +var ( + metricAddOptionPool = &sync.Pool{ + New: func() interface{} { + return &[]metric.AddOption{} + }, + } + + metricRecordOptionPool = &sync.Pool{ + New: func() interface{} { + return &[]metric.RecordOption{} + }, + } +) func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { if s.requestBytesCounter != nil && s.responseBytesCounter != nil && s.serverLatencyMeasure != nil { - attributes := OldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) addOpts := metricAddOptionPool.Get().(*[]metric.AddOption) @@ -135,9 +142,13 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { if s.duplicate && s.requestDurationHistogram != nil && s.requestBodySizeHistogram != nil && s.responseBodySizeHistogram != nil { attributes := CurrentHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) - s.requestBodySizeHistogram.Record(ctx, md.RequestSize, o) - s.responseBodySizeHistogram.Record(ctx, md.ResponseSize, o) + recordOpts := metricRecordOptionPool.Get().(*[]metric.RecordOption) + *recordOpts = append(*recordOpts, o) + s.requestBodySizeHistogram.Record(ctx, md.RequestSize, *recordOpts...) + s.responseBodySizeHistogram.Record(ctx, md.ResponseSize, *recordOpts...) s.requestDurationHistogram.Record(ctx, md.ElapsedTime, o) + *recordOpts = (*recordOpts)[:0] + metricRecordOptionPool.Put(recordOpts) } } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 8d3775b80d6..55e461d915d 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/metric/noop" ) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod b/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod index 9a0732efd15..fb6527513b6 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/go.mod @@ -6,6 +6,7 @@ require ( github.com/stretchr/testify v1.10.0 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.58.0 go.opentelemetry.io/otel v1.33.0 + go.opentelemetry.io/otel/metric v1.33.0 go.opentelemetry.io/otel/sdk v1.33.0 go.opentelemetry.io/otel/sdk/metric v1.33.0 ) @@ -17,7 +18,6 @@ require ( github.com/google/uuid v1.6.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/auto/sdk v1.1.0 // indirect - go.opentelemetry.io/otel/metric v1.33.0 // indirect go.opentelemetry.io/otel/trace v1.33.0 // indirect golang.org/x/sys v0.28.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go index 137c1717696..19ce914b47e 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" @@ -128,7 +129,6 @@ func TestNewRecordMetrics(t *testing.T) { expectedCurrentScopeMetric := expectedOldScopeMetric expectedCurrentScopeMetric.Metrics = append(expectedCurrentScopeMetric.Metrics, []metricdata.Metrics{ { - Name: "http.server.request.body.size", Description: "Size of HTTP server request bodies.", Unit: "By", From b750320e0026604a56e876196d0943e1cd09a05e Mon Sep 17 00:00:00 2001 From: Flc Date: Fri, 13 Dec 2024 16:16:45 +0800 Subject: [PATCH 14/18] optimize --- .../net/http/otelhttp/internal/semconv/test/httpconv_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go index 19ce914b47e..7b56aa1d150 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/test/httpconv_test.go @@ -192,7 +192,7 @@ func TestNewRecordMetrics(t *testing.T) { return semconv.NewHTTPServer(mp.Meter("test")) }, wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { - assert.Len(t, rm.ScopeMetrics, 1) + require.Len(t, rm.ScopeMetrics, 1) // because of OldHTTPServer require.Len(t, rm.ScopeMetrics[0].Metrics, 3) @@ -216,7 +216,7 @@ func TestNewRecordMetrics(t *testing.T) { return semconv.NewHTTPServer(mp.Meter("test")) }, wantFunc: func(t *testing.T, rm metricdata.ResourceMetrics) { - assert.Len(t, rm.ScopeMetrics, 1) + require.Len(t, rm.ScopeMetrics, 1) require.Len(t, rm.ScopeMetrics[0].Metrics, 6) metricdatatest.AssertEqual(t, expectedCurrentScopeMetric, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) }, From 6a94cce348d236a16c9349198f7356f116bd5f1b Mon Sep 17 00:00:00 2001 From: Flc Date: Fri, 13 Dec 2024 16:27:31 +0800 Subject: [PATCH 15/18] test(otelhttp): optimize tests --- .../otelhttp/internal/semconv/httpconv.go | 12 +--- .../http/otelhttp/internal/semconv/util.go | 10 +++ .../otelhttp/internal/semconv/util_test.go | 31 +++++++++ .../http/otelhttp/internal/semconv/v1.20.0.go | 15 +---- .../otelhttp/internal/semconv/v1.20.0_test.go | 66 ------------------- 5 files changed, 44 insertions(+), 90 deletions(-) delete mode 100644 instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go index 10b597bff1b..3c6c6a9b93b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go @@ -262,7 +262,7 @@ func (n CurrentHTTPServer) MetricAttributes(server string, req *http.Request, st attributes := slices.Grow(additionalAttributes, num) attributes = append(attributes, - standardizeNewHTTPMethodMetric(req.Method), + semconvNew.HTTPRequestMethodKey.String(standardizeHTTPMethod(req.Method)), n.scheme(req.TLS != nil), semconvNew.ServerAddress(host)) @@ -429,13 +429,3 @@ func (n CurrentHTTPClient) method(method string) (attribute.KeyValue, attribute. func isErrorStatusCode(code int) bool { return code >= 400 || code < 100 } - -func standardizeNewHTTPMethodMetric(method string) attribute.KeyValue { - method = strings.ToUpper(method) - switch method { - case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace: - default: - method = "_OTHER" - } - return semconvNew.HTTPRequestMethodKey.String(method) -} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/util.go b/instrumentation/net/http/otelhttp/internal/semconv/util.go index 93e8d0f94c1..2ce8f0d9591 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/util.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/util.go @@ -96,3 +96,13 @@ func handleErr(err error) { otel.Handle(err) } } + +func standardizeHTTPMethod(method string) string { + method = strings.ToUpper(method) + switch method { + case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace: + default: + method = "_OTHER" + } + return method +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/util_test.go b/instrumentation/net/http/otelhttp/internal/semconv/util_test.go index b0fe5439883..ccc3619b5c0 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/util_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/util_test.go @@ -39,3 +39,34 @@ func TestSplitHostPort(t *testing.T) { assert.Equal(t, test.port, p, test.hostport) } } + +func TestStandardizeHTTPMethod(t *testing.T) { + tests := []struct { + method string + want string + }{ + {"GET", "GET"}, + {"get", "GET"}, + {"POST", "POST"}, + {"post", "POST"}, + {"PUT", "PUT"}, + {"put", "PUT"}, + {"DELETE", "DELETE"}, + {"delete", "DELETE"}, + {"HEAD", "HEAD"}, + {"head", "HEAD"}, + {"OPTIONS", "OPTIONS"}, + {"options", "OPTIONS"}, + {"CONNECT", "CONNECT"}, + {"connect", "CONNECT"}, + {"TRACE", "TRACE"}, + {"trace", "TRACE"}, + {"PATCH", "PATCH"}, + {"patch", "PATCH"}, + {"unknown", "_OTHER"}, + {"", "_OTHER"}, + } + for _, test := range tests { + assert.Equal(t, test.want, standardizeHTTPMethod(test.method)) + } +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go index c042249dd72..8899cdb725f 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go @@ -8,7 +8,6 @@ import ( "io" "net/http" "slices" - "strings" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil" "go.opentelemetry.io/otel/attribute" @@ -144,7 +143,7 @@ func (o OldHTTPServer) MetricAttributes(server string, req *http.Request, status attributes := slices.Grow(additionalAttributes, n) attributes = append(attributes, - standardizeHTTPMethodMetric(req.Method), + semconv.HTTPMethod(standardizeHTTPMethod(req.Method)), o.scheme(req.TLS != nil), semconv.NetHostName(host)) @@ -214,7 +213,7 @@ func (o OldHTTPClient) MetricAttributes(req *http.Request, statusCode int, addit attributes := slices.Grow(additionalAttributes, n) attributes = append(attributes, - standardizeHTTPMethodMetric(req.Method), + semconv.HTTPMethod(standardizeHTTPMethod(req.Method)), semconv.NetPeerName(requestHost), ) @@ -262,13 +261,3 @@ func (o OldHTTPClient) createMeasures(meter metric.Meter) (metric.Int64Counter, return requestBytesCounter, responseBytesCounter, latencyMeasure } - -func standardizeHTTPMethodMetric(method string) attribute.KeyValue { - method = strings.ToUpper(method) - switch method { - case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace: - default: - method = "_OTHER" - } - return semconv.HTTPMethod(method) -} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go deleted file mode 100644 index a3731492729..00000000000 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package semconv - -import ( - "testing" - - "github.com/stretchr/testify/assert" - - "go.opentelemetry.io/otel/attribute" -) - -func TestStandardizeHTTPMethodMetric(t *testing.T) { - testCases := []struct { - method string - want attribute.KeyValue - }{ - { - method: "GET", - want: attribute.String("http.method", "GET"), - }, - { - method: "POST", - want: attribute.String("http.method", "POST"), - }, - { - method: "PUT", - want: attribute.String("http.method", "PUT"), - }, - { - method: "DELETE", - want: attribute.String("http.method", "DELETE"), - }, - { - method: "HEAD", - want: attribute.String("http.method", "HEAD"), - }, - { - method: "OPTIONS", - want: attribute.String("http.method", "OPTIONS"), - }, - { - method: "CONNECT", - want: attribute.String("http.method", "CONNECT"), - }, - { - method: "TRACE", - want: attribute.String("http.method", "TRACE"), - }, - { - method: "PATCH", - want: attribute.String("http.method", "PATCH"), - }, - { - method: "test", - want: attribute.String("http.method", "_OTHER"), - }, - } - for _, tt := range testCases { - t.Run(tt.method, func(t *testing.T) { - got := standardizeHTTPMethodMetric(tt.method) - assert.Equal(t, tt.want, got) - }) - } -} From 1bf0f8dbecb20202700a6a667af1384647aa4083 Mon Sep 17 00:00:00 2001 From: Flc Date: Fri, 13 Dec 2024 16:39:02 +0800 Subject: [PATCH 16/18] test(otelhttp): optimize tests --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aafe7d3a53d..48f6d0202ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added -- Generate new server metrics upon setting the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN=http/dup` within `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) +- Set the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN` to `http/dup` to generate new server metric information based on the `semconv` 1.26 version in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) ## [1.33.0/0.58.0/0.27.0/0.13.0/0.8.0/0.6.0/0.5.0] - 2024-12-12 From fb25ee409b7c398366502223693f2a50909f7817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Flc=E3=82=9B?= Date: Fri, 13 Dec 2024 21:47:07 +0800 Subject: [PATCH 17/18] Update CHANGELOG.md Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48f6d0202ce..bd817ab8e9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added -- Set the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN` to `http/dup` to generate new server metric information based on the `semconv` 1.26 version in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6411) +- Generate server metrics with semantic conventions v1.26 in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when `OTEL_SEMCONV_STABILITY_OPT_IN` is set to `http/dup`. (#6411) ## [1.33.0/0.58.0/0.27.0/0.13.0/0.8.0/0.6.0/0.5.0] - 2024-12-12 From f88778042a2f3aebc5f8211562dd38fbaaaa491b Mon Sep 17 00:00:00 2001 From: Flc Date: Sat, 14 Dec 2024 11:40:29 +0800 Subject: [PATCH 18/18] test(otelhttp): increase unit test coverage --- .../internal/semconv/httpconv_test.go | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 47b4db548ce..899661c4ad8 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -9,11 +9,72 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" ) +func TestCurrentHttpServer_MetricAttributes(t *testing.T) { + defaultRequest, err := http.NewRequest("GET", "http://example.com/path?query=test", nil) + require.NoError(t, err) + + tests := []struct { + name string + server string + req *http.Request + statusCode int + additionalAttributes []attribute.KeyValue + wantFunc func(t *testing.T, attrs []attribute.KeyValue) + }{ + { + name: "routine testing", + server: "", + req: defaultRequest, + statusCode: 200, + additionalAttributes: []attribute.KeyValue{attribute.String("test", "test")}, + wantFunc: func(t *testing.T, attrs []attribute.KeyValue) { + require.Len(t, attrs, 7) + assert.ElementsMatch(t, []attribute.KeyValue{ + attribute.String("http.request.method", "GET"), + attribute.String("url.scheme", "http"), + attribute.String("server.address", "example.com"), + attribute.String("network.protocol.name", "http"), + attribute.String("network.protocol.version", "1.1"), + attribute.Int64("http.response.status_code", 200), + attribute.String("test", "test"), + }, attrs) + }, + }, + { + name: "use server address", + server: "example.com:9999", + req: defaultRequest, + statusCode: 200, + additionalAttributes: nil, + wantFunc: func(t *testing.T, attrs []attribute.KeyValue) { + require.Len(t, attrs, 7) + assert.ElementsMatch(t, []attribute.KeyValue{ + attribute.String("http.request.method", "GET"), + attribute.String("url.scheme", "http"), + attribute.String("server.address", "example.com"), + attribute.Int("server.port", 9999), + attribute.String("network.protocol.name", "http"), + attribute.String("network.protocol.version", "1.1"), + attribute.Int64("http.response.status_code", 200), + }, attrs) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := CurrentHTTPServer{}.MetricAttributes(tt.server, tt.req, tt.statusCode, tt.additionalAttributes) + tt.wantFunc(t, got) + }) + } +} + func TestNewMethod(t *testing.T) { testCases := []struct { method string