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

bdewater · 2019-12-09T23:10:26Z

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.

nateberkopec · 2019-12-10T00:59:51Z

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

nateberkopec · 2019-12-10T01:00:23Z

lib/puma.rb

@@ -19,10 +19,16 @@ def self.stats_object=(val)
    @get_stats = val
  end

-  def self.stats
-    @get_stats.stats
+  def self.stats_hash


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

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

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

bdewater · 2019-12-10T03:54:57Z

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

nateberkopec · 2019-12-16T01:12:00Z

History.md

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


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

^^^^ Still needs this.

nateberkopec · 2019-12-16T01:15:03Z

lib/puma/cluster.rb

@@ -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*) }/)


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

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.

nateberkopec · 2019-12-16T01:15:45Z

lib/puma/cluster.rb

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


this is so much more readable now ❤️

nateberkopec

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

nateberkopec · 2019-12-17T01:58:47Z

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 · 2019-12-17T02:16:22Z

Changelog added & squashed to one commit :)

nateberkopec · 2019-12-17T05:39:03Z

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

schneems · 2020-05-08T01:52:49Z

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 · 2020-05-08T02:06:34Z

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 · 2020-05-08T02:17:50Z

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

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 · 2020-05-08T19:30:05Z

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

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

bdewater force-pushed the stats-hash branch from 50c2099 to b9565e0 Compare December 9, 2019 23:15

nateberkopec added the feature label Dec 10, 2019

nateberkopec reviewed Dec 10, 2019

View reviewed changes

bdewater force-pushed the stats-hash branch from b9565e0 to 93ec6e5 Compare December 10, 2019 01:40

bdewater mentioned this pull request Dec 11, 2019

Symbolize names when parsing JSON yob/puma-plugin-statsd#11

Merged

nateberkopec added this to the 5.0.0 milestone Dec 16, 2019

nateberkopec reviewed Dec 16, 2019

View reviewed changes

nateberkopec requested changes Dec 16, 2019

View reviewed changes

bdewater mentioned this pull request Dec 16, 2019

separate security for control and status actions #2081

Open

nateberkopec changed the title ~~Add Puma.stats_hash~~ Puma.stats is now a hash, not JSON. Dec 17, 2019

bdewater force-pushed the stats-hash branch 2 times, most recently from fb8b934 to bdf5cb5 Compare December 17, 2019 02:15

nateberkopec merged commit 1bec245 into puma:master Dec 17, 2019

bdewater deleted the stats-hash branch January 2, 2020 22:00

julik mentioned this pull request May 3, 2020

Allow use of Hash-shaped stats harmjanblok/puma-metrics#18

Merged

adamlogic mentioned this pull request May 7, 2020

Compatibility with Puma master branch heroku/barnes#31

Closed

bdewater mentioned this pull request May 8, 2020

Compatibility with Puma master heroku/barnes#32

Closed

schneems mentioned this pull request May 8, 2020

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

Merged

cjlarose mentioned this pull request Oct 30, 2020

phased-restart errors after upgrading from Puma 3.12.6 to 5.0.4 #2471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bdewater commented Dec 9, 2019 •

edited

nateberkopec commented Dec 10, 2019

nateberkopec Dec 10, 2019

bdewater Dec 10, 2019

nateberkopec Dec 10, 2019

bdewater commented Dec 10, 2019

nateberkopec Dec 16, 2019

nateberkopec Dec 17, 2019

nateberkopec Dec 16, 2019

bdewater Dec 16, 2019

nateberkopec Dec 16, 2019

nateberkopec left a comment

nateberkopec commented Dec 17, 2019

bdewater commented Dec 17, 2019

nateberkopec commented Dec 17, 2019

schneems commented May 8, 2020

bdewater commented May 8, 2020

schneems commented May 8, 2020

schneems commented May 8, 2020

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

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

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

Conversation

bdewater commented Dec 9, 2019 • edited

Description

Your checklist for this pull request

nateberkopec commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdewater commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec left a comment

Choose a reason for hiding this comment

nateberkopec commented Dec 17, 2019

bdewater commented Dec 17, 2019

nateberkopec commented Dec 17, 2019

schneems commented May 8, 2020

bdewater commented May 8, 2020

schneems commented May 8, 2020

schneems commented May 8, 2020

bdewater commented Dec 9, 2019 •

edited