From b553e4257efcb92a0f66cd0e1f46a6289222c5f4 Mon Sep 17 00:00:00 2001 From: AJ Date: Mon, 2 Dec 2024 15:44:12 -0500 Subject: [PATCH 01/14] feat: add method and benchmarks for pooling metric options --- .../net/http/otelhttp/internal/semconv/env.go | 25 ++++ .../otelhttp/internal/semconv/env_test.go | 107 ++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index fec528536a5..41a64d80705 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -9,6 +9,7 @@ import ( "net/http" "os" "strings" + "sync" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -118,6 +119,30 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { // TODO: Duplicate Metrics } +var metricAddOptionPool = &sync.Pool{ + New: func() interface{} { + return &[]metric.AddOption{} + }, +} + +// RecordMetricsPooled is an implementation of RecordMetrics() where []metric.AddOption is pulled from a pool +func (s HTTPServer) RecordMetricsPooled(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 + } + + attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) + o := metric.WithAttributeSet(attribute.NewSet(attributes...)) + addOpts := metricAddOptionPool.Get().(*[]metric.AddOption) + *addOpts = append(*addOpts, o) + s.requestBytesCounter.Add(ctx, md.RequestSize, *addOpts...) + s.responseBytesCounter.Add(ctx, md.ResponseSize, *addOpts...) + s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) + *addOpts = (*addOpts)[:0] + metricAddOptionPool.Put(addOpts) +} + func NewHTTPServer(meter metric.Meter) HTTPServer { env := strings.ToLower(os.Getenv("OTEL_SEMCONV_STABILITY_OPT_IN")) 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..02561e98399 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -53,6 +53,43 @@ func TestHTTPServerDoesNotPanic(t *testing.T) { } } +func TestHTTPServerDoesNotPanicPooled(t *testing.T) { + testCases := []struct { + name string + server HTTPServer + }{ + { + name: "empty", + server: HTTPServer{}, + }, + { + name: "nil meter", + server: NewHTTPServer(nil), + }, + { + name: "with Meter", + server: NewHTTPServer(noop.Meter{}), + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + require.NotPanics(t, func() { + req, err := http.NewRequest("GET", "http://example.com", nil) + require.NoError(t, err) + + _ = tt.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + }) + }) + } +} + func TestHTTPClientDoesNotPanic(t *testing.T) { testCases := []struct { name string @@ -132,3 +169,73 @@ func NewTestHTTPClient() HTTPClient { latencyMeasure: &testInst{}, } } + +func BenchmarkRecordMetricsPooled(b *testing.B) { + testCases := []struct { + name string + server HTTPServer + }{ + { + name: "empty", + server: HTTPServer{}, + }, + { + name: "nil meter", + server: NewHTTPServer(nil), + }, + { + name: "with Meter", + server: NewHTTPServer(noop.Meter{}), + }, + } + for _, tt := range testCases { + req, _ := http.NewRequest("GET", "http://example.com", nil) + + _ = tt.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + } + + b.ReportAllocs() + b.ResetTimer() +} + +func BenchmarkRecordMetrics(b *testing.B) { + testCases := []struct { + name string + server HTTPServer + }{ + { + name: "empty", + server: HTTPServer{}, + }, + { + name: "nil meter", + server: NewHTTPServer(nil), + }, + { + name: "with Meter", + server: NewHTTPServer(noop.Meter{}), + }, + } + for _, tt := range testCases { + req, _ := http.NewRequest("GET", "http://example.com", nil) + + _ = tt.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + } + + b.ReportAllocs() + b.ResetTimer() +} From c88e6e4eafd74c7075a78c3e40a0d8c8a70bfa7b Mon Sep 17 00:00:00 2001 From: AJ Date: Tue, 3 Dec 2024 12:52:15 -0500 Subject: [PATCH 02/14] fix: correctly impelemnt the record metrics benchmarks --- .../otelhttp/internal/semconv/env_test.go | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 02561e98399..a6544bba100 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -188,21 +188,22 @@ func BenchmarkRecordMetricsPooled(b *testing.B) { server: NewHTTPServer(noop.Meter{}), }, } - for _, tt := range testCases { - req, _ := http.NewRequest("GET", "http://example.com", nil) - - _ = tt.server.RequestTraceAttrs("stuff", req) - _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ - ServerName: "stuff", - MetricAttributes: MetricAttributes{ - Req: req, - }, - }) - } b.ReportAllocs() b.ResetTimer() + for i := 0; i < b.N; i++ { + for _, tt := range testCases { + req, _ := http.NewRequest("GET", "http://example.com", nil) + _ = tt.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + } + } } func BenchmarkRecordMetrics(b *testing.B) { @@ -223,19 +224,20 @@ func BenchmarkRecordMetrics(b *testing.B) { server: NewHTTPServer(noop.Meter{}), }, } - for _, tt := range testCases { - req, _ := http.NewRequest("GET", "http://example.com", nil) - - _ = tt.server.RequestTraceAttrs("stuff", req) - _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - tt.server.RecordMetrics(context.Background(), ServerMetricData{ - ServerName: "stuff", - MetricAttributes: MetricAttributes{ - Req: req, - }, - }) - } - b.ReportAllocs() b.ResetTimer() + for i := 0; i < b.N; i++ { + for _, tt := range testCases { + req, _ := http.NewRequest("GET", "http://example.com", nil) + + _ = tt.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + } + } } From e87885a688a012fb9e3cffa3269b80ce4069f779 Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 5 Dec 2024 07:49:42 -0500 Subject: [PATCH 03/14] fix: remove extra method and update existing --- .../net/http/otelhttp/internal/semconv/env.go | 21 +----- .../otelhttp/internal/semconv/env_test.go | 75 +------------------ 2 files changed, 4 insertions(+), 92 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 41a64d80705..05159d7d021 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -103,30 +103,13 @@ type MetricData struct { ElapsedTime float64 } -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 - } - - 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 -} - var metricAddOptionPool = &sync.Pool{ New: func() interface{} { return &[]metric.AddOption{} }, } -// RecordMetricsPooled is an implementation of RecordMetrics() where []metric.AddOption is pulled from a pool -func (s HTTPServer) RecordMetricsPooled(ctx context.Context, md ServerMetricData) { +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 @@ -141,6 +124,8 @@ func (s HTTPServer) RecordMetricsPooled(ctx context.Context, md ServerMetricData s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) *addOpts = (*addOpts)[:0] metricAddOptionPool.Put(addOpts) + + // TODO: Duplicate Metrics } func NewHTTPServer(meter metric.Meter) HTTPServer { diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index a6544bba100..83a4e72481b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -53,43 +53,6 @@ func TestHTTPServerDoesNotPanic(t *testing.T) { } } -func TestHTTPServerDoesNotPanicPooled(t *testing.T) { - testCases := []struct { - name string - server HTTPServer - }{ - { - name: "empty", - server: HTTPServer{}, - }, - { - name: "nil meter", - server: NewHTTPServer(nil), - }, - { - name: "with Meter", - server: NewHTTPServer(noop.Meter{}), - }, - } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - require.NotPanics(t, func() { - req, err := http.NewRequest("GET", "http://example.com", nil) - require.NoError(t, err) - - _ = tt.server.RequestTraceAttrs("stuff", req) - _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ - ServerName: "stuff", - MetricAttributes: MetricAttributes{ - Req: req, - }, - }) - }) - }) - } -} - func TestHTTPClientDoesNotPanic(t *testing.T) { testCases := []struct { name string @@ -170,42 +133,6 @@ func NewTestHTTPClient() HTTPClient { } } -func BenchmarkRecordMetricsPooled(b *testing.B) { - testCases := []struct { - name string - server HTTPServer - }{ - { - name: "empty", - server: HTTPServer{}, - }, - { - name: "nil meter", - server: NewHTTPServer(nil), - }, - { - name: "with Meter", - server: NewHTTPServer(noop.Meter{}), - }, - } - - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - for _, tt := range testCases { - req, _ := http.NewRequest("GET", "http://example.com", nil) - _ = tt.server.RequestTraceAttrs("stuff", req) - _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - tt.server.RecordMetricsPooled(context.Background(), ServerMetricData{ - ServerName: "stuff", - MetricAttributes: MetricAttributes{ - Req: req, - }, - }) - } - } -} - func BenchmarkRecordMetrics(b *testing.B) { testCases := []struct { name string @@ -229,7 +156,7 @@ func BenchmarkRecordMetrics(b *testing.B) { for i := 0; i < b.N; i++ { for _, tt := range testCases { req, _ := http.NewRequest("GET", "http://example.com", nil) - + _ = tt.server.RequestTraceAttrs("stuff", req) _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) tt.server.RecordMetrics(context.Background(), ServerMetricData{ From e7451dba5d2175e8adc04fa8c3a2e1d9105b4a00 Mon Sep 17 00:00:00 2001 From: AJ Danelz Date: Thu, 5 Dec 2024 10:25:47 -0500 Subject: [PATCH 04/14] fix: run benches for each test case and isolate RecordMetrics() Co-authored-by: Damien Mathieu <42@dmathieu.com> --- .../otelhttp/internal/semconv/env_test.go | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index 83a4e72481b..a0660ed3240 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -153,18 +153,20 @@ func BenchmarkRecordMetrics(b *testing.B) { } b.ReportAllocs() b.ResetTimer() - for i := 0; i < b.N; i++ { - for _, tt := range testCases { + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) _ = tt.server.RequestTraceAttrs("stuff", req) _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - tt.server.RecordMetrics(context.Background(), ServerMetricData{ - ServerName: "stuff", - MetricAttributes: MetricAttributes{ - Req: req, - }, - }) - } + for i := 0; i < b.N; i++ { + tt.server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", + MetricAttributes: MetricAttributes{ + Req: req, + }, + }) + } + }) } } From 2ef6ea8bb7dac1e0b36404962ff7da3b2c7c4bd0 Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 5 Dec 2024 10:29:09 -0500 Subject: [PATCH 05/14] revert: spellin change --- 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 05159d7d021..b8963eae62e 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -111,7 +111,7 @@ var metricAddOptionPool = &sync.Pool{ 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. + // This will happen if an HTTPServer{} is used insted of NewHTTPServer. return } @@ -216,7 +216,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 } @@ -228,7 +228,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 } From b6789bc5b51b1014eb33f66948e13e387b4d6ce5 Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 5 Dec 2024 10:57:21 -0500 Subject: [PATCH 06/14] fix: benchmark each case individually --- .../otelhttp/internal/semconv/env_test.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index a0660ed3240..a6562a4be30 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -134,7 +134,7 @@ func NewTestHTTPClient() HTTPClient { } func BenchmarkRecordMetrics(b *testing.B) { - testCases := []struct { + benchmarks := []struct { name string server HTTPServer }{ @@ -151,17 +151,18 @@ func BenchmarkRecordMetrics(b *testing.B) { server: NewHTTPServer(noop.Meter{}), }, } - b.ReportAllocs() - b.ResetTimer() - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + + for _, bm := range benchmarks { + b.Run(bm.name, func(b *testing.B) { req, _ := http.NewRequest("GET", "http://example.com", nil) + _ = bm.server.RequestTraceAttrs("stuff", req) + _ = bm.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - _ = tt.server.RequestTraceAttrs("stuff", req) - _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + b.ReportAllocs() + b.ResetTimer() for i := 0; i < b.N; i++ { - tt.server.RecordMetrics(context.Background(), ServerMetricData{ - ServerName: "stuff", + bm.server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: bm.name, MetricAttributes: MetricAttributes{ Req: req, }, From a9cd8f510709b8793401b60a9d623c4615afe480 Mon Sep 17 00:00:00 2001 From: AJ Date: Fri, 6 Dec 2024 08:11:01 -0500 Subject: [PATCH 07/14] fix: move context creation out of alloc report --- .../net/http/otelhttp/internal/semconv/env_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go index a6562a4be30..6dfb91c5553 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -157,11 +157,11 @@ func BenchmarkRecordMetrics(b *testing.B) { req, _ := http.NewRequest("GET", "http://example.com", nil) _ = bm.server.RequestTraceAttrs("stuff", req) _ = bm.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) - + ctx := context.Background() b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - bm.server.RecordMetrics(context.Background(), ServerMetricData{ + bm.server.RecordMetrics(ctx, ServerMetricData{ ServerName: bm.name, MetricAttributes: MetricAttributes{ Req: req, From d07d42e49a3d980126b04536ef984ff31fbf7686 Mon Sep 17 00:00:00 2001 From: AJ Date: Fri, 6 Dec 2024 08:11:17 -0500 Subject: [PATCH 08/14] fix: empty slice after getting from pool --- instrumentation/net/http/otelhttp/internal/semconv/env.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index b8963eae62e..c4d00cbaa9b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -118,7 +118,7 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) addOpts := metricAddOptionPool.Get().(*[]metric.AddOption) - *addOpts = append(*addOpts, o) + *addOpts = append((*addOpts)[:0], o) s.requestBytesCounter.Add(ctx, md.RequestSize, *addOpts...) s.responseBytesCounter.Add(ctx, md.ResponseSize, *addOpts...) s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) From 699101388e629a0c52f39f330cca83b532d7cdf8 Mon Sep 17 00:00:00 2001 From: AJ Date: Fri, 6 Dec 2024 08:14:30 -0500 Subject: [PATCH 09/14] chore: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61f0af8f6b..3aef1e49d4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +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) +- Updated the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv` `RecordMetrics()` to used a pool for the addOptions slice which reduced allocs/op. (#6394) ### Changed From 369ee1810b70cbaf081aca08ac59d5d5270e0ab6 Mon Sep 17 00:00:00 2001 From: AJ Danelz Date: Fri, 6 Dec 2024 09:09:14 -0500 Subject: [PATCH 10/14] 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 3aef1e49d4b..047fb0cf593 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) -- Updated the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv` `RecordMetrics()` to used a pool for the addOptions slice which reduced allocs/op. (#6394) +- Use a pool for the semantic convention metrics options in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6394) ### Changed From a20b4c01ac57d841bbf3b020158338e2e1e5ceb4 Mon Sep 17 00:00:00 2001 From: AJ Date: Fri, 6 Dec 2024 09:12:05 -0500 Subject: [PATCH 11/14] feat: only empty the slice before putting it into the pool --- instrumentation/net/http/otelhttp/internal/semconv/env.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index c4d00cbaa9b..c5e8d364a48 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -118,12 +118,11 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes) o := metric.WithAttributeSet(attribute.NewSet(attributes...)) addOpts := metricAddOptionPool.Get().(*[]metric.AddOption) - *addOpts = append((*addOpts)[:0], o) + *addOpts = append(*addOpts, o) s.requestBytesCounter.Add(ctx, md.RequestSize, *addOpts...) s.responseBytesCounter.Add(ctx, md.ResponseSize, *addOpts...) s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) - *addOpts = (*addOpts)[:0] - metricAddOptionPool.Put(addOpts) + metricAddOptionPool.Put((*addOpts)[:0]) // TODO: Duplicate Metrics } From 1cf36838a2ff0ebdad28838eb0361f0213364e68 Mon Sep 17 00:00:00 2001 From: AJ Date: Fri, 6 Dec 2024 10:13:45 -0500 Subject: [PATCH 12/14] fix: pointer allocation --- instrumentation/net/http/otelhttp/internal/semconv/env.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index c5e8d364a48..b8963eae62e 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -122,7 +122,8 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) { s.requestBytesCounter.Add(ctx, md.RequestSize, *addOpts...) s.responseBytesCounter.Add(ctx, md.ResponseSize, *addOpts...) s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) - metricAddOptionPool.Put((*addOpts)[:0]) + *addOpts = (*addOpts)[:0] + metricAddOptionPool.Put(addOpts) // TODO: Duplicate Metrics } From db511baaa18db3f6f11026aad49a6dd6dfe75277 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 12 Dec 2024 10:07:57 -0800 Subject: [PATCH 13/14] Apply suggestions from code review Revert invalid spelling change. --- 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 280677d7dad..3b036f8a37b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -111,7 +111,7 @@ var metricAddOptionPool = &sync.Pool{ 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 insted of NewHTTPServer. + // This will happen if an HTTPServer{} is used instead of NewHTTPServer. return } @@ -216,7 +216,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 } @@ -228,7 +228,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 dd9d6101a18e00f78a7cc2ea51dc97809c5f9cbf Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 12 Dec 2024 10:08:53 -0800 Subject: [PATCH 14/14] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f34d5c264d..3d6b0ec8fc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `go.opentelemetry.io/contrib/bridges/otellogr` module. This module provides an OpenTelemetry logging bridge for `github.com/go-logr/logr`. (#6386) - Added SNS instrumentation in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#6388) -- Use a pool for the semantic convention metrics options in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6394) +- Use a `sync.Pool` for metric options in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6394) ### Changed