Use strongly-typed `Duration`s for time

rtyley commented 1 month ago

This PR improves time & duration handling in Riff Raff by switching representation from Int or Long types (types which don't incorporate a time-unit, meaning we have to work out from our context whether we are using seconds, milliseconds, epoch-ms, etc) to using the great java.time.* classes Instant & Duration.

This fixes a lot of things like:

slightly obscure integer constants like 5 * 60 * 1000 (denoting 5 minutes)
the inability of the compiler to check we're performing the right kind of operation to change time-units - the compiler can't know if what we're doing with an Int makes sense!
variable names like durationMillis, where 'millis' had to be included because the declared type of Long isn't telling us what the type is
frequently multiplying numbers by 1000 to transform a value that we are quite sure is measured in seconds, into milliseconds!
the risk that integer arithmetic might overflow when making time calculations, silently corrupting the result

If you can use a strongly-typed time class instead - like Duration or Instant - the compiler and library author can do all of that stuff for us, it's much better! You also stand a better chance of avoiding the hallowed falsehoods programmers believe about time.

From `Param[Int]` to `Param[Duration]` - durations are still specified in seconds ⏱️

Riff Raff has several parameters which specify durations (eg healthcheckGrace, warmupGrace, etc). Historically these have all expected integer values and been documented to be duration in seconds:

    parameters:
      healthcheckGrace: 420
      warmupGrace: 300

We don't want to make everyone change their Riff Raff configuration, and so those parameters need to continue to accept the same numeric values and still interpret them as seconds.

The Param.waitingSecondsFor() helper method lets us create parameters that are a Param[Duration] (which could have been problematic because java.time.Duration has no specified time unit) that still explicitly parse their input as seconds, and are tested to do so.

Migrating from Joda-Time to java.time

We should be using java.time.* classes in our codebases, in preference to the wonderful but old Joda-Time library:

https://blog.joda.org/2014/11/converting-from-joda-time-to-javatime.html

Testing

I've deployed this branch to CODE Riff Raff (https://riffraff.code.dev-gutools.co.uk/) and then used CODE Riff Raff to deploy the Ophan Dashboard to PROD, and DotCom Frontend to CODE, successfully.

Frontend uses a custom warmupGrace value of 60 seconds, different to the default of 1 second we normally use:

https://github.com/guardian/riff-raff/blob/feaf4f0661fe50d6b3c9ea0f246c246691622403/magenta-lib/src/main/scala/magenta/deployment_type/AutoScaling.scala#L100-L105

We can see in this successful deploy of Frontend CODE by this branch of Riff Raff, that the warmupGrace value of 60 has still been correctly parsed, and displayed as 60000 milliseconds:

Deploy build 3379 of `tools::riffraff` to CODE

All deployment options

- [Deploy build 3379 of `tools::riffraff` to CODE](https://riffraff.gutools.co.uk/deployment/deployAgain?project=tools%3A%3Ariffraff&build=3379&stage=CODE&updateStrategy=MostlyHarmless&action=deploy) - [Deploy parts of build 3379 to CODE by previewing it first](https://riffraff.gutools.co.uk/preview/yaml?project=tools%3A%3Ariffraff&build=3379&stage=CODE&updateStrategy=MostlyHarmless) - [What's on CODE right now?](https://riffraff.gutools.co.uk/deployment/history?projectName=tools%3A%3Ariffraff&stage=CODE)

From guardian/actions-riff-raff.

rtyley commented 4 days ago

An unintended consequence of this PR is that unfortunately Riff Raff's autogenerated documentation is displaying default values which now look like ISO 8601 Durations (PT20S, PT15M, etc)

These are nicer to look at, but the real riff-raff.yaml fields actually currently do not accept ISO 8601 Duration strings, and only accept the number-of-seconds as they always did - so I've managed to make the documentation more confusing.

Looking at the docs.DeployTypeDocs.defaultFromParam() method, it looks difficult to cleanly fix this to render a default Duration value as seconds. It might be easier to make the field accept ISO 8601 Duration strings, as well as the numeric seconds fields that currently exist in the wild and which we will need to continue supporting.

guardian / riff-raff