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

[chore][exporter][batcher] Improve exporter.request merge splitting performance by caching items count #12136

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

sfc-gh-sili
Copy link
Contributor

@sfc-gh-sili sfc-gh-sili commented Jan 21, 2025

Description

This PR improves item-based merge splitting by caching item count in exporter.request. Benchmarks shows benefits the case where many requests are merged in the same batch. We observed roughly 30 - 75% latency improvement.

Event Type Before (ns/merge) After (ns/merge) Diff percentage
Logs 3643.971 2513.947 -31%
Metrics 24550.572 5847.417 -76%
Traces 5172.710 3233.694 -37%

Link to tracking issue

Fixes #12137

Testing

This PR adds four benchmarks for each event type

  1. 1000 requests are merged into the same batch.
  2. every incoming request is slightly above the limit and will cause a split.
  3. the incoming request is split into 10 batches

Benchmark 1 is designed to test the performance of merging, while Benchmark 2 and 3 are primarily designed to test the performance of splitting. Benchmarks indicate that the optimization implemented in this PR improves merging performance.

Before:

BenchmarkSplittingBasedOnItemCountManySmallLogs-10                    	     316	   3643971 ns/op
BenchmarkSplittingBasedOnItemCountManyLogsSlightlyAboveLimit-10       	      43	  25643229 ns/op
BenchmarkSplittingBasedOnItemCountHugeLogs-10                         	      39	  27257268 ns/op
BenchmarkSplittingBasedOnItemCountManySmallMetrics-10                 	      43	  24783318 ns/op
BenchmarkSplittingBasedOnItemCountManyMetricsSlightlyAboveLimit-10    	      16	  67046630 ns/op
BenchmarkSplittingBasedOnItemCountHugeMetrics-10                      	      13	  81208615 ns/op
BenchmarkSplittingBasedOnItemCountManySmallTraces-10                  	     235	   5172710 ns/op
BenchmarkSplittingBasedOnItemCountManyTracesSlightlyAboveLimit-10     	      37	  31968305 ns/op
BenchmarkSplittingBasedOnItemCountHugeTraces-10                       	      30	  35512446 ns/op

After:

BenchmarkSplittingBasedOnItemCountManySmallLogs-10                    	     507	   2513947 ns/op
BenchmarkSplittingBasedOnItemCountManyLogsSlightlyAboveLimit-10       	      40	  26553098 ns/op
BenchmarkSplittingBasedOnItemCountHugeLogs-10                         	      38	  27769027 ns/op
BenchmarkSplittingBasedOnItemCountManySmallMetrics-10                 	     189	   5847417 ns/op
BenchmarkSplittingBasedOnItemCountManyMetricsSlightlyAboveLimit-10    	      16	  63132219 ns/op
BenchmarkSplittingBasedOnItemCountHugeMetrics-10                      	      14	  81363741 ns/op
BenchmarkSplittingBasedOnItemCountManySmallTraces-10                  	     367	   3253624 ns/op
BenchmarkSplittingBasedOnItemCountManyTracesSlightlyAboveLimit-10     	      38	  32017175 ns/op
BenchmarkSplittingBasedOnItemCountHugeTraces-10                       	      31	  34387539 ns/op

Documentation

@sfc-gh-sili sfc-gh-sili changed the title Cache item size [chore] Improve exporter.request merge splitting performance by caching items count Jan 21, 2025
@sfc-gh-sili sfc-gh-sili changed the title [chore] Improve exporter.request merge splitting performance by caching items count [chore][exporter][batcher] Improve exporter.request merge splitting performance by caching items count Jan 21, 2025
@sfc-gh-sili sfc-gh-sili force-pushed the sili-cache-item-size branch 3 times, most recently from d09845d to 2d054e5 Compare January 21, 2025 02:07
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.74%. Comparing base (9757ead) to head (06b8416).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12136      +/-   ##
==========================================
+ Coverage   91.73%   91.74%   +0.01%     
==========================================
  Files         463      463              
  Lines       24819    24852      +33     
==========================================
+ Hits        22767    22800      +33     
  Misses       1670     1670              
  Partials      382      382              

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

@sfc-gh-sili sfc-gh-sili marked this pull request as ready for review January 21, 2025 02:21
@sfc-gh-sili sfc-gh-sili requested review from bogdandrutu, dmitryax and a team as code owners January 21, 2025 02:21
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2025
…ad of &{logs,metrics,traces}Request{} (#12152)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

As part of the effort to cache request size to improve merge splitting
performance
(See
#12137
and
#12136),
we want to make sure requests are created from create function instead
of getting initialized via struct directly.

* `newLogsRequest` instead of `&logsRequest{}`
* `newMetrcsRequest` instead of `&metricsRequest{}`
* `newTracesRequest` instead of `&tracesRequest{}`



<!-- Issue number if applicable -->
#### Link to tracking issue
#12137

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->
@sfc-gh-sili sfc-gh-sili force-pushed the sili-cache-item-size branch 2 times, most recently from 003e29e to bc525ed Compare January 22, 2025 01:51
@bogdandrutu bogdandrutu added this pull request to the merge queue Jan 22, 2025
Merged via the queue into open-telemetry:main with commit 6a546b2 Jan 22, 2025
53 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 23, 2025
…ty sender (#12169)

During
#12136
discussion, it was noted that the count may not always be called, but
that is not true since we always call it in the observability sender.
This PR only simplifies the logic and ensure we always have the right
number.

Signed-off-by: Bogdan Drutu <[email protected]>
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.

[exporter][queuebatcher] Improve merge splitting performance
2 participants