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

Custom logger #2770

Merged
merged 23 commits into from Mar 29, 2023
Merged

Custom logger #2770

merged 23 commits into from Mar 29, 2023

Conversation

vzajkov
Copy link
Contributor

@vzajkov vzajkov commented Dec 14, 2021

Description

Closes #2511

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) 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.

@MSP-Greg
Copy link
Member

MSP-Greg commented Dec 14, 2021

@vzajkov

  1. Could you please rebase?
  2. test/test_config.rb runs parallel. The added test uses capture_subprocess_io, which remaps io. This may intermittently cause other tests to fail. So, it needs to run in a separate class. I pushed a commit that changes it, as below, which is already in master:

puma/test/test_config.rb

Lines 415 to 425 in 65a821a

# contains tests that cannot run parallel
class TestConfigFileSingle < TestConfigFileBase
def test_custom_logger_from_DSL
conf = Puma::Configuration.new { |c| c.load 'test/config/custom_logger.rb' }
conf.load
out, _ = capture_subprocess_io { conf.options[:logger].write 'test' }
assert_equal 'Custom logging: test', out
end
end

Or, this is messy right now, as the original commit for the PR was reverted, but my commit to fix the test was not. So the test is failing in master...

@MSP-Greg
Copy link
Member

@vzajkov I reverted the test changes above so master will pass. See 65f0ced

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Feb 1, 2022
@nateberkopec
Copy link
Member

In order to merge, all places where Puma currently logs will have to be switched to this new logger object.

@johnnyshields
Copy link
Contributor

We just merged #2798 which affects logging, so you'll want to pull latest master to this branch.

@Overload119
Copy link

Hey @vzajkov

Do you need help with this? Let me know if I can help.

@vzajkov
Copy link
Contributor Author

vzajkov commented Mar 17, 2022

Hey thanks - I will rebase and let you know if I have any questions. Appreciate it.

lib/puma/log_writer.rb Outdated Show resolved Hide resolved
@phyzical
Copy link
Contributor

any update on this?

i am looking for a way to filter request logs matching a specific domain was hoping to handle this via a custom logger (unless someone knows of an existing way to achieve this for an app that doesn't use rails)

@svoop
Copy link

svoop commented Feb 12, 2023

@vzajkov Thanks a lot for your work on this! Would be really great to get custom loggers and DRY at least my code quite a bit.

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

For watchers of this thread: we cannot merge this because it breaks the tests. If you would like to pick this up and get the tests passing, you can do so.

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor contrib-wanted and removed waiting-for-review Waiting on review from anyone labels Feb 21, 2023
Copy link
Contributor

@phyzical phyzical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to get it passing with these changes, i noticed options[:logger] is already being used in the launcher.rb though im not sure if that was by design but it seemed that the change in the launcher is what caused all the other tests to output puma startup strings and cause failures

lib/puma/dsl.rb Outdated Show resolved Hide resolved
lib/puma/launcher.rb Outdated Show resolved Hide resolved
test/config/custom_logger.rb Outdated Show resolved Hide resolved
test/test_config.rb Outdated Show resolved Hide resolved
test/test_config.rb Outdated Show resolved Hide resolved
vzajkov and others added 6 commits March 1, 2023 15:55
Co-authored-by: Jack <5182053+phyzical@users.noreply.github.com>
Co-authored-by: Jack <5182053+phyzical@users.noreply.github.com>
Co-authored-by: Jack <5182053+phyzical@users.noreply.github.com>
Co-authored-by: Jack <5182053+phyzical@users.noreply.github.com>
Co-authored-by: Jack <5182053+phyzical@users.noreply.github.com>
@vzajkov
Copy link
Contributor Author

vzajkov commented Mar 2, 2023

Thanks @phyzical! I've added your changes but it seems to be failing 1 test after merging in master.

@nateberkopec Is it possible that this is an intermittent failure?

@MSP-Greg
Copy link
Member

MSP-Greg commented Mar 2, 2023

Is it possible that this is an intermittent failure?

Yes. I just ran the current PR in my fork, a different job failed...

@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Mar 29, 2023
@nateberkopec nateberkopec merged commit 1328380 into puma:master Mar 29, 2023
64 of 65 checks passed
@nateberkopec
Copy link
Member

It's finally done! Sorry this took so long... out in 6.2.

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.

Supporting Custom Logger Class
9 participants