Skip to content

Avoid OtapPdata::num_items in batch_processor#2885

Merged
jmacd merged 11 commits intoopen-telemetry:mainfrom
jmacd:jmacd/num_items
May 8, 2026
Merged

Avoid OtapPdata::num_items in batch_processor#2885
jmacd merged 11 commits intoopen-telemetry:mainfrom
jmacd:jmacd/num_items

Conversation

@jmacd
Copy link
Copy Markdown
Contributor

@jmacd jmacd commented May 7, 2026

Change Summary

Fixes #2882. Uses the sizer's natural weight which is O(1) for batch processor bookkeeping. OTLP batching pays a penalty otherwise, due to an (not strictly necessary) allocation in addition to the traversal.

@jmacd jmacd requested a review from a team as a code owner May 7, 2026 02:35
@github-actions github-actions Bot added the rust Pull requests that update Rust code label May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 98.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.18%. Comparing base (3c03f86) to head (bdc0559).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2885    +/-   ##
========================================
  Coverage   86.17%   86.18%            
========================================
  Files         714      714            
  Lines      270290   270546   +256     
========================================
+ Hits       232929   233168   +239     
- Misses      36837    36854    +17     
  Partials      524      524            
Components Coverage Δ
otap-dataflow 87.13% <98.33%> (+<0.01%) ⬆️
query_abstraction 80.61% <ø> (ø)
query_engine 90.73% <ø> (ø)
otel-arrow-go 52.45% <ø> (ø)
quiver 92.25% <ø> (ø)
🚀 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
Copy Markdown
Contributor

@JakeDern JakeDern left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

Comment thread rust/otap-dataflow/crates/core-nodes/src/processors/batch_processor/mod.rs Outdated
Comment thread rust/otap-dataflow/crates/core-nodes/src/processors/batch_processor/mod.rs Outdated
Comment thread rust/otap-dataflow/crates/core-nodes/src/processors/batch_processor/mod.rs Outdated
@drewrelmas
Copy link
Copy Markdown
Contributor

In general, I think the intention behind this change is related to issue #2859. If the num_items() is an expensive operation, we should:

  1. Try to optimize it as best we can
  2. (More importantly) only do signal count reporting at specific engine-level config-requested points and discourage individual components from calling it.

@jmacd
Copy link
Copy Markdown
Contributor Author

jmacd commented May 7, 2026

@drewrelmas I filed #2884 to talk about the same topic -- I would not add items/bytes counters to the flow measurement/stopwatch thing, I would add them to individual nodes, allow us to enable on a node-by-node basis at detailed metric level.

Copy link
Copy Markdown
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM

@lquerel
Copy link
Copy Markdown
Contributor

lquerel commented May 7, 2026

@drewrelmas Determining the number of signals/items is only costly for pdata with pure OTLP protobytes backend. For regular OTAP based pdata the cost is probably negligeable.

@jmacd jmacd enabled auto-merge May 8, 2026 00:49
@jmacd jmacd added this pull request to the merge queue May 8, 2026
jmacd added a commit to jmacd/otel-arrow that referenced this pull request May 8, 2026
…metry#2885

The batch_processor num_items/weight refactor and the matching
liveness-test assertion update were extracted into upstream PR
open-telemetry#2885. Restore both files to upstream/main
state on this branch so the changes flow back in via the upstream
merge rather than living in two places.

Files restored to upstream/main:
- crates/core-nodes/src/processors/batch_processor/mod.rs
- crates/otap/tests/core_node_liveness_tests.rs

Verified: cargo build (profiling, df_engine + obsday_logger) clean;
batch_processor unit tests 42/42 pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merged via the queue into open-telemetry:main with commit c19ac24 May 8, 2026
84 of 85 checks passed
@jmacd jmacd deleted the jmacd/num_items branch May 8, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Avoid using num_items for batch_processor bookkeeping

6 participants