Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(sdk-metrics): add maxExportBatchSize option to PeriodicExportingMetricReader #6655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
feat(sdk-metrics): add maxExportBatchSize option to PeriodicExportingMetricReader #6655
Changes from all commits
8979c03bbbd01dab9f04c8069cbab0f912279a0cad37304bd76d13ea6aaa163b9835f8ea31ad686483016424d49c935ad0d4180be1cf992a5158764File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The related part of the spec states:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, I missed the documentation in
callWithTimeoutthat states:If this is the case, I can modify the loop in
_doRunto stop processing further batches if aTimeoutErroroccurs. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the existing code already have this issue if there was a previous export still running?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 previousawait callWithTimeout(this._doRun(), this._exportTimeout)is still being run.Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.