feat(sdk-metrics): add maxExportBatchSize option to PeriodicExportingMetricReader#6655
feat(sdk-metrics): add maxExportBatchSize option to PeriodicExportingMetricReader#6655psx95 wants to merge 17 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6655 +/- ##
==========================================
+ Coverage 94.83% 94.85% +0.02%
==========================================
Files 376 377 +1
Lines 12711 12781 +70
Branches 2888 2904 +16
==========================================
+ Hits 12054 12124 +70
Misses 657 657
🚀 New features to boost your workflow:
|
127cbe1 to
566dc6a
Compare
| for (const batch of batches) { | ||
| try { | ||
| const result = await callWithTimeout( |
There was a problem hiding this comment.
Does the spec say if the batches must run serially in order or are they allowed to execute the batches in parallel?
There was a problem hiding this comment.
The related part of the spec states:
The reader MUST ensure all metric data points from a single Collect() are provided to Export before metric data points from a subsequent Collect() so that metric points are sent in-order.
So the points need to be sent in-order, would sending batches out-of-order (in case of parallel export) cause the points to be sent out-of-order?
There was a problem hiding this comment.
Ah that's right. It makes sense but it's unfortunate because a single collect call would only ever return one point per timeseries, so you know there is no risk of out-of-order points.
There was a problem hiding this comment.
Pull request overview
Adds support for the OpenTelemetry spec’s maxExportBatchSize option to the Metrics SDK’s PeriodicExportingMetricReader, enabling large Collect() results to be split into smaller export batches and exported sequentially.
Changes:
- Added
maxExportBatchSizetoPeriodicExportingMetricReaderOptionswith constructor validation and export-path integration. - Introduced
splitMetricData()utility to splitResourceMetricsinto multiple batches based on total data point count. - Expanded unit test coverage for batching/splitting behavior, export synchronization, and timeout handling; added a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts | Adds maxExportBatchSize, uses splitMetricData(), and serializes export calls. |
| packages/sdk-metrics/src/export/MetricDataSplitter.ts | New helper to split ResourceMetrics into multiple ResourceMetrics export batches. |
| packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts | Adds reader tests for validation, batching, export synchronization, and timeout logging behavior. |
| packages/sdk-metrics/test/export/MetricDataSplitter.test.ts | New comprehensive unit tests for splitMetricData() across metric types and scopes. |
| CHANGELOG.md | Documents the new feature in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const batch of batches) { | ||
| try { | ||
| const result = await callWithTimeout( | ||
| internal._export(this._exporter, batch), | ||
| this._exportTimeout | ||
| ); | ||
| if (result.code !== ExportResultCode.SUCCESS) { | ||
| const err = new Error( | ||
| `PeriodicExportingMetricReader: metrics export failed (error ${result.error})` | ||
| ); | ||
| api.diag.error(err.message); | ||
| anyErr = err; | ||
| } | ||
| } catch (e) { | ||
| if (e instanceof TimeoutError) { | ||
| // We do not report TimeoutError to the globalErrorHandler in _runOnce(). | ||
| api.diag.error( | ||
| `PeriodicExportingMetricReader: metrics export timed out after ${this._exportTimeout}ms` | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
This makes sense, I missed the documentation in callWithTimeout that states:
NOTE: this operation will continue even after it throws a {@link TimeoutError}.
If this is the case, I can modify the loop in _doRun to stop processing further batches if a TimeoutError occurs. This prevents starting new exports while the previous one might still be active in the background.
Important
Breaking the loop on timeout means that if one batch fails due to timeout, subsequent batches in the same export cycle will be skipped. This prioritizes safety (no concurrent exports) over data delivery (trying to send remaining batches)
There was a problem hiding this comment.
Did the existing code already have this issue if there was a previous export still running?
There was a problem hiding this comment.
The existing code also had the same issue (concurrent exports).
I don't see any check before invoking _runOnce() that would verify if the operation in the runOnce function (i.e., export) is still working.
This means that _runOnce() could technically be invoked again while the underlying export operation from previous await callWithTimeout(this._doRun(), this._exportTimeout) is still being run.
| }; | ||
| /** | ||
| * The maximum batch size for exports. If configured, the reader will split | ||
| * batches larger than this size into smaller batches. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The incorrect merge happened because metrics were only being grouped using descriptor.name and the SDK can produce multiple incompatible storages per name.
Which problem is this PR solving?
This PR implements the
maxExportBatchSizeoption for thePeriodicExportingMetricReaderin the Metrics SDK, as required by the OpenTelemetry specification. This option allows users to limit the size of batches sent to the exporter, preventing issues with large payloads. It ensures that batches are split correctly without combining data from differentCollect()calls, maintains order, and synchronizes calls toExportto prevent concurrency.Fixes #6641
Short description of the changes
maxExportBatchSizeoption toPeriodicExportingMetricReaderOptions.maxExportBatchSizeinPeriodicExportingMetricReaderconstructor to ensure it is greater than 0.MetricDataSplitterclass to handle the logic of splittingResourceMetricsinto smaller batches based on data point count.PeriodicExportingMetricReaderto useMetricDataSplitterand handle sequential exporting of split batches.PeriodicExportingMetricReader.test.tsfor synchronization, timeout behavior, and failure handling.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: