Revert api change from #2086 introduce Puma.stats_hash api

[schneems]

The change in #2086 is not backwards compatible with existing gems that parse the output of Puma.stats such as Barnes.

Releasing a version of puma with this change would break anyone using the Barnes app and only in production. I'm proposing to keep the existing interface and instead add a new API. This buys us all the features of #2086 without causing any production facing downtime by customers due to API incompatibilities.

Unfortunately, it requires that we serialize and then de-serialize the values. One prior benefit of returning json in a string was that it allowed an end-user to de-serialize using a faster json algorithm such as oj via the "multi json" gem. But the performance penalty will be better than a stability break.

[nateberkopec]

only in production

All the other breaking changes we made should blow up at startup-time in development, this change is definitely different in that respect.

Puma hasn't had a deprecation policy, promise or procedure, so this change makes me wonder what (if anything) that should be.

[bdewater]

only in production

All the other breaking changes we made should blow up at startup-time in development, this change is definitely different in that respect.

Something something tests 😉 but this is less surprising for sure.

Puma hasn't had a deprecation policy, promise or procedure, so this change makes me wonder what (if anything) that should be.

That's a good point. I do feel stats returning a JSON string is not a very intuitive API.

[schneems]

Thanks, y'all!

so this change makes me wonder what (if anything) that should be.

In Rails it's deprecate and warn before removal. Usually, deprecations are in there for a LONG time as the release cycle for Rails isn't terribly aggressive. Also they maintain multiple version forks at a time while we don't.

I've not done this in practice, but in theory it would be cool to ship all deprecations with a rubocop rule to update people's code.

All the other breaking changes we made should blow up at startup-time in development

Barnes only fires when it detects it's running on heroku and specifically in a "web" dyno, also it checks for the presence of a statsd connection. While it's technically possible to reproduce all those conditions in a dev environment, most people don't/won't. Looking at the other breaking changes, I think you're right that they would be made known at boot time. I think the only possible issue I could see is if dependabot sends a PR to upgrade a puma version, and their tests don't actually boot a server via puma (I don't think code triage does for example) then the tests would pass but when it's merged and auto deployed it would fail.

I'm not sure how much we care about that case or if/what we could do. Maybe provide a linter or check for the config/puma.rb file perhaps to use in a test environment could be one option.

[dentarg]

Barnes only fires when it detects it's running on heroku and specifically in a "web" dyno, also it checks for the presence of a statsd connection.

Thanks for explaining this (as I made a very brief attempt running my tests against Puma master with an Heroku-app using Barnes, but I got no failures)

I think the only possible issue I could see is if dependabot sends a PR to upgrade a puma version, and their tests don't actually boot a server via puma (I don't think code triage does for example) then the tests would pass but when it's merged and auto deployed it would fail.

I think more people should write tests booting their app, there's even an super useful gem for it! 😄 (Yes, I know who made it 😉)