-
Notifications
You must be signed in to change notification settings - Fork 39
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
Confusing to use in an application already using log/slog
#28
Comments
Aside from manually creating the It also seems to use |
It shouldn’t touch global slog, and you can pass options.Writer. it certainly could be better and more carefully designed, but I actually don’t have time and it works for my purposes. My suggestion is to submit a PR, or write your own package @diamondburned |
Should a PR make the v3 breaking changes described in this issue, or should it make the changes without being breaking? Part of the current issue is the fact that the README documents |
FWIW, I had configured log level to DEBUG in my program, then switched to the new httplog, and all DEBUG messages were gone until I explicitly re-defined the log level to DEBUG in the call of NewLogger. |
An additional feature request here: I'd like to get the |
may i suggest to just leave NewLogger as is but remove it from the docs and create a new function? |
I ran across this as well, and managed to work around it by using this as my middleware:
Definitely a work-around, but it seems to be doing the job for now. |
hey guys, @stephenafamo @diamondburned @dropwhile @bronger https://github.com/go-chi/httplog/pull/43/files this fixes the default slogger being modified, the json options etc work... normally you would enable json logging in prod since its structured logging which is ingested by cloud providers... |
@diamondburned it logs on os.StdOut by default on the default pretty logger which is meant for dev purposes... and well for prod you should make it os.StdErr imo... which is why the option exists :) |
@stephenafamo i agree, ill work on including an optional slogger in the create httplog function, so that you can inject your own slogger, other than that what else do you feel is wrong with the structure? |
Thanks for taking a look at this @Shubhaankar-Sharma I have nothing else to add to what was in my original comment.
|
It is likely that an application using
slog
will want to use its own pre-configured*slog.Logger
or provide aslog.Handler
that a logger should be created from.The design of the package makes it confusing to do this.
Sure, I can manually create a
httplog.Logger
, but the options mix what is required for "configuring the logger" and what is used in logging.I would suggest that the package asks only for a
slog.Handler
and only uses options to configure what to log:I understand that my changes would be breaking (so that means a
v3
so soon after av2
), but I think it becomes easier to use with other slog compatible packages.The text was updated successfully, but these errors were encountered: