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 custom ring implementation to the BatchProcessor #5237

Merged
merged 10 commits into from
Apr 24, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Apr 19, 2024

Part of #5063

The current queue implementation uses the built-in container/ring package for a ring implementation. This implementation stores its value as an any. Each time the batch processor stores a Record to this field it needs to be allocated to the heap to convert it to an any type.

This changes the implementation to be a paired-down ring with a Record as the value field. This removes the allocation and greatly improves the performance of the batch processor.

Benchmarks

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/log
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                       │   old.txt    │               new.txt               │
                       │    sec/op    │   sec/op     vs base                │
BatchProcessorOnEmit-8   312.3n ± 18%   118.0n ± 5%  -62.22% (p=0.000 n=10)

                       │    old.txt    │                new.txt                │
                       │      B/s      │     B/s       vs base                 │
BatchProcessorOnEmit-8   1.264Gi ± 15%   3.347Gi ± 5%  +164.69% (p=0.000 n=10)

                       │  old.txt   │              new.txt              │
                       │    B/op    │   B/op    vs base                 │
BatchProcessorOnEmit-8   416.0 ± 0%   0.0 ± 0%  -100.00% (p=0.000 n=10)

                       │  old.txt   │               new.txt               │
                       │ allocs/op  │ allocs/op   vs base                 │
BatchProcessorOnEmit-8   1.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:logs Part of OpenTelemetry logs Skip Changelog PRs that do not require a CHANGELOG.md entry labels Apr 19, 2024
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (baeb560) to head (12d43fd).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5237   +/-   ##
=====================================
  Coverage   84.5%   84.6%           
=====================================
  Files        258     259    +1     
  Lines      17349   17384   +35     
=====================================
+ Hits       14676   14708   +32     
- Misses      2362    2365    +3     
  Partials     311     311           
Files Coverage Δ
sdk/log/batch.go 99.4% <100.0%> (ø)
sdk/log/ring.go 100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

@MrAlias MrAlias force-pushed the blp-ring branch 3 times, most recently from 59c88db to 36edfb3 Compare April 19, 2024 17:03
sdk/log/batch.go Outdated Show resolved Hide resolved
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.

Just one minor comment that it would be nice to address.

@pellared pellared merged commit 29e1c7e into open-telemetry:main Apr 24, 2024
27 checks passed
@MrAlias MrAlias deleted the blp-ring branch April 24, 2024 14:19
@XSAM XSAM added this to the Old Untracked PRs milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs pkg:SDK Related to an SDK package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants