Add clarity to MVI timeouts #2410
Conversation
There was code that looked like it intended to set the open_timeout to 2 and the read_timeout to 10 for MVI, but it didn't actually do that. The default of 15 was used all along for both. Update the code to reflect that reality. I don't believe that we want to reduce the timeout because we regularly see timeout errors.
|
note to self: rerun tests after #2409 is merged |
|
@annaswims could you link to devops settings for these in Prod/Staging/Dev? I think we will need a corresponding devops PR to change these. |
| end | ||
|
|
||
| def self.default_mvi_timeout | ||
| Rails.logger.warn 'Settings.mvi.timeout not set, using default' | ||
| 10 |
There was a problem hiding this comment.
I don't mind removing default like these, provided you can link to the devops tickets here in the PR so we can verify they are properly set. If they are not set for each environment there is a risk of blowing up one of those environments.
There was a problem hiding this comment.
They're not set in ansible/deployment/config/vets-api/prod-settings.local.yml.j2 which becomes config/settings.local.yml. We're good because they're set in config/settings.yml in this repo, which doesn't get overridden.
spec/lib/mvi/configuration_spec.rb
Outdated
| expect(MVI::Configuration::TIMEOUT).to eq(10) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
i support removing these default ones, but can we instead replace them to ensure they have whatever is specified in Settings.yml... Ideally, be explicit in the spec.
ie. expect(MVI::Configuration.new.timeout).to eq(15) NOT expect(MVI::Configuration.new.timeout).to eq(Settings.mvi.readtimeout)
|
@U-DON Correct. There was already code that made it look like |
|
@kreek do you see any problem with us changing these timeouts? It's in an effort to better determine failure rates and come up with better SLI around MVI related issues. |
|
@saneshark 15 seconds seems long to connect, but if it's been using that setting anyway then 👍 |
|
@kreek probably connection timeouts if I had to guess, but its tough to tell for sure as I'm pretty sure the error returned for a timeout is the same for both. |
|
I'm thinking we might want to sync with IAM on how they have things implemented on their end before changing any of these. The timeouts went up markedly beginning about 2.5 weeks ago when they introduced DMDC fallback in querying using attributes. Depending on how they have their application configured we can probably set the open_timeout to something like 1 or 2 seconds, and increase the read_timeout to 15 seconds. But opening a connection shouldn't take long at all -- its the dependency on DMDC that might be hanging that is causing issues, so we should find out what their internal timeout is for DMDC, whether or not they cache that result, and most importantly what timeout they are using and either use something a little bit longer or sensible based on their recommendation. |
|
This PR doesn't change current behavior, it just makes it possible to set |
https://github.com/department-of-veterans-affairs/vets.gov-team/issues/14735
Description of change
There was code that looked like it intended to set the open_timeout to 2 and the read_timeout to 10 for MVI, but it didn't actually do that. The default of 15 was used all along for both. Update the code to reflect that reality. I don't believe that we want to reduce the timeout because we regularly see timeout errors.
tl;dr MVI:: Configuration:: OPEN_TIMEOUT is not the same as MVI::Configuration.instance.open_timeout and we already set default settings in config/settings.yml
Testing done
verified that request_options now contains the timeouts from settings.yml
Testing planned
no additional testing planned
Applies to all PRs