-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use more efficient WithAttributeSet()
#5664
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5664 +/- ##
=====================================
Coverage 63.5% 63.5%
=====================================
Files 194 194
Lines 12061 12064 +3
=====================================
+ Hits 7667 7670 +3
Misses 4178 4178
Partials 216 216
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run the existing benchmarks to see if they provide any improvement information?
I ran existing benchmarks in the package but they are unreliable. They report a different numbers on each run: 3 runs on the
3 runs on this PR:
p.s. 5fdddf2 broke the benchmarks and some of the tests now work differently. |
Co-authored-by: Damien Mathieu <42@dmathieu.com>
02a6d2f
to
059d3ce
Compare
) This adds high-level benchmarks for `otelhttp.Handler.ServeHTTP` and `otelhttp.Transport.RoundTrip`, as well as a benchmark comparison without those wrappers. Related: #5664. Current results on my machine: ``` goos: darwin goarch: arm64 pkg: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/test BenchmarkHandlerServeHTTP/without_the_otelhttp_handler-10 37971928 31.35 ns/op 28 B/op 0 allocs/op BenchmarkHandlerServeHTTP/with_the_otelhttp_handler-10 435142 2723 ns/op 5585 B/op 35 allocs/op BenchmarkTransportRoundTrip/without_the_otelhttp_transport-10 11430 401735 ns/op 24436 B/op 117 allocs/op BenchmarkTransportRoundTrip/with_the_otelhttp_transport-10 278 4367998 ns/op 5416 B/op 85 allocs/op PASS ok go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/test 10.056s ``` --------- Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
h.requestBytesCounter.Add(ctx, bw.read.Load(), o) | ||
h.responseBytesCounter.Add(ctx, rww.written, o) | ||
o := metric.WithAttributeSet(attribute.NewSet(attributes...)) | ||
addOpts := []metric.AddOption{o} // Allocate vararg slice once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ash2k please explain why this allocation is needed?
https://github.com/open-telemetry/opentelemetry-go/blob/5661ff0ded32cf1b83f1147dae96ca403c198504/metric/instrument.go#L349-L364
Since all the usage places of
WithAttributes()
operate on a locally allocated slice, there is no need to make a defensive copy.