Require Heroku log drains in all n-express apps

[rowanmanning]

Now that we have a timeline for completing the log drain migration (30th June 2023), we want to start failing health checks for apps which don't have them configured yet. The condition based on the migration environment variable was always intended to be removed (FTDCS-233) and now feels like a good time.

Outstanding question: what kind of release should this be? Minor?

Resolves FTDCS-316.

[andygout]

Would the healthcheck added in this PR have the same (or at least the equivalent) effect? (Albeit not stating it explicitly.)

Financial-Times/n-health#201

[rowanmanning]

It is that health check :) just the implementation of n-health's heroku-log-drain check in n-express. We're removing the requirement for MIGRATE_TO_HEROKU_LOG_DRAINS to be set so that any apps which have not yet migrated to log drains will start alerting on this check.

[andygout]

Oh, haha, I see.

[alexmuller]

Nice! I think minor would be ok for this. It will introduce a failing health check for apps which don't have a log drain, but I don't think that constitutes a breaking change? It means upgrading apps may need additional work but doesn't change the n-express API.

[serena97]

Thanks for looping us in on this! We think it should be a breaking change (major semver) because it’s now introducing a failing health check, which could create a lot of noise given we have a lot of apps using n-express that might not be using the log drains. We prefer a more controlled migration phase via a major semver release. Hope that's ok!

[ivomurrell]

Is this going to have the desired effect if only people who manually upgrade to the next major version get the failing health check, though? If the goal is to push people to migrate then I don't think that's going to be achieved if only the people who are already dedicated enough to manually upgrade the version will see the warning.

On the other hand, how much time have people been given so far to migrate to log drains? To me it feels a little premature to start firing off failing health checks in a new minor version when the migration deadline is a little less than a year away still?

[rowanmanning]

I think worth noting this is also a bit of a tidy-up. I'd like to keep the MIGRATE_TO_HEROKU_LOG_DRAINS environment variable around for as short a time as possible, and removing it from n-express would be nice 🙂

Is this going to have the desired effect if only people who manually upgrade to the next major version get the failing health check, though?

Probably not, as the people who upgrade versions quickly are also more likely to just do the log drain migration. however I think it's a non-zero number of apps which will bump n-express and not other dependencies, then get these failing health checks as a reminder. Similar to the Node.js 18 migration I think it's also good to get ahead of this deadline – traditionally we cut it pretty close to the wire and I'd rather target having everything migrated much earlier (like 2022-Q4 or 2023-Q1).

To me it feels a little premature to start firing off failing health checks in a new minor version when the migration deadline is a little less than a year away still?

We prefer a more controlled migration phase via a major semver release. Hope that's ok!

Absolutely fine by me to release this as a major version bump.

[rowanmanning]

I'm gonna say let's not merge this until we've decided what to do with this health check. We may remove it