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.

Sign up for GitHub

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

Jump to bottom

Hook in some basic logging to the service #20

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Hook in some basic logging to the service #20

wants to merge 19 commits into from

Conversation

danielmorrison
Copy link
Member

First attempt at outputting in some better logging. Not sure what exactly we want to output yet, but throwing this out as an initial idea.

albus522
albus522 reviewed Jan 10, 2025
View reviewed changes
lib/twirp/rails/engine.rb Outdated Show resolved Hide resolved
albus522
albus522 reviewed Jan 10, 2025
View reviewed changes
lib/twirp/rails/engine.rb Outdated Show resolved Hide resolved
albus522
albus522 reviewed Jan 10, 2025
View reviewed changes
lib/twirp/rails/engine.rb Outdated Show resolved Hide resolved
@danielmorrison
Copy link
Member Author

Ha, Ruby 3.4 build fails because the default hash to_s has changed:

         expected: ("Twirp invalid_argument (400) in 0ms (invalid_argument: is too big - {:argument=>\"inches\"})")
              got: (no args) (1 time)
                   ("Twirp invalid_argument (400) in 0ms (invalid_argument: is too big - {argument: \"inches\"})")

I'm a fan.

@danielmorrison
Fix test on Ruby 3.4
83a59f8 
Hash#to_s changed between version creating diffrent string output.
@danielmorrison
Use a custom middleware for logging and put response in debug log
9727aa3 
Way simpler than how I was doing this. Still can't decide what else to put in the logs, if anything.
@danielmorrison
Revert "Update readme to use procs"
76d0a22 
This reverts commit 4336d84.

No longer needed, so let's go back.
@danielmorrison
Revert change to service hook in test
5389c73 
We don't need this change anymore, so go back.
@danielmorrison
Refactor logger and make it configurable
3e2eba2 
* Add it at the top of the middleware stack.
* Inherit from Leverage Rack::CommonLogger though we don't use common log format.
* Improve handling of response output in when log_level is :debug
@danielmorrison
Copy link
Member Author

Refactored to use a Rack middleware and it is nicer for the common case.

The ability to show the response in :debug mode seems helpful but I don't like how it has to work. I think the better option would be to hook into the Service for that (which would get messier) or have it be a default after hook on the handler.

@danielmorrison danielmorrison marked this pull request as ready for review January 13, 2025 04:20
@danielmorrison
Copy link
Member Author

If you're using gzip/deflate, the current :debug logging doesn't show anything. It seems like putting this earlier in the process would be better.

@danielmorrison
Change up debug body logging again
ff95522 
* Move out of the Logger so we can output the response even when the actual response is gzip/brotli.
* Output Twirp objects rather than protobuf/json for readability.
@danielmorrison
Add a verbose_logging option to config
070151a
@danielmorrison
Fix testing hash output under old Ruby versions
e068abb
@danielmorrison
Copy link
Member Author

Another day, another update.

I'm now logging the Twirp object if you set verbose_logging = true (or by default if your log_level is :debug)

Using the Twirp object seems nice. While it can be fun to see the protobuf object in the logs, it really isn't handy.

@albus522 / @darronschall I'm happy with how this all works now. Happy to bikeshed about what format the logs take. I don't actually have opinions there.

albus522
albus522 reviewed Jan 14, 2025
View reviewed changes
lib/twirp/rails/engine.rb Outdated Show resolved Hide resolved
albus522
albus522 reviewed Jan 14, 2025
View reviewed changes
lib/twirp/rails/engine.rb Outdated Show resolved Hide resolved
@danielmorrison
Use block syntax for log output
df249ea 
Refactor tests as the block syntax makes checking a little different.

Ensure we have a test for exception_raised logging.
@danielmorrison
Turn off verbose_logging by default and updated readme
02bbbce
@danielmorrison
Copy link
Member Author

Improved the logs a bit, and I'm liking where it has landed.

Now, verbose_logging is false unless you opt-in. What it enables is the response logging. Error responses always get logged (as :debug), as that's not sensitive data.

Only thing left to bikeshed is the log messages themselves. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albus522 albus522 albus522 left review comments

@darronschall darronschall Awaiting requested review from darronschall

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielmorrison @albus522