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

Log middleware has problem receiving log level setting #44

Closed
jackalcooper opened this issue Feb 3, 2017 · 8 comments
Closed

Log middleware has problem receiving log level setting #44

jackalcooper opened this issue Feb 3, 2017 · 8 comments
Assignees

Comments

@jackalcooper
Copy link
Contributor

jackalcooper commented Feb 3, 2017

When I set it to :error
image

When I set it to :info
image

But there are all requests with 200 HTTP status code.

This is the log setting in my client:

middleware Maxwell.Middleware.Logger, [log_level: :info]
@zhongwencool
Copy link
Owner

Now:
Logger middleware will log all requests by Logger(even 200 code).
Logger writes log according to log_level(error, info, warning, debug).

But:
Add a option to decide which request should not logged will be better.@secretworry

   middleware Maxwell.Middleware.Logger, [code: 200]

And change default behavior to not logged 200 code.

@secretworry
Copy link
Collaborator

secretworry commented Feb 7, 2017

@zhongwencool Yes, but I think a better solution would be providing a mechanism to let the user define which status code should be logged in which log_level. For example, combining the option log_level and codes would do this job.
To log all requests in info level

middleware Maxwell.Middleware.Logger, [log_level: :info]

To log 2xx with info, others with error

middleware Maxwell.Middleware.Logger, log_level: :info, codes: 200..299
middleware Maxwell.Middleware.Logger, log_level: :error, codes: [0..199, 300..999]

But this method is too verbose for defining an exclusive case. Another idea is to use log_level to provide default log level, and use codes as a mapping from codes to log_level to define special cases.

middleware Maxwell.Middleware.Logger, log_level: :error, codes: %{200..299 => :info, 300..399 => :warn}

@zhongwencool any other ideas?

@zhongwencool
Copy link
Owner

zhongwencool commented Feb 7, 2017

I like the second idea~
but I want to put default log level into map:

middleware Maxwell.Middleware.Logger, log_level: [200..299 => :info,
                             300..399 => :warn, :default => :error]

@falood
Copy link
Collaborator

falood commented Feb 8, 2017

I have some suggestion about the DSL.

  1. we use Keyword not Map for options
  2. the number of status code is unsure but the number of log_level is 5
    so, how about such DSL? it's customized for complex requirements, but too long for simple use case 😞
middleware Maxwell.Middleware.Logger, log_level: [
  info: [1..100, 200..299],
  warn: 300..399,
  error: [:default, 123, 400..599],
]

@jackalcooper
Copy link
Contributor Author

I got to say I kind of like @falood 's idea. Besides the keyword list accepting a callback could be even better.

@secretworry
Copy link
Collaborator

@zhongwencool I don't think putting default value together with special values is a good idea.
So adding an option :default_level with default value :error would be more simple and clear.

middleware Maxwell.Middleware.Logger
# are equivalent to
middleware Maxwell.Middleware.Logger default: :error, log_level: %{200..299: :info, 300..399: :warn}

@falood I agree with your suggestion on using the level as key. The concern here is the level of the log, so using it as the key would be more concise.
Besides, your example can be simplified as

middleware Maxwell.Middleware.Logger, default_level: :error, log_level: [
  info: [1..100, 200..299],
  warn: 300..399
]
# or with using :error as default level
middleware Maxwell.Middleware.Logger, log_level: [
  info: [1..100, 200..299],
  warn: 300..399
]

@secretworry
Copy link
Collaborator

@jackalcooper Using an inline function as an argument of the macro is not easy and will make this middleware be too complicate. If you have more complicated filtering conditions, writing a customized middleware would be a better solution.

@falood
Copy link
Collaborator

falood commented Feb 8, 2017

so, the default level don't need other value, how about use follow DSL, I think it's clear enough.

middleware Maxwell.Middleware.Logger, level: [
  info: [1..100, 200..299],
  warn: 300..399,
  error: :default
]

if default level missed, I think it's better ignore the missed status code.
people who just want error log, should use

middleware Maxwell.Middleware.Logger, level: [
  error: 400..599
]

falood added a commit to falood/maxwell that referenced this issue Feb 13, 2017
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

No branches or pull requests

4 participants