Add support for batching in PeriodicMetricReader#8296
Add support for batching in PeriodicMetricReader#8296psx95 wants to merge 53 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8296 +/- ##
============================================
+ Coverage 90.27% 90.31% +0.03%
- Complexity 7693 7743 +50
============================================
Files 850 851 +1
Lines 23207 23398 +191
Branches 2356 2376 +20
============================================
+ Hits 20951 21132 +181
- Misses 1530 1537 +7
- Partials 726 729 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Collection<MetricData> currentBatch = new ArrayList<>(maxExportBatchSize); | ||
|
|
||
| // Fill active batch and split overlapping metric points if needed | ||
| for (MetricData metricData : metrics) { |
There was a problem hiding this comment.
nit: i expected prepareExportBatches to take the Collection so that all of the batching logic is contained within the batcher (and we don't have to pass + update currentBatch).
There was a problem hiding this comment.
But we are not technically updating the currentBatch in this loop - the contents within the currentBatch is updated by the prepareExportBatches.
My intention behind the prepareExportBatches was a function that splits a given MetricData into appropriate export batches. Since the batches need to be completely filled (except the last one), this function requires the currentBatch as well.
Maybe if the function was renamed to splitMetricData it would make more sense?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
# Conflicts: # sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExportBatcher.java
…ropagation' into lane-a-pr-8296 # Conflicts: # sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…propagation Fix batched forceFlush failure and make batching linear
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Couple of comments, but looks pretty good!
This PR adds support for configuring
setMaxExportBatchSizeon thePeriodicMetricReaderthat sets the maximum number of metric data points to be sent in a single export call.If the number of metric data points (across
Collection<MetricData>) scheduled to be export exceeds themaxExportBatchSize, the data points organized in batches and sent over multiple export calls.These changes do not modify the current timeout behavior, which is still per export call.
Fix #8245