filters applied after request mutated, potential to waste resources

[timruffles]

The filter options look designed to allow you to have different configurations per endpoint. However, the filters are applied after the request/response recording is set up. Therefore:

slogchi.NewWithConfig(slog, slogchi.Config{
    WithRequestBody: true,
    Filters: []slogchi.Filter{ slogchi.AcceptPathPrefix("/foo") },
})
slogchi.NewWithConfig(slog, slogchi.Config{
    WithRequestBody: true,
    Filters: []slogchi.Filter{ slogchi.AcceptPathPrefix("/bar") },
})

will result in two copies of request body being buffered for every request, regardless of whether it matches the filters.

I see 9501154, but this is still applied after the request is mutated with new reader/writers. I think filters should be processed here before the r.Body code is run.

[samber]

Why do you declare 2 middlewares?

Why not just use:

slogchi.Accept(func (ww middleware.WrapResponseWriter, r *http.Request) bool {
	return strings.HasPrefix(r.URL.Path, "/foo") || strings.HasPrefix(r.URL.Path,"/bar") 
}),

or:

slogchi.AcceptPathPrefix("/foo", "/bar")

Or we could implement a slogchi.Or sub-rule processor.

[timruffles]

The same issue happens with one. Even the routes which the filter excludes have their request bodies buffered into memory, due to the issue in the placement of filtering I mentioned.

But, that aside, as far as I can see you'll need >1 middleware whenever you want different logging options for different routes. E.g. a set where req body is logged, a set where res body is logged, a set where neither are logged.

[samber]

It's impossible to execute filters before handlers, because filters can use data from response writer (eg: ww.Status()).

[timruffles]

Good point. One option would be adding pre filters, i.e. a filter func only accepting request?

It's very unfortunate that if WithRequestBody is set true, all endpoints will buffer req body into memory regardless of whether or not they're logged. At the very least it'd be worth documenting this so people aren't surprised, as it's fairly common that apps have a subset of high QPS endpoints where logging is excluded precisely for performance reasons.