Documenting memory usage/management by BatchLogProcessor #2469

lalitb · 2024-12-24T14:20:19Z

Changes

Felt it would be good to document it for better understanding of the memory management by BatchLogProcessor. This is not the doc-comment so won't go the docs.rs. If there is better place to add this, please suggest.

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)

codecov · 2024-12-24T14:24:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.5%. Comparing base (b3879b6) to head (24797bb).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2469     +/-   ##
=======================================
+ Coverage   77.8%   79.5%   +1.6%     
=======================================
  Files        122     118      -4     
  Lines      23061   22526    -535     
=======================================
- Hits       17956   17909     -47     
+ Misses      5105    4617    -488

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

cijothomas · 2024-12-24T17:34:31Z

opentelemetry-sdk/src/logs/log_processor.rs

@@ -167,6 +167,72 @@ enum BatchMessage {

 /// A [`LogProcessor`] that buffers log records and reports
 /// them at a pre-configured interval from a dedicated background thread.
+// **Memory Management in BatchLogProcessor**


a lot of these can be moved off to docs.rs itself, as they are good for users to be aware of.

cijothomas

The contents are very good. Lets plan to move them to user-facing docs before stable release.

utpilla · 2024-12-27T23:47:30Z

opentelemetry-sdk/src/logs/log_processor.rs

+// 1. **Record Ingestion**:
+//    - Each `LogRecord` is **cloned** upon entering the processor.
+//    - `LogRecordAttributes` utilize a hybrid memory model:
+//      - Attributes up to `PREALLOCATED_ATTRIBUTE_CAPACITY` are **stack-allocated**.


Since PREALLOCATED_ATTRIBUTE_CAPACITY is not a public const, lets write the exact number of attributes here. I would suggest something like this:

- The first 5 attributes are allocated on the stack. - Any additional attributes are allocated on the heap in a dynamically growing vector.

opentelemetry-sdk/src/logs/log_processor.rs

utpilla · 2024-12-28T00:06:36Z

opentelemetry-sdk/src/logs/log_processor.rs

+//
+/// 5. **Memory Limits**:
+///    - **Worst-Case Memory Usage**:
+///      - **Queue Memory** = `max_queue_size * size of boxed (LogRecord + InstrumentationScope)`.


It'd be nice to mention the actual size of LogRecord and InstrumentationScope in bytes to get a better idea of the memory consumption.

Good point, have added some numbers of memory usage in worst case scenario, with average size of LogRecord and InstrumentationScope.

utpilla · 2024-12-28T00:18:11Z

opentelemetry-sdk/src/logs/log_processor.rs

+//          are moved into this temporary vector using `logs.split_off(0)`.
+//        - Ownership of the boxed records is transferred to the new vector, ensuring efficient
+//          memory usage without additional cloning.
+//    - The temporary vector is then used to construct references passed to the exporter via `LogBatch`.


It'd be good to mention that there two temporary vectors created during each export:

One vector is created by split_off(0) (we could avoid this if we pursue Avoid vec allocation during each export for BatchLogProcessor #2483)

The second vector is created by collecting references from the first vector mentioned above.

I think we should be able to avoid the second vector creation as well by updating LogBatch constructors.

I created #2488 to showcase one approach to avoid the vec allocation mentioned in the second point.

Have updated the document as per the latest optimizations, where both these vectors usage is removed.

lalitb · 2025-01-26T09:17:55Z

The contents are very good. Lets plan to move them to user-facing docs before stable release.

Have moved them as doc comment.

cijothomas · 2025-01-27T17:59:35Z

opentelemetry-sdk/src/logs/log_processor.rs

+///
+/// 1. **Record Ingestion**:
+///    - Each `LogRecord` is **cloned** upon entering the processor.
+///    - `LogRecordAttributes` utilize a hybrid memory model:


How attributes are stored inside LogRecord is not a concern of BatchLogProcessor. It can be documented (and should be!), but not in processor doc.

cijothomas · 2025-01-27T18:03:02Z

opentelemetry-sdk/src/logs/log_processor.rs

+///      to allocate them on the heap before entering the queue. This means:
+///      - The `LogRecord`'s inline attributes (if any) are moved to the heap as part of the boxed structure.
+///      - Any dynamically allocated data already on the heap (e.g., strings, overflow attributes) remains unaffected.
+///      - Ownership of the boxed data is transferred to the queue, ensuring it can be processed independently of the original objects.


Not sure I follow this.. The data inside box is already a clone, so it is already independent of originals right?

cijothomas · 2025-01-27T18:04:55Z

opentelemetry-sdk/src/logs/log_processor.rs

+///    - **Control Message Queue**:
+///      - Stores control messages (`BatchMessage`) to manage operations like exporting, force flushing, setting resources, and shutting down.
+///      - The control message queue has a fixed size (e.g., 64 messages).
+///      - Control messages are processed with higher priority, ensuring operational commands are handled promptly.


is there anything we do to process them with higher priority? This does not match the implementation.

cijothomas · 2025-01-27T18:05:43Z

opentelemetry-sdk/src/logs/log_processor.rs

+///      - Messages supported include:
+///        - `ExportLog`: Triggers an immediate export of log records.
+///        - `ForceFlush`: Flushes all buffered log records to the exporter.
+///        - `SetResource`: Updates the exporter with a new resource.


this is misleading. There is no ability to update a resource. This could be misinterpreted as Resource can be changed and processor will be informed of the same, which is not true.

cijothomas · 2025-01-27T18:06:42Z

opentelemetry-sdk/src/logs/log_processor.rs

+///      - The vector’s capacity is fixed at `max_export_batch_size`.
+///    - Records are **moved** (not cloned) from the log record queue to the vector for processing.
+///
+/// 4. **Export Process**:


this looks too much details for end users to care about!

cijothomas · 2025-01-27T18:08:08Z

opentelemetry-sdk/src/logs/log_processor.rs

 ///
+/// 7. **Control Queue Prioritization**:
+///    - Control messages take precedence over log record processing to ensure timely execution of critical operations.


This does not look like how BatchProcessor works.

lalitb added 2 commits December 24, 2024 06:06

initial commit

6c84650

more changes

1b5f52b

lalitb requested a review from a team as a code owner December 24, 2024 14:20

fix lint

76973f4

cijothomas reviewed Dec 24, 2024

View reviewed changes

cijothomas approved these changes Dec 24, 2024

View reviewed changes

utpilla reviewed Dec 27, 2024

View reviewed changes

utpilla reviewed Dec 28, 2024

View reviewed changes

opentelemetry-sdk/src/logs/log_processor.rs Outdated Show resolved Hide resolved

utpilla reviewed Dec 28, 2024

View reviewed changes

lalitb and others added 5 commits January 26, 2025 00:16

Merge branch 'main' into memory-consideration-batch-processor

67a1b32

update doc comment

ba36c8f

add rough size

403a412

move it to doc comment

1282bfc

doc ci

24797bb

cijothomas reviewed Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documenting memory usage/management by BatchLogProcessor #2469

Documenting memory usage/management by BatchLogProcessor #2469

lalitb commented Dec 24, 2024

codecov bot commented Dec 24, 2024 •

edited

Loading

cijothomas Dec 24, 2024

cijothomas left a comment

utpilla Dec 27, 2024

lalitb Jan 26, 2025

utpilla Dec 28, 2024

lalitb Jan 26, 2025

utpilla Dec 28, 2024

utpilla Dec 30, 2024

lalitb Jan 26, 2025

lalitb commented Jan 26, 2025

cijothomas Jan 27, 2025

cijothomas Jan 27, 2025

cijothomas Jan 27, 2025

cijothomas Jan 27, 2025

cijothomas Jan 27, 2025

cijothomas Jan 27, 2025

Documenting memory usage/management by BatchLogProcessor #2469

Are you sure you want to change the base?

Documenting memory usage/management by BatchLogProcessor #2469

Conversation

lalitb commented Dec 24, 2024

Changes

Merge requirement checklist

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Jan 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 24, 2024 •

edited

Loading