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

Puma.stats is now a hash, not JSON. #2086

Merged
merged 1 commit into from Dec 17, 2019
Merged

Conversation

@bdewater
Copy link
Contributor

bdewater commented Dec 9, 2019

Description

Having access to the hash allows to produce stats in other ways (such as StatsD) without having to parse JSON of data that is available in memory. Examples of this workaround are:

Changed Puma.stats to Puma.stats_json to help disambiguate. An alias is provided for backwards compatibility. In a new major release Puma.stats could point to the hash version or be removed altogether.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
@bdewater bdewater force-pushed the bdewater:stats-hash branch from 50c2099 to b9565e0 Dec 9, 2019
@nateberkopec
Copy link
Member

nateberkopec commented Dec 10, 2019

In a new major release Puma.stats could point to the hash version or be removed altogether.

Luckily for us the next release will probably be 5.0.

Anyone else have thoughts on this? Consider the proposal basically changing Puma.stats to a hash, so the upgrade path will be "everyone using Puma.stats should change to Puma.stats.to_json".

lib/puma.rb Outdated
@@ -19,10 +19,16 @@ def self.stats_object=(val)
@get_stats = val
end

def self.stats
@get_stats.stats
def self.stats_hash

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 10, 2019

Member

FWIW I don't like the rename. We should either add stats_hash or just change stats to a hash and bump major.

This comment has been minimized.

Copy link
@bdewater

bdewater Dec 10, 2019

Author Contributor

I was guessing this would land on a 4.x release, but happy to change :)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 10, 2019

Member

Yeah no way for you to know really but I'm targeting the next release to be a major bump 👍

@bdewater bdewater force-pushed the bdewater:stats-hash branch from b9565e0 to 93ec6e5 Dec 10, 2019
@bdewater
Copy link
Contributor Author

bdewater commented Dec 10, 2019

Consider the proposal basically changing Puma.stats to a hash, so the upgrade path will be "everyone using Puma.stats should change to Puma.stats.to_json".

Done :)

History.md Outdated
@@ -3,6 +3,7 @@
* Features
* Add pumactl `thread-backtraces` command to print thread backtraces (#2053)
* Configuration: `environment` is read from `RAILS_ENV`, if `RACK_ENV` can't be found (#2022)
* `Puma.stats` now returns a Hash instead of a JSON string

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 16, 2019

Member

Needs a link to your PR number (#2086).

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 17, 2019

Member

^^^^ Still needs this.

@@ -94,7 +94,11 @@ def term?

def ping!(status)
@last_checkin = Time.now
@last_status = status
captures = status.match(/{ "backlog":(?<backlog>\d*), "running":(?<running>\d*), "pool_capacity":(?<pool_capacity>\d*), "max_threads": (?<max_threads>\d*) }/)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 16, 2019

Member

Shouldn't we just deserialize the JSON response, really?

This comment has been minimized.

Copy link
@bdewater

bdewater Dec 16, 2019

Author Contributor

There seems to have been an intentional decision made in the past to not let Puma rely on the JSON gem. I don't have the context as to why but since this is an internal message being passed I figured it wasn't worth adding the dependency as we don't need more robust parsing. I can change it if you still think it is better to do so.

@@ -352,9 +356,26 @@ def reload_worker_directory
# the master process.
def stats

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Dec 16, 2019

Member

this is so much more readable now ❤️

Copy link
Member

nateberkopec left a comment

A few minor requests, then I think this is good for inclusion in 5.0

@nateberkopec nateberkopec changed the title Add Puma.stats_hash Puma.stats is now a hash, not JSON. Dec 17, 2019
@nateberkopec
Copy link
Member

nateberkopec commented Dec 17, 2019

Needs the changelog mod, then it's g2g.

Having access to the hash allows to produce stats in other ways (such as StatsD) without having to parse JSON of data that is available in memory. An example of this workaround is https://github.com/yob/puma-plugin-statsd/blob/fa6ba1f5074473618643ef7cf99747801a001dec/lib/puma/plugin/statsd.rb#L112-L114
@bdewater bdewater force-pushed the bdewater:stats-hash branch 2 times, most recently from fb8b934 to bdf5cb5 Dec 17, 2019
@bdewater
Copy link
Contributor Author

bdewater commented Dec 17, 2019

Changelog added & squashed to one commit :)

@nateberkopec nateberkopec merged commit 1bec245 into puma:master Dec 17, 2019
11 of 12 checks passed
11 of 12 checks passed
OS: ubuntu-16.04 Ruby: 2.3.x OS: ubuntu-16.04 Ruby: 2.3.x
Details
build
Details
OS: ubuntu-18.04 Ruby: 2.4.x
Details
OS: ubuntu-18.04 Ruby: 2.5.x
Details
OS: ubuntu-18.04 Ruby: 2.6.x
Details
OS: macos Ruby: 2.4.x
Details
OS: macos Ruby: 2.5.x
Details
OS: macos Ruby: 2.6.x
Details
OS: windows-latest Ruby: 2.4.x
Details
OS: windows-latest Ruby: 2.5.x
Details
OS: windows-latest Ruby: 2.6.x
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nateberkopec
Copy link
Member

nateberkopec commented Dec 17, 2019

Thanks! I'll be writing an upgrade guide later, will be sure to mention this.

@schneems
Copy link
Contributor

schneems commented May 8, 2020

I'm going to move to remove this change. It will break the barnes gem or anyone and only mysteriously in production. I'm proposing we introduce a new method so we can preserve backward compatibility I'm thinking Puma.stats_hash and preserving the current interface. If we still want to remove Puma.stats and it's current behavior, we should go through a deprecation cycle i.e. cut a release to start emitting warnings right now before releasing a major breaking change.

@bdewater
Copy link
Contributor Author

bdewater commented May 8, 2020

That gem has the exact behaviour why I opened this PR, why parse JSON of what you have in memory? It's trivial to work around.

@schneems
Copy link
Contributor

schneems commented May 8, 2020

That gem has the exact behaviour why I opened this PR, why parse JSON of what you have in memory? It's trivial to work around.

It's already existing behavior and would break countless apps if it changed. The proposed behavior is good, but lets make a non breaking change to get it

schneems added a commit that referenced this pull request 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request 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
Copy link
Contributor

schneems commented May 8, 2020

I've got a spiked PR of this in #2253. Tests are passing, can y'all take a look?

nateberkopec pushed a commit that referenced this pull request May 11, 2020
* Revert api change from #2086 introduce Puma.stats_hash api

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.
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

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