8000 Embrace Crystal standard Log for logging. by hugopl · Pull Request #705 · kemalcr/kemal · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Embrace Crystal standard Log for logging. #705

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

Merged
merged 7 commits into from
Apr 1, 2025

Conversation

hugopl
Copy link
Contributor
@hugopl hugopl commented Mar 21, 2025

Description of the Change

Kemal uses a LogHandler to log requests, this code predates the Crystal Log class so while the Kemal documentation says that logging is done using Log class, the http request log is done in a different way.

This patch deprecates:

  • Kemal::Config#logger
  • Kemal::Config#logger=(Kemal::BaseLogHandler)
  • log(String)
  • Kemal::LogHandler.initialize(IO)
  • Kemal::LogHandler
  • NullLogHandler

and changes:

  • Add Kemal::Log (Log = ::Log.for(self))
  • Kemal::LogHandler now uses Log.
  • Add Kemal::RequestLogHandler (nodoc)
  • No handler is created if logging is set to false.

Old code using custom log handlers must work as before.

Alternate Designs

Change nothing and follow with life.

Benefits

Unified log interface using the standard library Log facilities.

Possible Drawbacks

If there's some code using LogHandler.new(io) the log wont be written to io, maybe I could keep LogHandler untouched, just deprecated, and create a new RequestLogHandler.

Kemal uses a LogHandler to log requests, this code predates the
Crystal Log class so while the Kemal documentation says that logging is
done using Log class, the http request log is done in a different way.

This patch deprecates:
- Kemal::Config#logger
- Kemal::Config#logger=(Kemal::BaseLogHandler)
- log(String)
- Kemal::LogHandler.initialize(IO)
- NullLogHandler

and changes:

- Add Kemal::Log (Log = ::Log.for(self))
- Kemal::LogHandler now uses Log.
- No handler is created if logging is set to false.

Old code using custom log handlers must work as before.
@hugopl hugopl force-pushed the log-first-citzen branch from 0c937cc to 1b57c2f Compare March 26, 2025 18:10
@hugopl hugopl force-pushed the log-first-citzen branch from 1b57c2f to 8a365e5 Compare March 26, 2025 18:13
@hugopl hugopl requested a review from Sija March 26, 2025 19:43
@sdogruyol sdogruyol requested review from straight-shoota and removed request for Sija March 31, 2025 11:48
Copy link
Member
@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot @hugopl 👍

@sdogruyol sdogruyol merged commit 19d3913 into kemalcr:master Apr 1, 2025
3 of 7 checks passed
@compumike
Copy link
Contributor

We've been using a custom class which < Kemal::BaseLogHandler and emits structured JSON log lines. It ultimately does write to the global Log, roughly like this:

Log.info do
  JSON.build do |json|
    json.object do
      json.field "method", context.request.method
      ...
    end
  end
end

Is there a recommended way to do something like this now that Kemal.config.logger = is deprecated?

@hugopl
Copy link
Contributor Author
hugopl commented Apr 15, 2025

You can create a normal http handler and insert it on Kemal.

Other day I was thinking about a way of ease this process by letting the user pass a proc to be used by the RequestLogHandler Log.info call.

Maybe I will create a patch and request for comments.

@compumike
Copy link
Contributor

@hugopl Thanks -- I just made my custom logger a subclass of Kemal::Handler and did:

logging false
add_handler(MyRequestLogger.new, 1)

Good enough. Works for me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0