Skip to content

Conversation

@JacksonWeber
Copy link
Contributor

Which problem is this PR solving?

This pull request addresses a bug in the log batching processor so that logs are exported immediately when the batch size limit is reached, rather than waiting for the scheduled delay. It also adds and improves tests to ensure the correct export behavior for both full and partial batches, and clarifies the logic for queue size limits.

Bug Fixes and Core Logic:

  • Fixed the batchLogProcessor so that it exports logs immediately when maxExportBatchSize is reached, instead of waiting for _scheduledDelayMillis. This ensures timely log delivery and corrects previous batching behavior. [1] [2] [3]

Fixes #5706

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Updated and expanded tests in BatchLogRecordProcessor.test.ts to cover immediate export on batch size, timer-based export for partial batches, multiple immediate exports, and correct handling of queue size limits. [1] [2]
  • Improved test clarity by removing unnecessary fake timers where not needed, and by verifying the processor's behavior in a variety of batching and flushing scenarios. [1] [2]

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@JacksonWeber JacksonWeber requested a review from a team as a code owner September 25, 2025 20:06
@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.22%. Comparing base (af28dba) to head (82dd756).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...sdk-logs/src/export/BatchLogRecordProcessorBase.ts 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5961      +/-   ##
==========================================
- Coverage   95.23%   95.22%   -0.01%     
==========================================
  Files         311      311              
  Lines        8596     8603       +7     
  Branches     1799     1801       +2     
==========================================
+ Hits         8186     8192       +6     
- Misses        410      411       +1     
Files with missing lines Coverage Δ
...sdk-logs/src/export/BatchLogRecordProcessorBase.ts 93.06% <90.00%> (-0.55%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@trentm trentm 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 taking this on. We definitely should fix the issue.

@trentm
Copy link
Contributor

trentm commented Oct 1, 2025

Marc reminded me that this exporting code originally derives from the batch Span exporter. And there was a PR there doing a similar thing. I haven't looked at it, but this PR might have changes that could be borrowed: #3958

@JacksonWeber JacksonWeber requested a review from trentm October 1, 2025 21:11
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks!

@trentm trentm changed the title [sdk-logs] Fix Not Exporting Upon _maxExportBatchSize fix(sdk-logs): Fix Not Exporting Upon _maxExportBatchSize Oct 9, 2025
CHANGELOG.md Outdated
### :bug: Bug Fixes

* fix(sdk-logs): Fix the `batchLogProcessor` exporting only upon `_scheduledDelayMillis` and ignoring `maxExportBatchSize` [#5961](https://github.com/open-telemetry/opentelemetry-js/pull/5961) @jacksonweber
* test(shim-opentracing): add comparison thresholds in flaky assertions [#5974](https://github.com/open-telemetry/opentelemetry-js/pull/5974) @cjihrig
Copy link
Contributor

@trentm trentm Oct 9, 2025

Choose a reason for hiding this comment

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

This line shouldn't have been moved. I think this is an accidental mismerge from #5987.
The changelog line for this PR should also be in experimental/CHANGELOG.md rather than this file.

I'll fix in a sec.

@trentm trentm enabled auto-merge October 9, 2025 19:43
@trentm trentm added this pull request to the merge queue Oct 9, 2025
Merged via the queue into open-telemetry:main with commit 46b7d15 Oct 9, 2025
25 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.

BatchLogRecordProcessor not sending records when queue hits maxExportBatchSize

3 participants