Skip to content

Commit

Permalink
Add method and benchmarks for pooling metric options (#6394)
Browse files Browse the repository at this point in the history
This PR improves the `RecordMetrics()` allocations using a pool for the
`metric.AddOption` slice.

`benchstat main.txt pool.txt`:
```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv
cpu: Apple M1 Max
                            │  main.txt   │              pool.txt              │
                            │   sec/op    │   sec/op     vs base               │
RecordMetrics/empty-10        4.513n ± 3%   4.620n ± 0%  +2.38% (p=0.037 n=10)
RecordMetrics/nil_meter-10    341.8n ± 1%   330.7n ± 4%  -3.26% (p=0.022 n=10)
RecordMetrics/with_Meter-10   341.1n ± 1%   330.0n ± 1%  -3.27% (p=0.000 n=10)
geomean                       80.74n        79.59n       -1.42%

                            │   main.txt   │              pool.txt               │
                            │     B/op     │    B/op     vs base                 │
RecordMetrics/empty-10        0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
RecordMetrics/nil_meter-10    696.0 ± 0%     680.0 ± 0%  -2.30% (p=0.000 n=10)
RecordMetrics/with_Meter-10   696.0 ± 0%     680.0 ± 0%  -2.30% (p=0.000 n=10)
geomean                                  ²               -1.54%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │   main.txt   │               pool.txt               │
                            │  allocs/op   │ allocs/op   vs base                  │
RecordMetrics/empty-10        0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
RecordMetrics/nil_meter-10    6.000 ± 0%     5.000 ± 0%  -16.67% (p=0.000 n=10)
RecordMetrics/with_Meter-10   6.000 ± 0%     5.000 ± 0%  -16.67% (p=0.000 n=10)
geomean                                  ²               -11.45%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean
```

Fixes
#5968

---------

Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
  • Loading branch information
3 people authored Dec 12, 2024
1 parent 378d704 commit 0c0b385
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +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 `sync.Pool` for metric options in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#6394)

### Changed

Expand Down
16 changes: 13 additions & 3 deletions instrumentation/net/http/otelhttp/internal/semconv/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net/http"
"os"
"strings"
"sync"

"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
Expand Down Expand Up @@ -102,6 +103,12 @@ type MetricData struct {
ElapsedTime float64
}

var metricAddOptionPool = &sync.Pool{
New: func() interface{} {
return &[]metric.AddOption{}
},
}

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.
Expand All @@ -110,10 +117,13 @@ 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 := []metric.AddOption{o}
s.requestBytesCounter.Add(ctx, md.RequestSize, addOpts...)
s.responseBytesCounter.Add(ctx, md.ResponseSize, addOpts...)
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)

// TODO: Duplicate Metrics
}
Expand Down
39 changes: 39 additions & 0 deletions instrumentation/net/http/otelhttp/internal/semconv/env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,42 @@ func TestHTTPClientDoesNotPanic(t *testing.T) {
})
}
}

func BenchmarkRecordMetrics(b *testing.B) {
benchmarks := []struct {
name string
server HTTPServer
}{
{
name: "empty",
server: HTTPServer{},
},
{
name: "nil meter",
server: NewHTTPServer(nil),
},
{
name: "with Meter",
server: NewHTTPServer(noop.Meter{}),
},
}

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})
ctx := context.Background()
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
bm.server.RecordMetrics(ctx, ServerMetricData{
ServerName: bm.name,
MetricAttributes: MetricAttributes{
Req: req,
},
})
}
})
}
}

0 comments on commit 0c0b385

Please sign in to comment.