Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add method and benchmarks for pooling metric options #6394

Merged
merged 16 commits into from
Dec 12, 2024

Conversation

vordimous
Copy link
Contributor

@vordimous vordimous commented Dec 2, 2024

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

@vordimous vordimous requested a review from dmathieu December 3, 2024 18:00
@vordimous vordimous force-pushed the pool-metric-options branch 2 times, most recently from 93a6e56 to 24be4a7 Compare December 5, 2024 12:54
@vordimous vordimous marked this pull request as ready for review December 5, 2024 14:00
@vordimous vordimous requested a review from a team as a code owner December 5, 2024 14:00
@vordimous vordimous force-pushed the pool-metric-options branch from 22d5b93 to 57fc034 Compare December 5, 2024 16:06
@vordimous vordimous requested a review from dmathieu December 5, 2024 16:07
@vordimous
Copy link
Contributor Author

vordimous commented Dec 5, 2024

@dmathieu I implemented the feedback and followed the BenchmarkCoreWrite() to have each case benchmarked.

I also created the spelling fix PR: #6407

Thank you for all of the feedback and guiding me through the proper ways to implement this change.

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.8%. Comparing base (378d704) to head (dd9d610).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6394   +/-   ##
=====================================
  Coverage   67.8%   67.8%           
=====================================
  Files        200     200           
  Lines      16635   16641    +6     
=====================================
+ Hits       11291   11297    +6     
  Misses      4999    4999           
  Partials     345     345           
Files with missing lines Coverage Δ
...entation/net/http/otelhttp/internal/semconv/env.go 91.0% <100.0%> (+0.5%) ⬆️

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

This is starting to look good :)
We will need a changelog entry though.

@vordimous vordimous force-pushed the pool-metric-options branch from e246950 to 6991013 Compare December 6, 2024 13:15
@vordimous vordimous requested a review from dmathieu December 6, 2024 13:20
CHANGELOG.md Outdated Show resolved Hide resolved
@vordimous vordimous requested a review from dmathieu December 6, 2024 14:12
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Can you please update the branch?

…trib into pool-metric-options

# Conflicts:
#	CHANGELOG.md
#	instrumentation/net/http/otelhttp/internal/semconv/env_test.go
@vordimous
Copy link
Contributor Author

Thanks for the PR. Can you please update the branch?

PR updated to resolve conflicts

@MrAlias MrAlias added this to the v1.33.0 milestone Dec 12, 2024
Revert invalid spelling change.
CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 0c0b385 into open-telemetry:main Dec 12, 2024
25 checks passed
@vordimous vordimous deleted the pool-metric-options branch December 12, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pool metric options created in the otelhttp instrumentation
4 participants