Confusing to use in an application already using `log/slog`

[stephenafamo]

It is likely that an application using slog will want to use its own pre-configured *slog.Logger or provide a slog.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.

l := &httplog.Logger{
  Logger: myExistingSlogLogger,
  Options: httplog.Options{
    JSON: true, // I'm guessing this does nothing
  },
}

I would suggest that the package asks only for a slog.Handler and only uses options to configure what to log:

func Handler(h slog.Handler, opts Options) func(next http.Handler) http.Handler {
	if h == nil {
		h = slog.Default().Handler()
	}
	// implementation
}

type Options struct {
	// Level determines what level to log request details at
	Level slog.Level 

	// Concise mode includes fewer log details during the request flow. For example
	// excluding details like request content length, user-agent and other details.
	// This is useful if during development your console is too noisy.
	Concise bool

	// RequestHeaders enables logging of all request headers, however sensitive
	// headers like authorization, cookie and set-cookie are hidden.
	RequestHeaders bool

	// SkipRequestHeaders are additional requests headers which are redacted from the logs
	SkipRequestHeaders []string

	// QuietDownRoutes are routes which are temporarily excluded from logging for a QuietDownPeriod after it occurs
	// for the first time
	// to cancel noise from logging for routes that are known to be noisy.
	QuietDownRoutes []string

	// QuietDownPeriod is the duration for which a route is excluded from logging after it occurs for the first time
	// if the route is in QuietDownRoutes
	QuietDownPeriod time.Duration
}

I understand that my changes would be breaking (so that means a v3 so soon after a v2), but I think it becomes easier to use with other slog compatible packages.

[diamondburned]

Aside from manually creating the httplog.Logger, the README suggests to use httplog.NewLogger which not only has this issue but also somehow messes up the global slog (how does it even do this?).

It also seems to use os.Stdout as the default, which is very strange. There is also currently no way to change the level that the logger logs at. The LogLevel in Options is confusingly designed to override the global one, not to determine what level to log at.

[pkieltyka]

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

[diamondburned]

submit a PR

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 httplog.NewLogger already, so I'm not sure what the preferable approach is here.

[bronger]

It shouldn’t touch global slog, and you can pass options.Writer.

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.

[judofyr]

An additional feature request here: I'd like to get the slog.Handler from the ctx dynamically. This is already being configured with different attributes in earlier middlewares.

[aep]

submit a PR

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 httplog.NewLogger already, so I'm not sure what the preferable approach is here.

may i suggest to just leave NewLogger as is but remove it from the docs and create a new function? the current one is clearly wrong, so no point in keeping it except for not breaking existing code, specifically go-chi examples where it doesnt really matter.

[dropwhile]

I ran across this as well, and managed to work around it by using this as my middleware:

var skipPaths = []string{}
var logOptions = httplog.Options{
	Concise:        true,
	RequestHeaders: true,
	HideRequestHeaders: []string{
		"accept",
		"accept-encoding",
		"accept-language",
		"accept-ranges",
		"connection",
		"cookie",
		"user-agent",
	},
	QuietDownRoutes: []string{
		"/",
		"/ping",
	},
	QuietDownPeriod: 10 * time.Second,
}

func RequestLogger() func(next http.Handler) http.Handler {
	httpLogger := &httplog.Logger{
		Logger:  slog.Default(),
		Options: logOptions,
	}
	return httplog.Handler(httpLogger, skipPaths)
}

Definitely a work-around, but it seems to be doing the job for now.

[Shubhaankar-Sharma]

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...

[Shubhaankar-Sharma]

@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 :)

[Shubhaankar-Sharma]

@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?

[stephenafamo]

Thanks for taking a look at this @Shubhaankar-Sharma

I have nothing else to add to what was in my original comment.

1. Ability to use my own Handler
2. Clearer configuration