-
Notifications
You must be signed in to change notification settings - Fork 104
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
Use batches to call CreateTimeSeries #617
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hi @knightpp, thanks for opening this, just need to you to sign the CLA (see the bot comment) |
Hi, I've signed the CLA. There's a checkmark on the PR. Thanks. |
Ah sorry, I see it now. |
Codecov Report
@@ Coverage Diff @@
## main #617 +/- ##
==========================================
+ Coverage 63.27% 63.44% +0.17%
==========================================
Files 38 38
Lines 4446 4423 -23
==========================================
- Hits 2813 2806 -7
+ Misses 1508 1491 -17
- Partials 125 126 +1
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
looks good, one nit and can you add a test too? The test might be easier if you make batchSize
a configurable (private) field on the exporter, eg so you can run a test with a max batch size of like 1 or 2 instead of having to create 200+ test metrics
/gcbrun |
Added tests and refactored |
/gcbrun |
* Use batches for call to CreateTimeSeries * Refactor exportTimeSeries * Add tests
Based on #575 I've updated sending logic to batch time series data. The argument is hardcoded, and I couldn't find mocks for
*monitoring.MetricClient
so no tests.