Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert api change from #2086 introduce Puma.stats_hash api #2253

Merged
merged 2 commits into from May 11, 2020

Conversation

@schneems
Copy link
Contributor

schneems commented May 8, 2020

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.

@schneems schneems force-pushed the schneems/add-stats-back-in branch from da15cb5 to 6d8e889 May 8, 2020
schneems added 2 commits May 8, 2020
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 the 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.
@schneems schneems force-pushed the schneems/add-stats-back-in branch from 6d8e889 to 0cbace3 May 8, 2020
@dentarg dentarg mentioned this pull request May 9, 2020
5 of 8 tasks complete
@nateberkopec
Copy link
Member

nateberkopec commented May 10, 2020

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.

@nateberkopec nateberkopec added this to the 5.0.0 milestone May 10, 2020
@nateberkopec nateberkopec merged commit 0959648 into master May 11, 2020
53 checks passed
53 checks passed
ubuntu 2.2
Details
ubuntu 2.2
Details
build
Details
ubuntu 2.3
Details
ubuntu 2.3
Details
ubuntu 2.4
Details
ubuntu 2.4
Details
ubuntu 2.5
Details
ubuntu 2.5
Details
ubuntu 2.6
Details
ubuntu 2.6
Details
ubuntu 2.7
Details
ubuntu 2.7
Details
ubuntu head
Details
ubuntu head
Details
ubuntu jruby
Details
ubuntu jruby
Details
ubuntu truffleruby-head
Details
ubuntu truffleruby-head
Details
macos 2.2
Details
macos 2.2
Details
macos 2.3
Details
macos 2.3
Details
macos 2.4
Details
macos 2.4
Details
macos 2.5
Details
macos 2.5
Details
macos 2.6
Details
macos 2.6
Details
macos 2.7
Details
macos 2.7
Details
macos head
Details
macos head
Details
macos jruby
Details
macos jruby
Details
macos truffleruby-head macos truffleruby-head
Details
macos truffleruby-head
Details
windows 2.2
Details
windows 2.2
Details
windows 2.3
Details
windows 2.3
Details
windows 2.4
Details
windows 2.4
Details
windows 2.5
Details
windows 2.5
Details
windows 2.6
Details
windows 2.6
Details
windows 2.7
Details
windows 2.7
Details
windows mingw
Details
windows mingw
Details
ubuntu jruby-head ubuntu jruby-head
Details
ubuntu jruby-head ubuntu jruby-head
Details
@bdewater
Copy link
Contributor

bdewater commented May 11, 2020

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
Copy link
Contributor Author

schneems commented May 11, 2020

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
Copy link
Contributor

dentarg commented May 11, 2020

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 😉)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.