-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat(metrics): rename method purgeStoredMetrics to publishStoredMetrics #377
Conversation
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.
LGTM
@@ -2,9 +2,9 @@ | |||
// SPDX-License-Identifier: MIT-0 | |||
|
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.
A following PR will focus on refactoring e2e test to reuse tracing one
) | ||
.promise(); | ||
|
||
// Despite lambda has been called twice, coldstart metric sum should only be 1 | ||
const singleDataPoint = coldStartMetricStat.Datapoints ? coldStartMetricStat.Datapoints[0] : {}; | ||
expect(singleDataPoint.Sum).toBe(1); | ||
}, 9000000); | ||
}); | ||
}, 15000); |
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.
(minor) timeout is in ms. Will 15 sec be sufficient?
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.
Yes the test itself is quite fast. but since we sleep for 10s to get metrics ingested it still takes around 11sec
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.
One minor point to check. But this looks good enough to merge.
Description of your changes
purgeStoredMetrics is confusing, renaming to publishStoredMetrics.
How to verify this change
Related issues, RFCs
#301
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.