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

Refactor: Split out LogWriter from Events (no logic change) #2798

Merged
merged 18 commits into from Feb 5, 2022

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Jan 11, 2022

This PR splits the Puma::Events object into two separate objects, Events and LogWriter.

Currently the Events object combines two completely unrelated responsibilities:

  • Register and callback "hooks" for internal events, like :on_booted
  • Perform logging, including info/warn/debug.

These two concerns have no dependency on each other, so they should be separate objects. As Puma newbie, the fact that logging functionality was in the Events class gave me the mistaken impression that logging somehow used the same callback hooks that events do. When the @events object is passed into other classes, it's also not clear for which of these responsibilities it will be used.

This PR does not change any application logic, but it does change the constructor interface of some classes, most notably Puma::Server (it adds an extra arg for a LogWriter instance.)

@nateberkopec nateberkopec added refactor waiting-for-review Waiting on review from anyone labels Jan 13, 2022
@johnnyshields
Copy link
Contributor Author

johnnyshields commented Feb 1, 2022

@nateberkopec This PR is likely to break as other PRs are merged, so I'd like to get a preliminary review to confirm the Puma team accepts this change in principle.

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Feb 1, 2022
@nateberkopec
Copy link
Member

I love it! We could take this even further: if you moved lines 83+ from log_writer.rb into their own object or somewhere else, then at that point I think you could just rename LogWriter to Logger and inherit from < ::Logger and clean this up completely. What do you think about that?

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Feb 3, 2022

LogWriter to Logger and inherit from < ::Logger and clean this up completely

Wow, yeah! That would be the ultimate! Was trying to minimize change.

That being said, lets do it as a series of two PRs.

@nateberkopec
Copy link
Member

This is shippable for me when the tests pass

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Feb 5, 2022

@nateberkopec this is ready to merge. Appreciate if we can merge this first before other PRs because it's likely to have conflicts.

@nateberkopec nateberkopec merged commit 8a4ef0c into puma:master Feb 5, 2022
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
…ma#2798)

* Split out LogWriter from Events

* Improve code comment

* Fix constructor interfaces

* Fix file includes

* Fix specs and requires

* Fix LogWriter

* More fixes

* Fix tests

* Fix specs

* Fix spec

* Fix more specs

* Refactor: Split out LogWriter from Events

* Improve comments

* Fix bundle pruner

Co-authored-by: shields <shields@tablecheck.com>
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 20, 2022
nateberkopec pushed a commit that referenced this pull request Sep 21, 2022
…2965)

* Revert change to `Server#initialize` (new) method signature from #2798

* Test changes for updated Server#intialize
@elia
Copy link
Contributor

elia commented Oct 14, 2022

@nateberkopec thoughts on adding this PR to the breaking changes section of the v6 changelog in light of teamcapybara/capybara#2590?

@dentarg
Copy link
Member

dentarg commented Oct 14, 2022

@elia Sure, just make a PR

@johnnyshields johnnyshields deleted the split-log-writer-from-events branch October 15, 2022 05:50
@johnnyshields
Copy link
Contributor Author

@nateberkopec not sure if Puma is following this but I've seen other repos use a code annotation @api private for methods/classes that should be considered private, even though in terms of Ruby the methods are public. This will prevent heartbreak in terms of third party libraries expecting classes like LogWriter to be a "stable public API".

elia added a commit to elia/puma that referenced this pull request Oct 15, 2022
@elia
Copy link
Contributor

elia commented Oct 15, 2022

@johnnyshields good call, would be great if, in addition to marking them as private, a public api that covers that use case was also offered.

@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 15, 2022

@api private is essentially the Yard equivalent of RDoc's :nodoc:. Recently, a few classes/modules had :nodoc: added to them, see PR #2988.

I don't think that LogWriter should be made :nodoc: / @api private. Using Puma in standard way, it probably could be private, but Puma is often used in test frameworks like capybara, where a Puma Server is started using Server.new. That needs LogWriter...

EDIT:
To clarify (as some of this stuff is pretty 'pounded into my head'), in Puma 5 and earlier, Server.new had a parameter that was an Events object. In Puma 6, Events was split, and all methods that wrote to a log were moved to LogWriter. To keep the parameters the same, the LogWriter was added to the config/options hash parameter. If the key doesn't exist, a LogWriter.stdio is created. Test frameworks often want server output silenced, so that isn't appropriate.

It is still a breaking change, in that code to create a Server with Puma 5 and earlier will not work with Puma 6.

dentarg pushed a commit that referenced this pull request Oct 15, 2022
tombruijn added a commit to appsignal/appsignal-ruby that referenced this pull request Oct 18, 2022
The class responsible for logging was split of from the Puma Events
class into a new LogWriter class in PR
puma/puma#2798.

Rely on this location first for Puma 6 and fallback on the events class
for Puma 5.

Fixes #886
tombruijn added a commit to appsignal/appsignal-ruby that referenced this pull request Oct 18, 2022
The class responsible for logging was split of from the Puma Events
class into a new LogWriter class in PR
puma/puma#2798.

Rely on this location first for Puma 6 and fallback on the events class
for Puma 5.

Fixes #886
danarnold added a commit to cacheventures/puma-plugin-statsd that referenced this pull request Nov 15, 2022
tricknotes pushed a commit to tricknotes/puma-plugin-statsd that referenced this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants