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

Fix multiple logger handlers when custom logger is used #653

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

aravindavk
Copy link
Contributor

@aravindavk aravindavk commented Jan 29, 2023

When logger function is used, it adds logger handler after the static file handler. This causes 404 errors for static files. Default logger is also added this makes two logger handlers per every request.

Reproducer:

require "kemal"

public_folder "./public"

get "/hello" do |env|
  "Hello!"
end

class MyCustomLogger < Kemal::BaseLogHandler
  def call(context)
    puts "I'm logging some custom stuff here."
    call_next(context) # => This calls the next handler
  end

  # This is used from `log` method.
  def write(message)
    STDERR.puts message # => Logs the output to STDERR
  end
end

# This works
# Kemal.config.logger = MyCustomLogger.new

# This fails while serving static files
logger MyCustomLogger.new

Kemal.run

With this PR, logger function only updates the Kemal.config and Kemal.run will take care of adding logger handler.

This PR also fixes the warning when tried to run the example code for adding custom logger.

 10 | def call(env)
               ^--
Warning: positional parameter 'env' corresponds to
parameter 'context' of the overridden method
Kemal::BaseLogHandler#call(context : HTTP::Server::Context),
which has a different name and may affect named argument passing

Signed-off-by: Aravinda Vishwanathapura [email protected]

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

When `logger` function is used, it adds logger handler
after the static file handler. This causes 404 errors for
static files. Default logger is also added this makes two
logger handlers per every request.

Reproducer:

```crystal
require "kemal"

public_folder "./public"

get "/hello" do |env|
  "Hello!"
end

class MyCustomLogger < Kemal::BaseLogHandler
  def call(context)
    puts "I'm logging some custom stuff here."
    call_next(context) # => This calls the next handler
  end

  # This is used from `log` method.
  def write(message)
    STDERR.puts message # => Logs the output to STDERR
  end
end

\# This works
\# Kemal.config.logger = MyCustomLogger.new

\# This fails while serving static files
logger MyCustomLogger.new

Kemal.run

```

With this PR, `logger` function only updates the `Kemal.config`
and `Kemal.run` will take care of adding logger handler.

This PR also fixes the warning when tried to run the example code
for adding custom logger.

```
 10 | def call(env)
               ^--
Warning: positional parameter 'env' corresponds to
parameter 'context' of the overridden method
Kemal::BaseLogHandler#call(context : HTTP::Server::Context),
which has a different name and may affect named argument passing
```

Signed-off-by: Aravinda Vishwanathapura <[email protected]>
@aravindavk
Copy link
Contributor Author

CI failures(fmt and lint issues) are not related to this PR. Please look into this.

aravindavk added a commit to aravindavk/moana that referenced this pull request Jan 30, 2023
- Use `HTTP::LogHandler`
- Fix the logger initialization. Use `Kemal.config.logger = APILogHandler.new`
  instead of `logger APILogHandler.new` (See issue:
  kemalcr/kemal#653)

Signed-off-by: Aravinda Vishwanathapura <[email protected]>
aravindavk added a commit to kadalu/moana that referenced this pull request Feb 1, 2023
- Use `HTTP::LogHandler`
- Fix the logger initialization. Use `Kemal.config.logger = APILogHandler.new`
  instead of `logger APILogHandler.new` (See issue:
  kemalcr/kemal#653)

Signed-off-by: Aravinda Vishwanathapura <[email protected]>
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.

Thanks a lot @aravindavk 🙏

@sdogruyol sdogruyol merged commit 1966189 into kemalcr:master Feb 17, 2023
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.

2 participants