math-dojo / user-account-service

Microservice for managing the users of the math-dojo platform
1 stars 0 forks source link

Enable Retrieval of Existing Users based on userid #44

Closed aakano684 closed 3 years ago

aakano684 commented 3 years ago

⚠️⚠️ ⚠️ All PRs must have a version of the format pr${number} and an override target url set up on the user-accout-test api ⚠️ ⚠️ ⚠️

Purpose

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

What to Check

Verify that the following are valid

Other Information

noce2 commented 3 years ago

In terms of why a timeout in the request stage isn't the right approach:

The timeout essentially says, "wait until this window has passed before failing the test if the promise hasn't resolved." Here are the docs if you're curious.

Currently in the tests we have timeouts in 2 places. The first is on the startup of the app, in hooks.js, that one has to be there because it's a long running task and we want to "bail out" if for any reason it takes too long to do start.

The second timeout is for the step where we assert the status codes of responses. That one is necessary because the Azure function when deployed has a cold start period (the container in which our code runs has been de-provisioned and so Azure needs to "wake up the app").

Given you're trying to throttle requests, the timeout won't do what you're hoping. It's easy to modify the rate limit, just log onto the API gateway and see what it is for the API in question.

aakano684 commented 3 years ago

In terms of why a timeout in the request stage isn't the right approach:

The timeout essentially says, "wait until this window has passed before failing the test if the promise hasn't resolved." Here are the docs if you're curious.

Currently in the tests we have timeouts in 2 places. The first is on the startup of the app, in hooks.js, that one has to be there because it's a long running task and we want to "bail out" if for any reason it takes too long to do start.

The second timeout is for the step where we assert the status codes of responses. That one is necessary because the Azure function when deployed has a cold start period (the container in which our code runs has been de-provisioned and so Azure needs to "wake up the app").

Given you're trying to throttle requests, the timeout won't do what you're hoping. It's easy to modify the rate limit, just log onto the API gateway and see what it is for the API in question.

Sorry I still don't see what long or short term harm it does. The linked docs say nothing about what you mentioned. And it seemed to be a convenient way to add a short wait before we send http requests. Your proposed solution of changing the rate on the gateway doesn't seem to scale