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

Feature/support custom logger with request logs #3140

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

phyzical
Copy link
Contributor

@phyzical phyzical commented May 1, 2023

Description

This is just a small adjustment to what was done with #2770

This just also uses the custom logger for request logs aswell

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.

@dentarg
Copy link
Member

dentarg commented May 1, 2023

I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.

can't see you have ;)

@phyzical
Copy link
Contributor Author

phyzical commented May 1, 2023

@dentarg sorry, i will have a crack at them tomorrow, my fault for pushing a small pr at the end of the day

@phyzical
Copy link
Contributor Author

phyzical commented May 2, 2023

@dentarg hey i was hoping for some guidance i was able to get a test going and working as expected in test/test_integration_single.rb but i cant seem to be able to provide the config for the test in test/test_puma_server.rb. and i don't seem to see any other tests testing the request_logs config

or is this one working test sufficient?

@nateberkopec nateberkopec added feature waiting-for-review Waiting on review from anyone labels May 7, 2023
test/test_puma_server.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels May 16, 2023
feature/support-custom-logger-with-request-logs
@phyzical
Copy link
Contributor Author

@nateberkopec should be all fixed up now 👍

@phyzical phyzical requested a review from dentarg May 29, 2023 00:54
@ndbroadbent
Copy link

ndbroadbent commented Jul 13, 2023

Would love to start using this, I just set up a custom logger since I assumed it would be used for the request logs as well. My use-case is filtering noisy requests from the log (health check requests):

  class CustomLogger
    def initialize(output = $stdout)
      @output = output
    end

    def write(msg)
      return if msg.include?('GET /health')

      @output.puts msg
      @output.flush
    end
  end

@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Jul 17, 2023
@nateberkopec nateberkopec added breaking change v7 Breaking changes for v7 and removed waiting-for-review Waiting on review from anyone labels Aug 31, 2023
@nateberkopec
Copy link
Member

Marking as breaking/v7.

We now have 2 changes on master for v7, maybe one or two more and we can consider a major release. Samuel's fiber-per-request might be a good "banner" feature for it as well.

@dentarg
Copy link
Member

dentarg commented Aug 31, 2023

We now have 2 changes on master for v7

Which are those?

I just created an 7.0.0 milestone, would be good to add their issues/PRs to it.

@dentarg dentarg added this to the 7.0.0 milestone Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature v7 Breaking changes for v7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants