fix: Stackdriver batch limit for TimeSeries#88
fix: Stackdriver batch limit for TimeSeries#88mayurkale22 merged 6 commits intoGoogleCloudPlatform:masterfrom
Conversation
|
Did you get a chance to verify these changes against real instance? Alss, please fix the lint. |
TigerHe7
left a comment
TigerHe7
left a comment
There was a problem hiding this comment.
Added some comments for clarity and for a question I had
| timeSeries, | ||
| MAX_BATCH_EXPORT_SIZE | ||
| )) { | ||
| await this._sendTimeSeries(batchedTimeSeries); |
There was a problem hiding this comment.
Added an await here for the async code in _sendTimeSeries to be properly called and spied on in tests.
I was wondering if there was a preferred testing practice so we don't have to wait here, or if it's ok to wait so we can handle if an error is thrown?
|
|
||
| /** | ||
| * Converts metric's timeseries to a list of TimeSeries, so that metric can be | ||
| * Converts metric's timeseries to a TimeSeries, so that metric can be |
There was a problem hiding this comment.
Updated the comment here since a single TimeSeries is being returned. I think the discrepancy was made when this code was copied over.
| metricDescriptors = sinon.spy( | ||
| /* tslint:disable no-any */ | ||
| (request: any, callback: (err: Error | null) => void): any => { | ||
| (request: any, params: any, callback: (err: Error | null) => void): any => { |
There was a problem hiding this comment.
See spied on code on line 187 in packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
+ Coverage 88.53% 92.74% +4.21%
==========================================
Files 9 10 +1
Lines 253 262 +9
Branches 43 43
==========================================
+ Hits 224 243 +19
+ Misses 29 19 -10
Continue to review full report at Codecov.
|
TigerHe7
changed the title
|
@mayurkale22 Did not get to testing against a real instance but will look into that now |
nlehrer
left a comment
nlehrer
left a comment
There was a problem hiding this comment.
Looks good to me. Just one comment about array mutation.
| import { TimeSeries } from './types'; | ||
|
|
||
| /** Returns an array with arrays of the given size. */ | ||
| export function partitionList(list: TimeSeries[], chunkSize: number) { |
There was a problem hiding this comment.
It would be better to copy the array first, since this will mutate the original array passed into the function. E.g.
const listCopy === [...list];
There was a problem hiding this comment.
Thanks for the feedback, updated!
|
|
||
| import { TimeSeries } from './types'; | ||
|
|
||
| /** Returns an array with arrays of the given size. */ |
There was a problem hiding this comment.
I think this could be more clear, like maybe "Returns the given array partitioned into arrays of the given size."
There was a problem hiding this comment.
Updated to "Returns the minimum number of arrays of max size chunkSize, partitioned from the given array"
3 participants
Description:
Smaller changes:
transform.tsmetricDescriptorsspyPrevious implementation in OpenCensus:
census-instrumentation/opencensus-node#644
Fixes #87