-
Notifications
You must be signed in to change notification settings - Fork 47
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
10432: Performance Measurement - TO TEST #5254
Conversation
…quence takes to run; Add a minimum amount of time to console log the results;
…s and store in an array in props;
…ster client in application context;
…ion for sequences.
…list of sequence to log performance;
.saveSystemPerformanceData.mockImplementation(() => {}); | ||
}); | ||
|
||
it('should call persistence method with correct data', async () => { |
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.
Perhaps it('should not allow unauthorized call to log data')
).should('be.visible'); | ||
|
||
retry(() => { | ||
cy.reload(true); |
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.
Why the reload? (Just making sure I understand!)
@@ -46,6 +46,7 @@ IRS_CLIENT_ID=$(aws cognito-idp list-user-pool-clients \ | |||
echo "DAWSON_ENV='${ENV}'" | |||
echo "DYNAMODB_TABLE_NAME='${SOURCE_TABLE}'" | |||
echo "ELASTICSEARCH_ENDPOINT='${ELASTICSEARCH_ENDPOINT}'" | |||
echo "ELASTICSEARCH_INFO_ENDPOINT='${ELASTICSEARCH_INFO_ENDPOINT}'" |
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.
Nitpick, and probably not worth doing if it involves changing infrastructure, but I wonder if we should give this a clearer name? Info
can mean just about anything.
* logging | ||
*/ | ||
{ | ||
app.post( |
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.
Could this contribute to high server load if we end up logging a lot?
sequences: presenterSequences, | ||
sequences: Object.entries(presenterSequences).reduce( | ||
(acc, [sequenceName, sequence]) => { | ||
const SYSTEM_PERFORMANCE_BLACKLIST: string[] = [ |
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.
👍
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 is super cool! I left a few comments. I would want to understand things a little more clearly before merging to a non-test environment (mostly around server load implications and making sure nested sequences are giving accurate data EDIT: Nevermind! I thought we might be logging something wrong, but it turns out the logging caught something that was wrong!), but I think it's good to test.
This PR might not get merged.
It is working but using CloudWatch Metrics will be too expensive.