-
Notifications
You must be signed in to change notification settings - Fork 867
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
fix(analysis): explicitly set datadog aggregator to last only on v2 #3730
Conversation
the field is required by API and we currently leave it empty string Signed-off-by: Alex Eftimie <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3730 +/- ##
==========================================
+ Coverage 80.27% 80.29% +0.02%
==========================================
Files 156 156
Lines 17964 18023 +59
==========================================
+ Hits 14420 14472 +52
- Misses 2631 2637 +6
- Partials 913 914 +1 ☔ View full report in Codecov by Sentry. |
Go Published Test Results2 171 tests 2 171 ✅ 2m 54s ⏱️ Results for commit c9a5326. ♻️ This comment has been updated with latest results. |
Signed-off-by: Alex Eftimie <[email protected]>
E2E Tests Published Test Results 4 files 4 suites 3h 23m 42s ⏱️ Results for commit c9a5326. |
@alexef I also experience it , is there a way to workaround it and still using Datadog V2 ? |
@mickeyrouash yes, the workaround is adding |
…rgoproj#3730) * Datadog: explicitly set aggregator to last the field is required by API and we currently leave it empty string Signed-off-by: Alex Eftimie <[email protected]> * add unit tests Signed-off-by: Alex Eftimie <[email protected]> --------- Signed-off-by: Alex Eftimie <[email protected]>
…3730) * Datadog: explicitly set aggregator to last the field is required by API and we currently leave it empty string Signed-off-by: Alex Eftimie <[email protected]> * add unit tests Signed-off-by: Alex Eftimie <[email protected]> --------- Signed-off-by: Alex Eftimie <[email protected]>
Is this PR addressing #3707 ? |
@aguiraorodriguez exactly, that issue should be fixed by this PR, as part of 1.7.2 release |
the field is required by the v2 API and we currently leave it empty string.
@zachaller you removed it in #3643 and that is fine. this PR will not add it back to the schema and not break the v1 API.
without this change and without aggregator set on a v2 metric, one gets:
If we set it to None, according to datadog docs it will default to
avg
. According to rollouts docs, we should default tolast
(which makes sense for analysis)https://docs.datadoghq.com/api/latest/metrics/#query-scalar-data-across-multiple-products
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.