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

exporter/collector: avoid using req.String() to check for empty request #889

Merged

Conversation

ridwanmsharif
Copy link
Contributor

@ridwanmsharif ridwanmsharif commented Sep 9, 2024

This change is motivated by some CPU profiling that showed that its responsible for over 20% of CPU time in my environment (running avalanche, OTel with Prom receiver and the gmp exporter). Sending this change because its logically equivalent to before but avoids the serialization to the string.

Flamegraph: pprof/?id=f2daddf0bdf127bfa136618996b263c2&tab=flame&path=1ygnz740h91y6o

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/check-empty branch 3 times, most recently from 960c664 to 709d9af Compare September 9, 2024 20:56
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/check-empty branch 2 times, most recently from a6978e7 to 2adedca Compare September 10, 2024 01:38
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.99%. Comparing base (4caace7) to head (15099b9).
Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
+ Coverage   61.03%   62.99%   +1.96%     
==========================================
  Files          56       57       +1     
  Lines        5903     6013     +110     
==========================================
+ Hits         3603     3788     +185     
+ Misses       2143     2063      -80     
- Partials      157      162       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ridwanmsharif ridwanmsharif marked this pull request as ready for review September 10, 2024 01:44
@ridwanmsharif ridwanmsharif requested a review from a team as a code owner September 10, 2024 01:44
Copy link

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, amazing work! Sounds like our benchmarks already has been proven to be useful.

The best optimizations are one-liners 💪🏽

I guess it would be nice to run some microbenchmark here to double check, but in theory this be faster vs stringifying

exporter/collector/metrics.go Outdated Show resolved Hide resolved
exporter/collector/metrics.go Outdated Show resolved Hide resolved
@ridwanmsharif
Copy link
Contributor Author

ridwanmsharif commented Sep 10, 2024

Added some microbenchmarks, here are some savings:

➜  collector git:(ridwanmsharif/check-empty) ✗ go test -bench . -benchmem -count=10 > orig.txt
... applied the fix ...
➜  collector git:(ridwanmsharif/check-empty) ✗ go test -bench . -benchmem -count=10 > fix.txt 
➜  collector git:(ridwanmsharif/check-empty) ✗ benchstat orig.txt fix.txt            
goos: linux
goarch: amd64
pkg: github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/collector
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
                         │  orig.txt   │               fix.txt               │
                         │   sec/op    │   sec/op     vs base                │
_TestExport-16             3.194µ ± 7%   1.910µ ± 7%  -40.21% (p=0.000 n=10)
_TestReadWALAndExport-16   7.585m ± 3%   6.797m ± 3%  -10.39% (p=0.000 n=10)
geomean                    155.6µ        113.9µ       -26.80%

                         │   orig.txt   │               fix.txt                │
                         │     B/op     │     B/op      vs base                │
_TestExport-16               953.0 ± 0%     656.0 ± 0%  -31.16% (p=0.000 n=10)
_TestReadWALAndExport-16   8.060Ki ± 1%   7.466Ki ± 0%   -7.36% (p=0.000 n=10)
geomean                    2.739Ki        2.187Ki       -20.14%

                         │  orig.txt  │              fix.txt               │
                         │ allocs/op  │ allocs/op   vs base                │
_TestExport-16             20.00 ± 0%   10.00 ± 0%  -50.00% (p=0.000 n=10)
_TestReadWALAndExport-16   128.0 ± 0%   108.0 ± 0%  -15.62% (p=0.000 n=10)
geomean                    50.60        32.86       -35.05%

When WAL is disabled (default) the savings are higher, but with WAL this still has significant savings.

Note that these numbers are pretty fudged since the exporting logic is omitted, but some significant improvement is visible nonetheless

@damemi
Copy link
Contributor

damemi commented Sep 12, 2024

/gcbrun

This change is motivated by some CPU profiling that showed that its
responsible for over 20% of CPU time in my environment (running
avalanche, OTel with Prom receiver and the gmp exporter). Sending this
change because its logically equivalent to before but avoids the
serialization to the string.
@ridwanmsharif ridwanmsharif merged commit 78c2bb5 into GoogleCloudPlatform:main Sep 12, 2024
34 checks passed
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.

4 participants