Update routes within router

[karlbaker02]

This PR adds functionality to Router so that it takes charge of updating its own routes, polling MongoDB every 30 seconds to check for changes and performing the swapping of route tables if updates are found to have occurred. This change takes place at the same time as we stop requests being sent by Router API to force updates to Router instances, allowing us to decouple these apps and make it far easier to run in ECS (as Router API no longer needs to know where to find Router instances).

For more information around these changes, view RFC 135.

[karlbaker02]

Not sure how much logging we want to keep from what I've added: all the useful stuff that I used to debug I've made logDebug and everything else kept as logInfo to have what's hopefully the minimally-viable amount of logs to know when routes are / are not updated. Happy to change this as people see fit.

[sengi]

Haven't fully reviewed but spotted a couple of things I thought worth mentioning - hopefully useful.

[sengi]

Not sure how much logging we want to keep from what I've added: all the useful stuff that I used to debug I've made logDebug and everything else kept as logInfo to have what's hopefully the minimally-viable amount of logs to know when routes are / are not updated. Happy to change this as people see fit.

I think the logging is fine for now, but (just as an optional suggestion for later) you could get better value out of the log lines that you already have by adding specifics to them. For example I would consider either including the timestamps in the updates found / no updates found messages (so that someone reading the log can tell why it decided it found / didn't find an update) or removing them. I agree that Info feels like the right level for those messages, FWIW.

On a related note, it'd be great to add a Prometheus metric for the last-updated timestamp. Not for this PR though - just mentioning as a possibility for later.

[karlbaker02]

Not sure how much logging we want to keep from what I've added: all the useful stuff that I used to debug I've made logDebug and everything else kept as logInfo to have what's hopefully the minimally-viable amount of logs to know when routes are / are not updated. Happy to change this as people see fit.

I think the logging is fine for now, but (just as an optional suggestion for later) you could get better value out of the log lines that you already have by adding specifics to them. For example I would consider either including the timestamps in the updates found / no updates found messages (so that someone reading the log can tell why it decided it found / didn't find an update) or removing them. I agree that Info feels like the right level for those messages, FWIW.

On a related note, it'd be great to add a Prometheus metric for the last-updated timestamp. Not for this PR though - just mentioning as a possibility for later.

I did think about the specifics, actually timestamp of when the event happened, but then I figured that we already have this information recorded (it must be by the logging functions) in Kibana, as well as timestamps provided by Kibana itself (or at least visible therein). Saying that, I'd be happy to add some specifics in.

The Prometheus metric sounds like a good thing.

[sengi]

I'm having a bit of crisis of conscience with this review if I'm honest. I'm used to a much higher bar for well-structured code and testing, and the lack of tests for this really worry me. On the other hand, the existing tests are in a poor state to begin with and this is an essentially unowned code base with a lot of pre-existing issues, and I sense that you're feeling pressure to get this out the door.

I'll therefore defer to your judgement as to where to set the bar. As an experienced GOV.UK developer you no doubt have a better sense of where that bar is than I do. So if you're happy to roll this out, then I don't want to block you. I appreciate you've done some manual testing and let the branch soak in Integration for a bit.

(In case you're wondering what I would do if it were my change: I'd probably split up and simplify pollAndReload to make it a more obvious "if shouldReload() then reload" loop, rip out those dubious pre-existing/copied recovers and the anonymous functions that are only there because of them, and add some tests for all the stuff I'd touched - which would then be easier/possible to write once the nontrivial stuff is no longer buried in anon functions. Nothing stopping us doing that later, of course.)

[karlbaker02]

I'm having a bit of crisis of conscience with this review if I'm honest. I'm used to a much higher bar for well-structured code and testing, and the lack of tests for this really worry me. On the other hand, the existing tests are in a poor state to begin with and this is an essentially unowned code base with a lot of pre-existing issues, and I sense that you're feeling pressure to get this out the door.

I'll therefore defer to your judgement as to where to set the bar. As an experienced GOV.UK developer you no doubt have a better sense of where that bar is than I do. So if you're happy to roll this out, then I don't want to block you. I appreciate you've done some manual testing and let the branch soak in Integration for a bit.

(In case you're wondering what I would do if it were my change: I'd probably split up and simplify pollAndReload to make it a more obvious "if shouldReload() then reload" loop, rip out those dubious pre-existing/copied recovers and the anonymous functions that are only there because of them, and add some tests for all the stuff I'd touched - which would then be easier/possible to write once the nontrivial stuff is no longer buried in anon functions. Nothing stopping us doing that later, of course.)

Thanks for this Chris. I felt like we might have wanted to get this out of the door as it has been around for a little while now, but I hear what you say and I think you're right that it could benefit from some tests. I was initially happy to use the existing tests as they cover the behaviour of reloading routes and they caught quite a few issues during development, which gave me confidence that they had good coverage. Still, it probably is worth spending a bit more time adding some additional tests (there are currently no unit tests around Router for example, though as documented in the README, this was a design choice at the time Router was written). I'll get working on this and push up another commit once done - it might not cover everything you've mentioned but I think it should at least make a good start.