Skip to content

fix: [Geneva Exporter] Add LZ4 compression benchmark #284

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

Merged
merged 10 commits into from
Jun 30, 2025

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jun 4, 2025

Changes

  • Added benchmark test

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team as a code owner June 4, 2025 04:43
@lalitb lalitb changed the title [Geneva Exporter] Optimize lz4 compression, and add benchmark fix: [Geneva Exporter] Optimize lz4 compression, and add benchmark Jun 4, 2025
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

Attention: Patch coverage is 21.73913% with 18 lines in your changes missing coverage. Please review.

Project coverage is 53.0%. Comparing base (05221c0) to head (91f81be).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...metry-exporter-geneva/geneva-uploader/src/bench.rs 0.0% 18 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #284     +/-   ##
=======================================
- Coverage   53.0%   53.0%   -0.1%     
=======================================
  Files         63      64      +1     
  Lines       8288    8311     +23     
=======================================
+ Hits        4400    4405      +5     
- Misses      3888    3906     +18     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@utpilla
Copy link
Contributor

utpilla commented Jun 9, 2025

@lalitb We want the tests related to the changes in the PR to be run in the CI. Could we update the workspace to include these projects first?

# "opentelemetry-exporter-geneva/geneva-uploader",
# "opentelemetry-exporter-geneva/geneva-uploader-ffi",
# "opentelemetry-exporter-geneva/opentelemetry-exporter-geneva",

#[test]
fn lz4_benchmark() {
let mut criterion = Criterion::default()
.sample_size(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason to be changing the default sample size and measurement time?

Copy link
Member Author

Choose a reason for hiding this comment

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

The benchmark was not completing in default time, so the sample size was changed. But not getting the issue anymore, so removed this.

@lalitb
Copy link
Member Author

lalitb commented Jun 9, 2025

Could we update the workspace to include these projects first?

Sure.

let max_chunk_compressed = get_maximum_output_size(CHUNK_SIZE);
let num_chunks = input.len().div_ceil(CHUNK_SIZE);
let mut output = Vec::with_capacity(num_chunks * (4 + max_chunk_compressed));
let mut temp_buf = vec![0u8; max_chunk_compressed];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow. We had removed all temporary buffer allocation. Why are we introducing it again?

Copy link
Member Author

@lalitb lalitb Jun 11, 2025

Choose a reason for hiding this comment

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

Good point. This is not correct. I see improvements in the small size with this, but not with large size. Have reverted the optimization efforts - the PR is now limited to adding the benchmark! Thanks for catching this.

return Ok(Vec::new());
}
// Special case: very small data (less than one chunk)
if input.len() <= CHUNK_SIZE {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the PR description:

Added a fast path for small inputs (≤ one chunk): compresses the whole input in a single step, avoiding the main loop and extra allocations.

It doesn't look like there is anything special happening for this case. It would have been same (cost-wise) even if you didn't have a special case for this. The while loop would have only run once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree.

@lalitb lalitb changed the title fix: [Geneva Exporter] Optimize lz4 compression, and add benchmark fix: [Geneva Exporter] Add LZ4 compression benchmark Jun 11, 2025
| 1 byte | ~533 ns | ~1.6 MiB/s |
| 1 KiB | ~597 ns | ~1.5 GiB/s |
| 1 MiB | ~42 us | ~22.1 GiB/s |
- No significant regressions or improvements detected.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think you could remove this line.

|-------------|--------------------|--------------------|
| 1 byte | ~533 ns | ~1.6 MiB/s |
| 1 KiB | ~597 ns | ~1.5 GiB/s |
| 1 MiB | ~42 us | ~22.1 GiB/s |
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to identify what size would better represent the actual scenario.

let mut group = criterion.benchmark_group("lz4_compression");

for size in [1, 1024, 1024 * 1024] {
let data = setup_test_data(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider adding a benchmark for OTLP data as well.

@lalitb lalitb merged commit 476ecb6 into open-telemetry:main Jun 30, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants